Use dinosaur icon on "you are offline" error page.

Use dinosaur icon on "you are offline" error page.

This changes the ERR_INTERNET_DISCONNECTED net error page to show a
dinosaur icon instead of the sad face icon.

BUG=263464
R=mmenke@chromium.org, nkostylev@chromium.org, thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213710

[newt (away), 2013-07-23 22:19:10]

+ygorshenin for cros changes
+mmenke for neterror.html and localized_error.cc
+thakis for OWNER goodness

[Nico, 2013-07-23 22:39:04]

owners lgtm, assuming mmenke and ygorshenin are happy

[mmenke, 2013-07-24 14:54:17]

Also, have you run pngcrush on the icons?

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error...
chrome/common/localized_error.cc:533: error_code ==
net::ERR_INTERNET_DISCONNECTED);
Rather than using a bool, suggest a string, since it scales better, if we ever
add icons for different pages.

Also, you should check the error_domain is net::kErrorDomain as well.  There's a
check below on it, and you may want to move your logic down there (Or not).

[Nico, 2013-07-24 15:55:10]

Oshima runs some script to crush all images before releases.
On Jul 24, 2013 7:54 AM, <mmenke@chromium.org> wrote:

> Also, have you run pngcrush on the icons?
>
>
> https://codereview.chromium.**org/19477006/diff/1/chrome/**
>
common/localized_error.cc<https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc>
> File chrome/common/localized_error.**cc (right):
>
> https://codereview.chromium.**org/19477006/diff/1/chrome/**
>
common/localized_error.cc#**newcode533<https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc#newcode533>
> chrome/common/localized_error.**cc:533: error_code ==
> net::ERR_INTERNET_**DISCONNECTED);
> Rather than using a bool, suggest a string, since it scales better, if
> we ever add icons for different pages.
>
> Also, you should check the error_domain is net::kErrorDomain as well.
> There's a check below on it, and you may want to move your logic down
> there (Or not).
>
>
https://codereview.chromium.**org/19477006/<https://codereview.chromium.org/1...
>

[newt (away), 2013-07-24 17:30:52]

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error...
chrome/common/localized_error.cc:533: error_code ==
net::ERR_INTERNET_DISCONNECTED);
On 2013/07/24 14:54:17, mmenke wrote:
> Rather than using a bool, suggest a string, since it scales better, if we ever
> add icons for different pages.
> Also, you should check the error_domain is net::kErrorDomain as well.  There's
a
> check below on it, and you may want to move your logic down there (Or not).

Shouldn't error_code == net::ERR_INTERNET_DISCONNECTED imply that error_domain
== net::kErrorDomain?

[mmenke, 2013-07-24 17:51:04]

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error...
chrome/common/localized_error.cc:533: error_code ==
net::ERR_INTERNET_DISCONNECTED);
On 2013/07/24 17:30:52, newt wrote:
> On 2013/07/24 14:54:17, mmenke wrote:
> > Rather than using a bool, suggest a string, since it scales better, if we
ever
> > add icons for different pages.
> > Also, you should check the error_domain is net::kErrorDomain as well. 
There's
> a
> > check below on it, and you may want to move your logic down there (Or not).
> 
> Shouldn't error_code == net::ERR_INTERNET_DISCONNECTED imply that error_domain
> == net::kErrorDomain?

In practice, that's currently the case.  However, long term, there's no
guarantee that some other error domain will never use the same error code.

[newt (away), 2013-07-24 18:37:15]

On 2013/07/24 17:51:04, mmenke wrote:
>
https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error.cc
> File chrome/common/localized_error.cc (right):
> 
>
https://codereview.chromium.org/19477006/diff/1/chrome/common/localized_error...
> chrome/common/localized_error.cc:533: error_code ==
> net::ERR_INTERNET_DISCONNECTED);
> On 2013/07/24 17:30:52, newt wrote:
> > On 2013/07/24 14:54:17, mmenke wrote:
> > > Rather than using a bool, suggest a string, since it scales better, if we
> ever
> > > add icons for different pages.
> > > Also, you should check the error_domain is net::kErrorDomain as well. 
> There's
> > a
> > > check below on it, and you may want to move your logic down there (Or
not).
> > 
> > Shouldn't error_code == net::ERR_INTERNET_DISCONNECTED imply that
error_domain
> > == net::kErrorDomain?
> 
> In practice, that's currently the case.  However, long term, there's no
> guarantee that some other error domain will never use the same error code.

Gotcha. I didn't realize that the error numbers could potentially overlap.

[newt (away), 2013-07-24 18:38:13]

ready for re-review, thanks!

[mmenke, 2013-07-24 18:45:42]

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:332: <div><img class="icon"
jsvalues=".className:iconClass"></div>
Doesn't setting classname overwrite the current class?

I believe you'd want something like:

jseval="$this.classList.add(iconClass)" (Or maybe "this."?)

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:359: <img class="error-img"
jsvalues=".title:errorDetails">
Need to modify this, icon, too, for iframes (Admittedly, less likely to see this
error in iframes than main frames, but it can sitll happen).

[Nikita (slow), 2013-07-24 18:52:06]

You'll have to commit images in a separate CL so that you could then run trybots
on this one.

[newt (away), 2013-07-24 21:02:34]

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:332: <div><img class="icon"
jsvalues=".className:iconClass"></div>
On 2013/07/24 18:45:42, mmenke wrote:
> Doesn't setting classname overwrite the current class?
> 
> I believe you'd want something like:
> 
> jseval="$this.classList.add(iconClass)" (Or maybe "this."?)

Good catch. Fixed. (it's "this" btw)

https://codereview.chromium.org/19477006/diff/10001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:359: <img class="error-img"
jsvalues=".title:errorDetails">
On 2013/07/24 18:45:42, mmenke wrote:
> Need to modify this, icon, too, for iframes (Admittedly, less likely to see
this
> error in iframes than main frames, but it can sitll happen).

Done.

[mmenke, 2013-07-24 21:04:14]

localized_error and neterror LGTM!

[Nikita (slow), 2013-07-25 00:35:03]

chromeos/login lgtm

[commit-bot: I haz the power, 2013-07-25 20:42:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/newt@chromium.org/19477006/24001

[newt (away), 2013-07-25 22:40:50]

Committed patchset #4 manually as r213710 (presubmit successful).