Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837233002:
Reland of SafeBrowsing: update interstitial layouts (Closed)
|
Created:
3 years, 8 months ago by Nate Fischer Modified:
3 years, 7 months ago CC:
chromium-reviews, felt, Nathan Parker Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland of SafeBrowsing: update interstitial layouts (patchset #1 id:1 of https://codereview.chromium.org/2842633002/ )
Reason for revert:
Fixing build issues
Original issue's description:
> Revert of SafeBrowsing: update interstitial layouts (patchset #7 id:120001 of https://codereview.chromium.org/2788323002/ )
>
> Reason for revert:
> This looks like the cause of failures here: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29
>
> First seen here:
> https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%281%29/builds/37590
>
> Original issue's 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
>
> TBR=edwardjung@chromium.org,nparker@chromium.org,ntfschr@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=707481
>
> Review-Url: https://codereview.chromium.org/2842633002
> Cr-Commit-Position: refs/heads/master@{#466811}
> Committed: https://chromium.googlesource.com/chromium/src/+/06dfd71d425ac5b0c7cef3060aee354946dc13b9
TBR=edwardjung@chromium.org,nparker@chromium.org,hcarmona@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=707481
Review-Url: https://codereview.chromium.org/2837233002
Cr-Commit-Position: refs/heads/master@{#467789}
Committed: https://chromium.googlesource.com/chromium/src/+/9a822f4b8c9d7d1ec7df123e48d24118ad102f00
Patch Set 1 #Patch Set 2 : Properly set hidden class to fix test failures #
Total comments: 5
Patch Set 3 : Use 5.5vh instead for details padding-top #Patch Set 4 : Revert previous changes, fix the JS code in test #Patch Set 5 : Remove padding from hidden elements, add it to visible elements #
Total comments: 4
Patch Set 6 : Fix nits #
Total comments: 4
Patch Set 7 : Fix formatting #
Messages
Total messages: 52 (27 generated)
Created Reland of SafeBrowsing: update interstitial layouts
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
edwardjung@, could you take another look? The test failures happened because ChromeOS tests create a different sized window, which was formerly the Desktop layout and now uses mobile layout, exposing a bug in the mobile layout. The mobile layout didn't properly hide the details. Fixing the bug fixed the test failures. This should have no effect on appearance. I tested this locally on my machine (let me know if someone knows how to run the bot) and everything passes.
https://codereview.chromium.org/2837233002/diff/60001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/60001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:128: transition: none; Are you sure this fixes the issue? Looking at the code, in the mobile layout, these attributes are overridden by the media query, so the details are still 'visible'. #details.hidden, #main-content.hidden { display: block; height: 0; opacity: 0; overflow: hidden; transition: none; } https://codereview.chromium.org/2837233002/diff/60001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:382: } This wasn't in the original CL, it is required for the bug fix? Suggestion to use proportional units, 5.5vh gives you ~ a range of 20 to 40px.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837233002/diff/60001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/60001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:128: transition: none; On 2017/04/25 at 10:32:04, edwardjung wrote: > Are you sure this fixes the issue? > > Looking at the code, in the mobile layout, these attributes are overridden by the media query, so the details are still 'visible'. > > #details.hidden, > #main-content.hidden { > display: block; > height: 0; > opacity: 0; > overflow: hidden; > transition: none; > } That code in the media query looked like a bug, so I already removed it in patchset 2. If it's in another spot (I only see line 414 in the original file), please let me know. https://codereview.chromium.org/2837233002/diff/60001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:382: } On 2017/04/25 at 10:32:04, edwardjung wrote: > This wasn't in the original CL, it is required for the bug fix? > > Suggestion to use proportional units, 5.5vh gives you ~ a range of 20 to 40px. This has to do with the change above. Setting `display = block` for details.hidden,main-content.hidden caused the padding-bottom to be turned on for both (even while hidden). Because main-content sits above details, its padding-bottom = 40px acted like a padding-top for details (which was probably a mistake). Since main-content.hidden is actually hidden now, I had to add the padding back to the un-hidden details. Your suggestion for 5.5vh looks good though, so I'll go with that instead.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2837233002/diff/60001/components/security_int... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/60001/components/security_int... components/security_interstitials/core/browser/resources/interstitial_v2.css:128: transition: none; Apologies, I missed the removal of that other code. That block wasn't a bug. The idea behind using opacity was so that the details text comes in with a fade in transition rather than just appearing directly. It's a subtle difference but feels nicer. Can you fix that browser test instead? Checking for opacity rather than display block when the mobile size nav kicks in? On 2017/04/25 19:04:42, Nate Fischer wrote: > On 2017/04/25 at 10:32:04, edwardjung wrote: > > Are you sure this fixes the issue? > > > > Looking at the code, in the mobile layout, these attributes are overridden by > the media query, so the details are still 'visible'. > > > > #details.hidden, > > #main-content.hidden { > > display: block; > > height: 0; > > opacity: 0; > > overflow: hidden; > > transition: none; > > } > > That code in the media query looked like a bug, so I already removed it in > patchset 2. If it's in another spot (I only see line 414 in the original file), > please let me know.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> That block wasn't a bug. The idea behind using opacity was so that the details text comes in with a fade in transition rather than just appearing directly. It's a subtle difference but feels nicer. I see the fade is gone by this change. This is a little confusing, since the transition is on #details, not #details.hidden. My knowledge on this is limited, perhaps you can clarify it for me. Is this because a `display: none` kills the CSS transition, even if it applies to the state we're transitioning away from? MDN suggests starting the transition a bit after changing `display: none` to `display: block`, perhaps this is the cleanest fix? > Can you fix that browser test instead? Checking for opacity rather than display block when the mobile size nav kicks in? Checking opacity doesn't work for #proceed-link. Even though it's invisible, it doesn't inherit the opacity value from its parent node. In general, it's pretty difficult to properly check visibility in vanilla JS: http://www.useallfive.com/thoughts/javascript-tool-detect-if-a-dom-element-is.... Suggestions are appreciated. The other approach I've considered is to enforce a window size for the tests. This doesn't seem great though, since the tests shouldn't really depend on window size.
> That block wasn't a bug. The idea behind using opacity was so that the details text comes in with a fade in transition rather than just appearing directly. It's a subtle difference but feels nicer. I see the fade is gone by this change. This is a little confusing, since the transition is on #details, not #details.hidden. My knowledge on this is limited, perhaps you can clarify it for me. Is this because a `display: none` kills the CSS transition, even if it applies to the state we're transitioning away from? MDN suggests starting the transition a bit after changing `display: none` to `display: block`, perhaps this is the cleanest fix? > Can you fix that browser test instead? Checking for opacity rather than display block when the mobile size nav kicks in? Checking opacity doesn't work for #proceed-link. Even though it's invisible, it doesn't inherit the opacity value from its parent node. In general, it's pretty difficult to properly check visibility in vanilla JS: http://www.useallfive.com/thoughts/javascript-tool-detect-if-a-dom-element-is.... Suggestions are appreciated. The other approach I've considered is to enforce a window size for the tests. This doesn't seem great though, since the tests shouldn't really depend on window size.
Description was changed from ========== Reland of SafeBrowsing: update interstitial layouts (patchset #1 id:1 of https://codereview.chromium.org/2842633002/ ) Reason for revert: Fixing build issues Original issue's description: > Revert of SafeBrowsing: update interstitial layouts (patchset #7 id:120001 of https://codereview.chromium.org/2788323002/ ) > > Reason for revert: > 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... > > Original issue's 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/+/2f0527c9fcfdb3b53628acec1b31... > > TBR=edwardjung@chromium.org,nparker@chromium.org,ntfschr@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=707481 > > Review-Url: https://codereview.chromium.org/2842633002 > Cr-Commit-Position: refs/heads/master@{#466811} > Committed: https://chromium.googlesource.com/chromium/src/+/06dfd71d425ac5b0c7cef3060aee... TBR=edwardjung@chromium.org,nparker@chromium.org,hcarmona@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=707481 ========== to ========== Reland of SafeBrowsing: update interstitial layouts (patchset #1 id:1 of https://codereview.chromium.org/2842633002/ ) Reason for revert: Fixing build issues Original issue's description: > Revert of SafeBrowsing: update interstitial layouts (patchset #7 id:120001 of https://codereview.chromium.org/2788323002/ ) > > Reason for revert: > 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... > > Original issue's 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/+/2f0527c9fcfdb3b53628acec1b31... > > TBR=edwardjung@chromium.org,nparker@chromium.org,ntfschr@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=707481 > > Review-Url: https://codereview.chromium.org/2842633002 > Cr-Commit-Position: refs/heads/master@{#466811} > Committed: https://chromium.googlesource.com/chromium/src/+/06dfd71d425ac5b0c7cef3060aee... TBR=edwardjung@chromium.org,nparker@chromium.org,hcarmona@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=707481 ==========
cc'ing felt in case she has any suggestions on improving test robustness.
On 2017/04/26 02:04:46, Nate Fischer wrote: > > That block wasn't a bug. The idea behind using opacity was so that the details > text comes in with a fade in transition rather than just appearing directly. > It's a subtle difference but feels nicer. > > I see the fade is gone by this change. This is a little confusing, since the > transition is on #details, not #details.hidden. My knowledge on this is limited, > perhaps you can clarify it for me. Is this because a `display: none` kills the > CSS transition, even if it applies to the state we're transitioning away from? > MDN suggests starting the transition a bit after changing `display: none` to > `display: block`, perhaps this is the cleanest fix? Yes 'display: none' means that the transitions are not honoured as there is no starting point to transition from. So #details.hidden overrides this with block but hiding it with a opacity and height. I tried a delay when this was first implemented and it wasn't satisfactory, if I recall there was some jank on some devices, possibly iOS where the WebView isn't as up to date as Chrome. Things might be different now so it's worth a trying again. I'll do some tests and get back to you. > > > Can you fix that browser test instead? Checking for opacity rather than > display block when the mobile size nav kicks in? > > Checking opacity doesn't work for #proceed-link. Even though it's invisible, it > doesn't inherit the opacity value from its parent node. > > In general, it's pretty difficult to properly check visibility in vanilla JS: > http://www.useallfive.com/thoughts/javascript-tool-detect-if-a-dom-element-is.... > Suggestions are appreciated. Alternatively just checking for the 'hidden' class on the details element? Is that a robust enough test? > > The other approach I've considered is to enforce a window size for the tests. > This doesn't seem great though, since the tests shouldn't really depend on > window size.
On 2017/04/26 20:22:17, edwardjung wrote: > On 2017/04/26 02:04:46, Nate Fischer wrote: > > > That block wasn't a bug. The idea behind using opacity was so that the > details > > text comes in with a fade in transition rather than just appearing directly. > > It's a subtle difference but feels nicer. > > > > I see the fade is gone by this change. This is a little confusing, since the > > transition is on #details, not #details.hidden. My knowledge on this is > limited, > > perhaps you can clarify it for me. Is this because a `display: none` kills the > > CSS transition, even if it applies to the state we're transitioning away from? > > MDN suggests starting the transition a bit after changing `display: none` to > > `display: block`, perhaps this is the cleanest fix? > > Yes 'display: none' means that the transitions are not honoured as there is no > starting point to transition from. > So #details.hidden overrides this with block but hiding it with a opacity and > height. > > I tried a delay when this was first implemented and it wasn't satisfactory, if I > recall there was some jank on some devices, possibly iOS where the WebView isn't > as up to date as Chrome. Things might be different now so it's worth a trying > again. I'll do some tests and get back to you. > Ok, thanks for the explanation. Let me know if it turns out to be a reasonable approach. > > > > > Can you fix that browser test instead? Checking for opacity rather than > > display block when the mobile size nav kicks in? > > > > Checking opacity doesn't work for #proceed-link. Even though it's invisible, > it > > doesn't inherit the opacity value from its parent node. > > > > In general, it's pretty difficult to properly check visibility in vanilla JS: > > > http://www.useallfive.com/thoughts/javascript-tool-detect-if-a-dom-element-is.... > > Suggestions are appreciated. > > Alternatively just checking for the 'hidden' class on the details element? Is > that a robust enough test? > Unfortunately, that wouldn't help cases like proceed-link, since it doesn't inherit that class. > > > > The other approach I've considered is to enforce a window size for the tests. > > This doesn't seem great though, since the tests shouldn't really depend on > > window size.
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ntfschr@chromium.org
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ok, I think this should pass tests now. PS4: - This reverts my other changes and only changes the JS in the test code. - I tried checking for opacity, but it turns out that opacity is still 0 immediately after un-hiding something (the CSS transition is relatively slow). Checking for the class should not have a race condition, but it trusts that the 'hidden' class always does the right thing. PS5: - It didn't seem right to me that 'hidden' elements would have non-zero padding. Now hidden elements have padding = 0. Since the main-content (sitting above details) is completely hidden, details needs a top-padding to compensate. - This should have no effect on the css transition (the transition is still there from what I see). Not sure if this has any weird effect on iOS though. Edward, can you review for the minimal css changes (should be limited to PS5)? nparker@, can you review for the test changes and owners approval?
Thanks for test fix. Just a couple of small things. https://codereview.chromium.org/2837233002/diff/120001/components/security_in... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/120001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:421: padding-bottom: 0; Nit: Alphabetise. https://codereview.chromium.org/2837233002/diff/120001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin-bottom: 12px; Late request from our designer. Can we switch this to 5.69vh It was felt that the space below the icon collapsed too quickly. Having a proportional margin bottom provides enough spacing.
hcarmona@chromium.org changed reviewers: - hcarmona@chromium.org
Thanks, Edward! https://codereview.chromium.org/2837233002/diff/120001/components/security_in... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/120001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:421: padding-bottom: 0; On 2017/04/27 10:14:29, edwardjung wrote: > Nit: Alphabetise. Done. https://codereview.chromium.org/2837233002/diff/120001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:438: margin-bottom: 12px; On 2017/04/27 10:14:29, edwardjung wrote: > Late request from our designer. Can we switch this to 5.69vh > > It was felt that the space below the icon collapsed too quickly. Having a > proportional margin bottom provides enough spacing. Done.
You need to upload a new PS but lgtm
On 2017/04/27 18:56:24, edwardjung wrote: > You need to upload a new PS but lgtm Whoops, thanks. Uploaded now. Just waiting on nparker@ once he gets back for owners permission and reviewing the test change.
felt@chromium.org changed reviewers: + felt@chromium.org
surprise owners lgtm for /security_interstitials/ (sorry, ya still have to wait for nparker for /safebrowsing/) https://codereview.chromium.org/2837233002/diff/140001/components/security_in... File components/security_interstitials/core/browser/resources/interstitial_v2.css (right): https://codereview.chromium.org/2837233002/diff/140001/components/security_in... components/security_interstitials/core/browser/resources/interstitial_v2.css:472: margin-top: 10vh; should we move everything over to vh/vw? (not related to review / out of scope of this CL -- i'm just curious)
jialiul@chromium.org changed reviewers: + jialiul@chromium.org - nparker@chromium.org
LGTM for chrome/browser/safe_browsing :-) https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:579: node_id + R"(')));)"; nit: strange indentation.
Thanks, felt@ and Jialiu! https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:579: node_id + R"(')));)"; On 2017/04/27 20:44:40, Jialiu Lin wrote: > nit: strange indentation. I agree, that's what clang-format did. Is it acceptable to add clang-format directives to disable it for this block? Otherwise the next person to touch the code will probably wind up reformatting it.
https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:579: node_id + R"(')));)"; On 2017/04/27 20:47:54, Nate Fischer wrote: > On 2017/04/27 20:44:40, Jialiu Lin wrote: > > nit: strange indentation. > > I agree, that's what clang-format did. Is it acceptable to add clang-format > directives to disable it for this block? Otherwise the next person to touch the > code will probably wind up reformatting it. Sure.
On 2017/04/27 20:51:37, Jialiu Lin wrote: > https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... > File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): > > https://codereview.chromium.org/2837233002/diff/140001/chrome/browser/safe_br... > chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:579: node_id + > R"(')));)"; > On 2017/04/27 20:47:54, Nate Fischer wrote: > > On 2017/04/27 20:44:40, Jialiu Lin wrote: > > > nit: strange indentation. > > > > I agree, that's what clang-format did. Is it acceptable to add clang-format > > directives to disable it for this block? Otherwise the next person to touch > the > > code will probably wind up reformatting it. > > Sure. Ok, formatting is fixed in patchset 7.
The CQ bit was checked by ntfschr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from edwardjung@chromium.org, felt@chromium.org, jialiul@chromium.org Link to the patchset: https://codereview.chromium.org/2837233002/#ps160001 (title: "Fix formatting")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493327310831960,
"parent_rev": "63cfe1991cbd66f86963ed2e1afecb1c2ed5519e", "commit_rev":
"9a822f4b8c9d7d1ec7df123e48d24118ad102f00"}
Message was sent while issue was closed.
Description was changed from ========== Reland of SafeBrowsing: update interstitial layouts (patchset #1 id:1 of https://codereview.chromium.org/2842633002/ ) Reason for revert: Fixing build issues Original issue's description: > Revert of SafeBrowsing: update interstitial layouts (patchset #7 id:120001 of https://codereview.chromium.org/2788323002/ ) > > Reason for revert: > 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... > > Original issue's 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/+/2f0527c9fcfdb3b53628acec1b31... > > TBR=edwardjung@chromium.org,nparker@chromium.org,ntfschr@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=707481 > > Review-Url: https://codereview.chromium.org/2842633002 > Cr-Commit-Position: refs/heads/master@{#466811} > Committed: https://chromium.googlesource.com/chromium/src/+/06dfd71d425ac5b0c7cef3060aee... TBR=edwardjung@chromium.org,nparker@chromium.org,hcarmona@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=707481 ========== to ========== Reland of SafeBrowsing: update interstitial layouts (patchset #1 id:1 of https://codereview.chromium.org/2842633002/ ) Reason for revert: Fixing build issues Original issue's description: > Revert of SafeBrowsing: update interstitial layouts (patchset #7 id:120001 of https://codereview.chromium.org/2788323002/ ) > > Reason for revert: > 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... > > Original issue's 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/+/2f0527c9fcfdb3b53628acec1b31... > > TBR=edwardjung@chromium.org,nparker@chromium.org,ntfschr@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=707481 > > Review-Url: https://codereview.chromium.org/2842633002 > Cr-Commit-Position: refs/heads/master@{#466811} > Committed: https://chromium.googlesource.com/chromium/src/+/06dfd71d425ac5b0c7cef3060aee... TBR=edwardjung@chromium.org,nparker@chromium.org,hcarmona@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=707481 Review-Url: https://codereview.chromium.org/2837233002 Cr-Commit-Position: refs/heads/master@{#467789} Committed: https://chromium.googlesource.com/chromium/src/+/9a822f4b8c9d7d1ec7df123e48d2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9a822f4b8c9d7d1ec7df123e48d2...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/2848483004/ by ntfschr@chromium.org. The reason for reverting is: Bot breakages.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/2845093006/ by pfeldman@chromium.org. The reason for reverting is: Breaks the build, should not have been landed as NOTRY=true: https://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests....
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) identified this CL at revision 467789 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
