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

Issue 545973003: Update network error template to new design (Closed)

Created:
6 years, 3 months ago by edwardjung
Modified:
6 years, 2 months ago
CC:
chromium-reviews, felt
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Update network error template to new design BUG=414233 Committed: https://crrev.com/f8cb5584fe0d38a3d3f19d4bb8cb92c753f8d1f3 Cr-Commit-Position: refs/heads/master@{#296209}

Patch Set 1 #

Patch Set 2 : Added primary paragraph strings for non offline errors #

Patch Set 3 : Updated generic error icon and removing commented code #

Patch Set 4 : Remove debug code from JS file. #

Total comments: 37

Patch Set 5 : Address code review comments #

Total comments: 24

Patch Set 6 : Fix styling comments #

Patch Set 7 : Fix diagnose button placement on Chrome OS #

Total comments: 38

Patch Set 8 : JS style updates #

Total comments: 34

Patch Set 9 : More JS style changes #

Total comments: 14

Patch Set 10 : Fix style oversights #

Patch Set 11 : Fix indentation #

Total comments: 6

Patch Set 12 : Convert static methods to functions. #

Total comments: 1

Patch Set 13 : Rename touch enable const #

Patch Set 14 : Revert details link back to a button #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2419 lines, -218 lines) Patch
M chrome/app/chromium_strings.grd View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +2 lines, -11 lines 0 comments Download
M chrome/app/google_chrome_strings.grd View 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/common/localized_error.cc View 1 2 3 chunks +12 lines, -5 lines 0 comments Download
A chrome/renderer/resources/default_100_percent/common/error_network_generic.png View 1 2 Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-cloud.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-error-offline.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-horizon.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-obstacle-large-sprite.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-obstacle-small-sprite.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-offline-trex.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-restart.png View Binary file 0 comments Download
A chrome/renderer/resources/default_100_percent/offline/100-text-sprite.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/common/error_network_generic.png View 1 2 Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-cloud.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-error-offline.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-horizon.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-obstacle-large-sprite.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-obstacle-small-sprite.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-offline-trex.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-restart.png View Binary file 0 comments Download
A chrome/renderer/resources/default_200_percent/offline/200-text-sprite.png View 1 Binary file 0 comments Download
M chrome/renderer/resources/neterror.css View 1 2 3 4 5 6 6 chunks +61 lines, -141 lines 0 comments Download
M chrome/renderer/resources/neterror.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +82 lines, -60 lines 0 comments Download
M chrome/renderer/resources/neterror.js View 1 2 3 4 5 1 chunk +14 lines, -1 line 0 comments Download
A chrome/renderer/resources/offline.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2234 lines, -0 lines 0 comments Download
A chrome/renderer/resources/sounds/button-press.mp3 View 1 2 3 4 5 6 7 8 Binary file 0 comments Download
A chrome/renderer/resources/sounds/hit.mp3 View 1 2 3 4 5 6 7 8 Binary file 0 comments Download
A chrome/renderer/resources/sounds/score-reached.mp3 View 1 2 3 4 5 6 7 8 Binary file 0 comments Download

Messages

