Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(593)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 865943002: Update SSL/malware/net error interstitial design (Closed)

Created:
5 years, 11 months ago by edwardjung
Modified:
5 years, 11 months ago
CC:
chromium-reviews, arv+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

+ On small screens the navigation buttons are anchored to the bottom of the screen. + Tweaks to the text colour + Change button styling of safe browsing page BUG=422882 Committed: https://crrev.com/7ee5b21903f8a466e8f8e995e62354deceb987f0 Cr-Commit-Position: refs/heads/master@{#313280}

Patch Set 1 #

Patch Set 2 : Remove iOS specific code #

Patch Set 3 : Revert aria label back to 'for', as it broke the checkbox #

Patch Set 4 : Fix hidden buttons on offline interstitial on CrOS #

Total comments: 3

Patch Set 5 : Fix background colour for nav-wrapper on safe browsing state #

Total comments: 3

Patch Set 6 : Switch button layout switching from JS to CSS flexbox #

Total comments: 5

Patch Set 7 : Address code review comments #

Patch Set 8 : Fix merge issue #

Patch Set 9 : Refactor the safe browsing checkbox to pass the HTML presubmit #

Patch Set 10 : Last minute font tweak requested #

Unified diffs Side-by-side diffs Delta from patch set Stats (+502 lines, -92 lines) Patch
M chrome/browser/resources/security_warnings/interstitial_v2.css View 1 2 3 4 5 6 7 8 9 10 chunks +369 lines, -12 lines 0 comments Download
M chrome/browser/resources/security_warnings/interstitial_v2.html View 1 2 3 4 5 6 7 8 1 chunk +22 lines, -17 lines 0 comments Download
M chrome/browser/resources/security_warnings/interstitial_v2.js View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
A chrome/browser/resources/security_warnings/interstitial_v2_mobile.js View 1 2 3 4 5 1 chunk +28 lines, -0 lines 0 comments Download
M chrome/browser/resources/security_warnings/safe_browsing.js View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/resources/neterror.css View 1 2 3 4 5 6 7 8 9 6 chunks +12 lines, -7 lines 0 comments Download
M chrome/renderer/resources/neterror.html View 1 2 3 4 5 6 2 chunks +54 lines, -54 lines 0 comments Download
M chrome/renderer/resources/neterror.js View 1 2 3 4 5 3 chunks +8 lines, -2 lines 0 comments Download

Messages

Total messages: 25 (7 generated)
edwardjung
Hi Erik, Please could you review these updates to the design of the interstitials. Mainly ...
5 years, 11 months ago (2015-01-23 12:19:40 UTC) #2
arv (Not doing code reviews)
https://codereview.chromium.org/865943002/diff/80001/chrome/browser/resources/security_warnings/interstitial_v2_mobile.js File chrome/browser/resources/security_warnings/interstitial_v2_mobile.js (right): https://codereview.chromium.org/865943002/diff/80001/chrome/browser/resources/security_warnings/interstitial_v2_mobile.js#newcode22 chrome/browser/resources/security_warnings/interstitial_v2_mobile.js:22: document.body.insertBefore(navWrapper, mainFrame.nextSibling); Why is this done in js instead ...
5 years, 11 months ago (2015-01-23 15:03:20 UTC) #3
edwardjung
https://codereview.chromium.org/865943002/diff/80001/chrome/browser/resources/security_warnings/interstitial_v2_mobile.js File chrome/browser/resources/security_warnings/interstitial_v2_mobile.js (right): https://codereview.chromium.org/865943002/diff/80001/chrome/browser/resources/security_warnings/interstitial_v2_mobile.js#newcode22 chrome/browser/resources/security_warnings/interstitial_v2_mobile.js:22: document.body.insertBefore(navWrapper, mainFrame.nextSibling); On 2015/01/23 15:03:20, arv wrote: > Why ...
5 years, 11 months ago (2015-01-23 16:33:42 UTC) #4
edwardjung
So I checked my previous version and using a pure flexbox approach was problematic on ...
5 years, 11 months ago (2015-01-23 22:04:25 UTC) #5
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/865943002/diff/100001/chrome/browser/resources/security_warnings/interstitial_v2.js File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/865943002/diff/100001/chrome/browser/resources/security_warnings/interstitial_v2.js#newcode147 chrome/browser/resources/security_warnings/interstitial_v2.js:147: $('main-content').classList.toggle('hidden', false); remove('hidden') https://codereview.chromium.org/865943002/diff/100001/chrome/renderer/resources/neterror.html File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/865943002/diff/100001/chrome/renderer/resources/neterror.html#newcode45 ...
5 years, 11 months ago (2015-01-23 22:13:13 UTC) #6
edwardjung
Your comments addressed. Thanks. https://codereview.chromium.org/865943002/diff/100001/chrome/browser/resources/security_warnings/interstitial_v2.js File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/865943002/diff/100001/chrome/browser/resources/security_warnings/interstitial_v2.js#newcode147 chrome/browser/resources/security_warnings/interstitial_v2.js:147: $('main-content').classList.toggle('hidden', false); On 2015/01/23 22:13:13, ...
5 years, 11 months ago (2015-01-26 10:55:38 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865943002/120001
5 years, 11 months ago (2015-01-26 10:56:17 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/38270) Try jobs failed on following ...
5 years, 11 months ago (2015-01-26 11:02:02 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865943002/140001
5 years, 11 months ago (2015-01-26 11:41:28 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/38274) Try jobs failed on following ...
5 years, 11 months ago (2015-01-26 11:46:24 UTC) #15
edwardjung
@felt, I had to refactor the checkbox and label as it was failing the HTML ...
5 years, 11 months ago (2015-01-26 16:03:07 UTC) #17
arv (Not doing code reviews)
LGTM
5 years, 11 months ago (2015-01-27 00:53:48 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/865943002/180001
5 years, 11 months ago (2015-01-27 13:59:09 UTC) #20
felt
lgtm, thanks for the improvements. presumably you have, but to double check: have you made ...
5 years, 11 months ago (2015-01-27 15:19:02 UTC) #21
commit-bot: I haz the power
Committed patchset #10 (id:180001)
5 years, 11 months ago (2015-01-27 15:26:29 UTC) #22
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/7ee5b21903f8a466e8f8e995e62354deceb987f0 Cr-Commit-Position: refs/heads/master@{#313280}
5 years, 11 months ago (2015-01-27 15:27:27 UTC) #23
edwardjung
On 2015/01/27 15:19:02, felt wrote: > lgtm, thanks for the improvements. > > presumably you ...
5 years, 11 months ago (2015-01-27 15:53:00 UTC) #24
dconnelly
5 years, 11 months ago (2015-01-27 16:40:57 UTC) #25

Powered by Google App Engine
This is Rietveld 408576698