Chromium Code Reviews
Help | Chromium Project | Sign in
(8)

Issue 2788323002: SafeBrowsing: update interstitial layouts (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 weeks ago by Nate Fischer
Modified:
5 days, 2 hours ago
CC:
chromium-reviews, Jialiu Lin
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

SafeBrowsing: change interstitial sizes This CL changes the CSS max-height, max-width, etc. dimensions for determining when to use mobile vs. desktop interstitial layouts. In particular, it targets: * wide and short views -> mobile landscape * skinny and tall views -> mobile portrait * wide and medium-height -> mobile landscape (w/ details on the same page) The phablet layout has been removed because it seems to actually be better to just use the mobile layout instead. This also allows the mobile layout to remain centered even for very wide views (parts of it were left-justified before), and reduces the top-margin for the icon in the mobile layout, since we were leaving a huge gap. BUG=707481 Review-Url: https://codereview.chromium.org/2788323002 Cr-Commit-Position: refs/heads/master@{#466746} Committed: https://chromium.googlesource.com/chromium/src/+/2f0527c9fcfdb3b53628acec1b3195563f71ec51

Patch Set 1 #

Patch Set 2 : Allow details to show below when view height is 600-736px #

Patch Set 3 : Don't let details button overlap back-to-safety #

Patch Set 4 : Increase top margin for middle-sized interstitials #

Patch Set 5 : Rebase off master #

Total comments: 10

Patch Set 6 : Switch to proportional margins #

Total comments: 4

Patch Set 7 : Address reviewer comments #

Messages

Total messages: 43 (22 generated)
Nate Fischer
nparker@: for owners approval edwardjung@: for UX design For your convenience, I've uploaded screenshots here: ...
3 weeks, 3 days ago (2017-04-04 23:32:56 UTC) #12
edwardjung
Nate, I'm currently OOO at a conference so won't be able to look at this ...
3 weeks, 3 days ago (2017-04-05 12:54:37 UTC) #13
Nate Fischer
On 2017/04/05 at 12:54:37, edwardjung wrote: > Nate, I'm currently OOO at a conference so ...
3 weeks, 3 days ago (2017-04-05 22:37:56 UTC) #14
edwardjung
> Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for both ...
3 weeks, 2 days ago (2017-04-06 13:01:50 UTC) #15
Nate Fischer
On 2017/04/06 at 13:01:50, edwardjung wrote: > > Thanks, Edward! Also, I just uploaded screenshots ...
3 weeks, 1 day ago (2017-04-07 04:44:31 UTC) #16
edwardjung
I had a chat with maxwalker@ (our designer who is working on webview safe browsing ...
2 weeks, 4 days ago (2017-04-11 14:58:55 UTC) #17
Nate Fischer
On 2017/04/11 at 14:58:55, edwardjung wrote: > I had a chat with maxwalker@ (our designer ...
2 weeks, 4 days ago (2017-04-11 17:12:12 UTC) #18
Nate Fischer
Edward, anything you want me to try out here? It wasn't clear to me if ...
2 weeks, 3 days ago (2017-04-12 16:10:11 UTC) #19
edwardjung
On 2017/04/12 16:10:11, Nate Fischer wrote: > Edward, anything you want me to try out ...
1 week, 2 days ago (2017-04-20 09:55:09 UTC) #20
Nate Fischer
On 2017/04/20 at 09:55:09, edwardjung wrote: > On 2017/04/12 16:10:11, Nate Fischer wrote: > > ...
1 week, 2 days ago (2017-04-20 19:03:41 UTC) #21
Nate Fischer
On 2017/04/20 at 19:03:41, Nate Fischer wrote: > On 2017/04/20 at 09:55:09, edwardjung wrote: > ...
1 week, 2 days ago (2017-04-20 23:07:12 UTC) #26
edwardjung
Thanks Nate, I think this is mostly there. I would like to switch to proportional ...
1 week, 1 day ago (2017-04-21 12:16:30 UTC) #31
Nate Fischer
I don't have an iOS device to try this out on, but this LGTM otherwise. ...
1 week, 1 day ago (2017-04-21 18:28:43 UTC) #32
edwardjung
Just a couple of other changes. Thanks. https://codereview.chromium.org/2788323002/diff/80001/components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js File components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js (right): https://codereview.chromium.org/2788323002/diff/80001/components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js#newcode16 components/security_interstitials/core/browser/resources/interstitial_v2_mobile.js:16: '(max-height: 640px) ...
5 days, 12 hours ago (2017-04-24 10:30:41 UTC) #33
Nate Fischer
https://codereview.chromium.org/2788323002/diff/100001/components/security_interstitials/core/browser/resources/interstitial_v2.css File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2788323002/diff/100001/components/security_interstitials/core/browser/resources/interstitial_v2.css#newcode438 components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin: 24px auto 12px; On 2017/04/24 at 10:30:41, edwardjung ...
5 days, 4 hours ago (2017-04-24 19:04:53 UTC) #34
edwardjung
lgtm
5 days, 3 hours ago (2017-04-24 19:17:24 UTC) #35
Nate Fischer
On 2017/04/24 19:17:24, edwardjung wrote: > lgtm Thanks for the help on this, Edward! nparker@, ...
5 days, 3 hours ago (2017-04-24 20:01:08 UTC) #36
Nathan Parker (ooo till May 1)
echoing edwarjung's LGTM, for OWNERS. Thanks Nate.
5 days, 3 hours ago (2017-04-24 20:08:50 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2788323002/120001
5 days, 3 hours ago (2017-04-24 20:13:15 UTC) #39
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2f0527c9fcfdb3b53628acec1b3195563f71ec51
5 days, 2 hours ago (2017-04-24 20:36:30 UTC) #42
hcarmona
5 days ago (2017-04-24 23:04:34 UTC) #43
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2842633002/ by hcarmona@chromium.org.

The reason for reverting is: This looks like the cause of failures here:
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C...

First seen here:
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46