Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2788323002: SafeBrowsing: update interstitial layouts (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 3 weeks ago by Nate Fischer
Modified:
6 months 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: ...
6 months, 3 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks ago (2017-04-05 22:37:56 UTC) #14
edwardjung
> Thanks, Edward! Also, I just uploaded screenshots for Nexus 5X and 6P for both ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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 ...
6 months, 1 week 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 ...
6 months 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: > > ...
6 months 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: > ...
6 months 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 ...
6 months 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. ...
6 months 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) ...
6 months 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 ...
6 months ago (2017-04-24 19:04:53 UTC) #34
edwardjung
lgtm
6 months 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@, ...
6 months ago (2017-04-24 20:01:08 UTC) #36
Nathan Parker
echoing edwarjung's LGTM, for OWNERS. Thanks Nate.
6 months 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
6 months 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
6 months ago (2017-04-24 20:36:30 UTC) #42
hcarmona
6 months 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 81bcdb8aa