DNS probes: several tweaks to error page updating

DNS probes: several tweaks to error page updating

1. Re-render the entire error page, not just the details, so we can see the new title (in the NO_INTERNET case).
2. Choose icon-offline for the NO_INTERNET case.
3. Remove the old icon class when filling in the new one.

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

[Deprecated (see juliatuttle), 2013-08-26 19:26:13]

PTAL, mmenke.

[mmenke, 2013-08-26 19:34:42]

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
File chrome/renderer/resources/neterror.js (right):

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
chrome/renderer/resources/neterror.js:39: classList.add(newClass);
I wonder about this scaling.  Not too bad with two classes, but if there were 5,
or 5-dozen...  Also seems like it's very likely to regress when more classes are
added.

[Deprecated (see juliatuttle), 2013-08-26 20:24:40]

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
File chrome/renderer/resources/neterror.js (right):

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
chrome/renderer/resources/neterror.js:39: classList.add(newClass);
On 2013/08/26 19:34:42, mmenke wrote:
> I wonder about this scaling.  Not too bad with two classes, but if there were
5,
> or 5-dozen...  Also seems like it's very likely to regress when more classes
are
> added.

Agreed.  If it got to be more than two or three, I'd stuff the previous class
somewhere and remove just it.

[mmenke, 2013-08-26 20:26:24]

On 2013/08/26 20:24:40, ttuttle wrote:
>
https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
> File chrome/renderer/resources/neterror.js (right):
> 
>
https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
> chrome/renderer/resources/neterror.js:39: classList.add(newClass);
> On 2013/08/26 19:34:42, mmenke wrote:
> > I wonder about this scaling.  Not too bad with two classes, but if there
were
> 5,
> > or 5-dozen...  Also seems like it's very likely to regress when more classes
> are
> > added.
> 
> Agreed.  If it got to be more than two or three, I'd stuff the previous class
> somewhere and remove just it.

Right, but I think there's a high probability that whoever does the updating
will overlook this.

[Deprecated (see juliatuttle), 2013-08-26 21:44:17]

PTAL, mmenke.

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
File chrome/renderer/resources/neterror.js (right):

https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
chrome/renderer/resources/neterror.js:39: classList.add(newClass);
On 2013/08/26 20:24:40, ttuttle wrote:
> On 2013/08/26 19:34:42, mmenke wrote:
> > I wonder about this scaling.  Not too bad with two classes, but if there
were
> 5,
> > or 5-dozen...  Also seems like it's very likely to regress when more classes
> are
> > added.
> 
> Agreed.  If it got to be more than two or three, I'd stuff the previous class
> somewhere and remove just it.

Alright, fixed.  It now stashes the last name in a property "last_icon_class" of
the classList.  It's ugly, but it works.

[mmenke, 2013-08-27 14:25:47]

On 2013/08/26 21:44:17, ttuttle wrote:
> PTAL, mmenke.
> 
>
https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
> File chrome/renderer/resources/neterror.js (right):
> 
>
https://chromiumcodereview.appspot.com/23434002/diff/4001/chrome/renderer/res...
> chrome/renderer/resources/neterror.js:39: classList.add(newClass);
> On 2013/08/26 20:24:40, ttuttle wrote:
> > On 2013/08/26 19:34:42, mmenke wrote:
> > > I wonder about this scaling.  Not too bad with two classes, but if there
> were
> > 5,
> > > or 5-dozen...  Also seems like it's very likely to regress when more
classes
> > are
> > > added.
> > 
> > Agreed.  If it got to be more than two or three, I'd stuff the previous
class
> > somewhere and remove just it.
> 
> Alright, fixed.  It now stashes the last name in a property "last_icon_class"
of
> the classList.  It's ugly, but it works.

LGTM.  Yea, it's ugly, but I don't have a suggestion that's clearly better.

[Deprecated (see juliatuttle), 2013-08-27 16:46:23]

Adding jhawkins for OWNERS review.

[James Hawkins, 2013-08-27 18:07:15]

LGTM with nits.

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/common/loca...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/common/loca...
chrome/common/localized_error.cc:486: } else {
Optional nit: else not required.

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/renderer/re...
File chrome/renderer/resources/neterror.html (right):

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/renderer/re...
chrome/renderer/resources/neterror.html:17: <img class="icon"
jseval="updateIconClass(this.classList, iconClass)">
nit: Missing one more space of indentation.

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/renderer/re...
chrome/renderer/resources/neterror.html:49: jsvalues=".title:errorDetails">
nit: 4 space indentation required for wrapped lines.

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/renderer/re...
File chrome/renderer/resources/neterror.js (right):

https://chromiumcodereview.appspot.com/23434002/diff/17001/chrome/renderer/re...
chrome/renderer/resources/neterror.js:30: function updateForDnsProbe(strings) {
nit: Document methods and params.

[commit-bot: I haz the power, 2013-08-28 15:39:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/23434002/32001

[commit-bot: I haz the power, 2013-08-28 19:01:29]

Change committed as 220066