Total messages: 31 (7 generated)
edwardjung
Please see the bug for specific extra details which explains the reasoning behind files added ...
6 years, 3 months ago (2014-09-15 14:52:11 UTC) #2
sky
LGTM - but I'm adding arv to make sure the html/css changes are ok. Wait ...
6 years, 3 months ago (2014-09-15 16:15:31 UTC) #4
arv (Not doing code reviews)
https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resources/neterror.css File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resources/neterror.css#newcode50 chrome/renderer/resources/neterror.css:50: width: 41px; Keep these in alphabetical order https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resources/neterror.html File ...
6 years, 3 months ago (2014-09-15 17:35:44 UTC) #5
edwardjung
Thanks for the quick review. I've addressed all the comments. Wonder if you had any ...
6 years, 3 months ago (2014-09-16 13:14:33 UTC) #6
arv (Not doing code reviews)
Sorry for all the comments. This code is pretty good. I'm just trying to help ...
6 years, 3 months ago (2014-09-16 14:20:21 UTC) #7
felt
Is the end result of this that the security warnings and neterrors have the same ...
6 years, 3 months ago (2014-09-16 15:08:34 UTC) #9
edwardjung
On 2014/09/16 15:08:34, felt wrote: > Is the end result of this that the security ...
6 years, 3 months ago (2014-09-17 09:13:21 UTC) #10
edwardjung
Gone through and updated. Thanks. https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resources/neterror.js File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resources/neterror.js#newcode138 chrome/renderer/resources/neterror.js:138: setButtonLayout(); On 2014/09/16 14:20:20, ...
6 years, 3 months ago (2014-09-17 12:12:35 UTC) #11
edwardjung
@arv did you have any more comments?
6 years, 3 months ago (2014-09-19 09:02:30 UTC) #12
arv (Not doing code reviews)
Sorry, this fell off my radar. A few more style nits. https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): ...
6 years, 3 months ago (2014-09-19 15:50:42 UTC) #13
edwardjung
No problem, please have another look. I removed the top author comment since this now ...
6 years, 3 months ago (2014-09-19 17:56:46 UTC) #14
arv (Not doing code reviews)
https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resources/offline.js#newcode142 chrome/renderer/resources/offline.js:142: ], -2 spaces Runner.imageSources = { LDPI: [ {name: ...
6 years, 3 months ago (2014-09-19 18:24:28 UTC) #15
edwardjung
Did another pass to correct the comments. Thanks. https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resources/offline.js#newcode142 chrome/renderer/resources/offline.js:142: ], ...
6 years, 3 months ago (2014-09-21 20:26:53 UTC) #16
arv (Not doing code reviews)
https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resources/offline.js#newcode878 chrome/renderer/resources/offline.js:878: Runner.GameOverPanel = function(canvas, textSprite, restartImg, dimensions) { I still ...
6 years, 3 months ago (2014-09-22 15:03:15 UTC) #17
edwardjung
Made the changes. Apologies that you've been spotting the same style issues that I've overlooked. ...
6 years, 3 months ago (2014-09-22 16:54:26 UTC) #18
arv (Not doing code reviews)
LGTM with nits/ I think that you could do a few less "statics" but at ...
6 years, 3 months ago (2014-09-22 17:02:44 UTC) #19
edwardjung
Thanks for the all the tips, learnt a lot. https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resources/offline.js#newcode81 chrome/renderer/resources/offline.js:81: ...
6 years, 3 months ago (2014-09-22 17:50:26 UTC) #20
arv (Not doing code reviews)
Still LGTM Thanks for the extra cleanup. https://codereview.chromium.org/545973003/diff/220001/chrome/renderer/resources/offline.js File chrome/renderer/resources/offline.js (right): https://codereview.chromium.org/545973003/diff/220001/chrome/renderer/resources/offline.js#newcode87 chrome/renderer/resources/offline.js:87: var IS_TOUCHENABLED ...
6 years, 3 months ago (2014-09-22 18:05:33 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545973003/240001
6 years, 3 months ago (2014-09-23 08:27:37 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_swarming/builds/18825)
6 years, 3 months ago (2014-09-23 09:44:48 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/545973003/260001
6 years, 3 months ago (2014-09-23 16:38:35 UTC) #27
commit-bot: I haz the power
Committed patchset #14 (id:260001) as cadb3af4494f2a14930436d13c5b69d525acb364
6 years, 3 months ago (2014-09-23 18:01:44 UTC) #28
commit-bot: I haz the power
Patchset 14 (id:??) landed as https://crrev.com/f8cb5584fe0d38a3d3f19d4bb8cb92c753f8d1f3 Cr-Commit-Position: refs/heads/master@{#296209}
6 years, 3 months ago (2014-09-23 18:02:25 UTC) #29
dmazzoni
6 years, 2 months ago (2014-09-25 17:41:19 UTC) #31
Message was sent while issue was closed.
Nicely done! Didn't take long for the game to be discovered:

http://thenextweb.com/google/2014/09/25/googles-latest-chrome-build-hidden-ga...

Powered by Google App Engine
This is Rietveld 408576698