Viewing cached webpages while offline is not available on Chrome OS

Viewing cached webpages while offline is not available on Chrome OS

Fixed by creating the OfflineResourceThrottle in ChromeOS only if the
"enable-offline-load-stale-cache" flag is not enabled. This allows us to
go back to the default behavior if this flag is turned off.

BUG=433388
TEST=manually
R=oshima@chromium.org,rdsmith@chromium.org

Committed: https://crrev.com/b4fd13cb9dca2d60333e6e9211dd5b88b0ca9739
Cr-Commit-Position: refs/heads/master@{#313851}

[afakhry, 2015-01-15 23:38:43]

Patchset #1 (id:1) has been deleted

[Randy Smith (Not in Mondays), 2015-01-16 01:18:54]

A couple of high level thoughts:
* What testing have you done with this?  As mentioned elsewhere, I'm concerned
about making sure the functionality of the old ChromeOS error page is still
available, and given that I don't see any changes to the network error page, I'm
worried that this loses the functionality the ChromeOS error page had that the
network error page lacks.  Have you looked into that?  Similarly, have you
tested that POST requests are handled the same?  (Or decided they shouldn't be,
in which case the CL really should document that, and arguably that behavior
should be changed in a different CL.)

* I don't think the linkage with the Load Stale Cache flag makes sense.  If you
want a new flag/finch trial, that's awesome, but load stale cache is a basically
orthogonal feature to this change, and should be controllable separately from
it.

[afakhry, 2015-01-16 01:31:36]

On 2015/01/16 01:18:54, rdsmith wrote:
> A couple of high level thoughts:
> * What testing have you done with this?  As mentioned elsewhere, I'm concerned
> about making sure the functionality of the old ChromeOS error page is still
> available, and given that I don't see any changes to the network error page,
I'm
> worried that this loses the functionality the ChromeOS error page had that the
> network error page lacks.  Have you looked into that?  Similarly, have you
> tested that POST requests are handled the same?  (Or decided they shouldn't
be,
> in which case the CL really should document that, and arguably that behavior
> should be changed in a different CL.)
> 
> * I don't think the linkage with the Load Stale Cache flag makes sense.  If
you
> want a new flag/finch trial, that's awesome, but load stale cache is a
basically
> orthogonal feature to this change, and should be controllable separately from
> it.

There is no difference in the functionality of the old ChromeOS error page. All
it
 does it just displays an error message a long with a button "Diagnose". Which
is the 
same thing that is displayed by the page generated by NetErrorHelper.

As I told you earlier, I thought the Load Stale Cache flag is linked since the 
OfflineResourceThrottle doesn't support the offline load from cache feature, so 
if that flag is turned on, then we never create the OfflineResourceThrottle. I
can certainly
create a new flag if that would make more sense.

[oshima, 2015-01-16 18:18:21]

On 2015/01/16 01:31:36, afakhry wrote:
> On 2015/01/16 01:18:54, rdsmith wrote:
> > A couple of high level thoughts:
> > * What testing have you done with this?  As mentioned elsewhere, I'm
concerned
> > about making sure the functionality of the old ChromeOS error page is still
> > available, and given that I don't see any changes to the network error page,
> I'm
> > worried that this loses the functionality the ChromeOS error page had that
the
> > network error page lacks.  Have you looked into that?  Similarly, have you
> > tested that POST requests are handled the same?  (Or decided they shouldn't
> be,
> > in which case the CL really should document that, and arguably that behavior
> > should be changed in a different CL.)

The POST behavior on other platforms seems to be the right behavior. I talked to
albert (abodenha@) and he agreed.

Yes, we should enable this now so that we have enough time to catch issues in
this MS.
Let's just have a ineternal flag so that we can switch back to old behavior
quickly and
safely.

> > 
> > * I don't think the linkage with the Load Stale Cache flag makes sense.  If
> you
> > want a new flag/finch trial, that's awesome, but load stale cache is a
> basically
> > orthogonal feature to this change, and should be controllable separately
from
> > it.
> 
> There is no difference in the functionality of the old ChromeOS error page.
All
> it
>  does it just displays an error message a long with a button "Diagnose". Which
> is the 
> same thing that is displayed by the page generated by NetErrorHelper.
> 
> As I told you earlier, I thought the Load Stale Cache flag is linked since the

> OfflineResourceThrottle doesn't support the offline load from cache feature,
so 
> if that flag is turned on, then we never create the OfflineResourceThrottle. I
> can certainly
> create a new flag if that would make more sense.

[afakhry, 2015-01-16 23:51:20]

Added a separate flag "enable-new-offline-error-page" which is defaulted to
enabled if it's not explicitly set to be disabled.

[Randy Smith (Not in Mondays), 2015-01-20 17:25:44]

On 2015/01/16 01:31:36, afakhry wrote:
> On 2015/01/16 01:18:54, rdsmith wrote:
> > A couple of high level thoughts:
> > * What testing have you done with this?  As mentioned elsewhere, I'm
concerned
> > about making sure the functionality of the old ChromeOS error page is still
> > available, and given that I don't see any changes to the network error page,
> I'm
> > worried that this loses the functionality the ChromeOS error page had that
the
> > network error page lacks.  Have you looked into that?  Similarly, have you
> > tested that POST requests are handled the same?  (Or decided they shouldn't
> be,
> > in which case the CL really should document that, and arguably that behavior
> > should be changed in a different CL.)
> > 
> > * I don't think the linkage with the Load Stale Cache flag makes sense.  If
> you
> > want a new flag/finch trial, that's awesome, but load stale cache is a
> basically
> > orthogonal feature to this change, and should be controllable separately
from
> > it.
> 
> There is no difference in the functionality of the old ChromeOS error page.
All
> it
>  does it just displays an error message a long with a button "Diagnose". Which
> is the 
> same thing that is displayed by the page generated by NetErrorHelper.

Sorry, this still confuses me.  Two points:
* I don't see any "Diagnose" button when I force an offline error in Chrome
desktop.  I see "Details" and "Reload", but no diagnose.
* When trace out the source, the desktop error page source is
chrome/renderer/resources/neterror.{html,js,css}, and the chromeos error page
source is chrome/browser/resources/chromeos/offline_net_load.html and
chrome/browser/resources/chromeos/neterror.{js,css}.  (OfflineResourceThrottle
-> OfflineLoadPath -> IDR_OFFLINE_NET_LOAD_HTML -> offline_net_load.html).  Both
HTMLs include a diagnose button (which makes me confused why I'm not seeing it
when I force an error page on the desktop), but the implementation of the
diagnoseErrors() function in the JS is different.  Desktop:

function diagnoseErrors() {
  var extensionId = 'idddmepepmjcgiedknnmlbadcokidhoa';
  var diagnoseFrame = document.getElementById('diagnose-frame');
  diagnoseFrame.innerHTML =
      '<iframe src="chrome-extension://' + extensionId +
      '/index.html"></iframe>';
}

ChromeOS:

function diagnoseErrors() {
  window.domAutomationController.setAutomationId(1);
  window.domAutomationController.send('open_connectivity_diagnostics');
}

I'm not a ChromeOS expert, and if oshima@ tells me that the functionalities are
equivalent, I'll yield.  But it really looks to me like there's a (possibly two;
not sure why I'm not seeing the button on desktop) questions here.


> 
> As I told you earlier, I thought the Load Stale Cache flag is linked since the

> OfflineResourceThrottle doesn't support the offline load from cache feature,
so 
> if that flag is turned on, then we never create the OfflineResourceThrottle. I
> can certainly
> create a new flag if that would make more sense.

[afakhry, 2015-01-20 18:17:32]

On 2015/01/20 17:25:44, rdsmith wrote:
> On 2015/01/16 01:31:36, afakhry wrote:
> > On 2015/01/16 01:18:54, rdsmith wrote:
> > > A couple of high level thoughts:
> > > * What testing have you done with this?  As mentioned elsewhere, I'm
> concerned
> > > about making sure the functionality of the old ChromeOS error page is
still
> > > available, and given that I don't see any changes to the network error
page,
> > I'm
> > > worried that this loses the functionality the ChromeOS error page had that
> the
> > > network error page lacks.  Have you looked into that?  Similarly, have you
> > > tested that POST requests are handled the same?  (Or decided they
shouldn't
> > be,
> > > in which case the CL really should document that, and arguably that
behavior
> > > should be changed in a different CL.)
> > > 
> > > * I don't think the linkage with the Load Stale Cache flag makes sense. 
If
> > you
> > > want a new flag/finch trial, that's awesome, but load stale cache is a
> > basically
> > > orthogonal feature to this change, and should be controllable separately
> from
> > > it.
> > 
> > There is no difference in the functionality of the old ChromeOS error page.
> All
> > it
> >  does it just displays an error message a long with a button "Diagnose".
Which
> > is the 
> > same thing that is displayed by the page generated by NetErrorHelper.
> 
> Sorry, this still confuses me.  Two points:
> * I don't see any "Diagnose" button when I force an offline error in Chrome
> desktop.  I see "Details" and "Reload", but no diagnose.
> * When trace out the source, the desktop error page source is
> chrome/renderer/resources/neterror.{html,js,css}, and the chromeos error page
> source is chrome/browser/resources/chromeos/offline_net_load.html and
> chrome/browser/resources/chromeos/neterror.{js,css}.  (OfflineResourceThrottle
> -> OfflineLoadPath -> IDR_OFFLINE_NET_LOAD_HTML -> offline_net_load.html). 
Both
> HTMLs include a diagnose button (which makes me confused why I'm not seeing it
> when I force an error page on the desktop), but the implementation of the
> diagnoseErrors() function in the JS is different.  Desktop:
> 
> function diagnoseErrors() {
>   var extensionId = 'idddmepepmjcgiedknnmlbadcokidhoa';
>   var diagnoseFrame = document.getElementById('diagnose-frame');
>   diagnoseFrame.innerHTML =
>       '<iframe src="chrome-extension://' + extensionId +
>       '/index.html"></iframe>';
> }
> 
> ChromeOS:
> 
> function diagnoseErrors() {
>   window.domAutomationController.setAutomationId(1);
>   window.domAutomationController.send('open_connectivity_diagnostics');
> }
> 
> I'm not a ChromeOS expert, and if oshima@ tells me that the functionalities
are
> equivalent, I'll yield.  But it really looks to me like there's a (possibly
two;
> not sure why I'm not seeing the button on desktop) questions here.
> 
> 
> > 
> > As I told you earlier, I thought the Load Stale Cache flag is linked since
the
> 
> > OfflineResourceThrottle doesn't support the offline load from cache feature,
> so 
> > if that flag is turned on, then we never create the OfflineResourceThrottle.
I
> > can certainly
> > create a new flag if that would make more sense.

@rdsmith:
Exactly, when you click on the 'details' link in the ChromeOS error page, the
diagnose button will be shown on the page. Clicking on the diagnose button on
either the new or old error pages will take you to the same 'Connectivity
diagnostics' window.

[Randy Smith (Not in Mondays), 2015-01-20 18:19:05]

On 2015/01/20 18:17:32, afakhry wrote:
> On 2015/01/20 17:25:44, rdsmith wrote:
> > On 2015/01/16 01:31:36, afakhry wrote:
> > > On 2015/01/16 01:18:54, rdsmith wrote:
> > > > A couple of high level thoughts:
> > > > * What testing have you done with this?  As mentioned elsewhere, I'm
> > concerned
> > > > about making sure the functionality of the old ChromeOS error page is
> still
> > > > available, and given that I don't see any changes to the network error
> page,
> > > I'm
> > > > worried that this loses the functionality the ChromeOS error page had
that
> > the
> > > > network error page lacks.  Have you looked into that?  Similarly, have
you
> > > > tested that POST requests are handled the same?  (Or decided they
> shouldn't
> > > be,
> > > > in which case the CL really should document that, and arguably that
> behavior
> > > > should be changed in a different CL.)
> > > > 
> > > > * I don't think the linkage with the Load Stale Cache flag makes sense. 
> If
> > > you
> > > > want a new flag/finch trial, that's awesome, but load stale cache is a
> > > basically
> > > > orthogonal feature to this change, and should be controllable separately
> > from
> > > > it.
> > > 
> > > There is no difference in the functionality of the old ChromeOS error
page.
> > All
> > > it
> > >  does it just displays an error message a long with a button "Diagnose".
> Which
> > > is the 
> > > same thing that is displayed by the page generated by NetErrorHelper.
> > 
> > Sorry, this still confuses me.  Two points:
> > * I don't see any "Diagnose" button when I force an offline error in Chrome
> > desktop.  I see "Details" and "Reload", but no diagnose.
> > * When trace out the source, the desktop error page source is
> > chrome/renderer/resources/neterror.{html,js,css}, and the chromeos error
page
> > source is chrome/browser/resources/chromeos/offline_net_load.html and
> > chrome/browser/resources/chromeos/neterror.{js,css}. 
(OfflineResourceThrottle
> > -> OfflineLoadPath -> IDR_OFFLINE_NET_LOAD_HTML -> offline_net_load.html). 
> Both
> > HTMLs include a diagnose button (which makes me confused why I'm not seeing
it
> > when I force an error page on the desktop), but the implementation of the
> > diagnoseErrors() function in the JS is different.  Desktop:
> > 
> > function diagnoseErrors() {
> >   var extensionId = 'idddmepepmjcgiedknnmlbadcokidhoa';
> >   var diagnoseFrame = document.getElementById('diagnose-frame');
> >   diagnoseFrame.innerHTML =
> >       '<iframe src="chrome-extension://' + extensionId +
> >       '/index.html"></iframe>';
> > }
> > 
> > ChromeOS:
> > 
> > function diagnoseErrors() {
> >   window.domAutomationController.setAutomationId(1);
> >   window.domAutomationController.send('open_connectivity_diagnostics');
> > }
> > 
> > I'm not a ChromeOS expert, and if oshima@ tells me that the functionalities
> are
> > equivalent, I'll yield.  But it really looks to me like there's a (possibly
> two;
> > not sure why I'm not seeing the button on desktop) questions here.
> > 
> > 
> > > 
> > > As I told you earlier, I thought the Load Stale Cache flag is linked since
> the
> > 
> > > OfflineResourceThrottle doesn't support the offline load from cache
feature,
> > so 
> > > if that flag is turned on, then we never create the
OfflineResourceThrottle.
> I
> > > can certainly
> > > create a new flag if that would make more sense.
> 
> @rdsmith:
> Exactly, when you click on the 'details' link in the ChromeOS error page, the
> diagnose button will be shown on the page. Clicking on the diagnose button on
> either the new or old error pages will take you to the same 'Connectivity
> diagnostics' window.

But why is that button implemented differently?  It has been my belief that the
ChromeOS diagnostics hook into the OS in a different way from the desktop
diagnostics; am I confused?

[afakhry, 2015-01-20 18:28:41]

On 2015/01/20 18:19:05, rdsmith wrote:
> On 2015/01/20 18:17:32, afakhry wrote:
> > On 2015/01/20 17:25:44, rdsmith wrote:
> > > On 2015/01/16 01:31:36, afakhry wrote:
> > > > On 2015/01/16 01:18:54, rdsmith wrote:
> > > > > A couple of high level thoughts:
> > > > > * What testing have you done with this?  As mentioned elsewhere, I'm
> > > concerned
> > > > > about making sure the functionality of the old ChromeOS error page is
> > still
> > > > > available, and given that I don't see any changes to the network error
> > page,
> > > > I'm
> > > > > worried that this loses the functionality the ChromeOS error page had
> that
> > > the
> > > > > network error page lacks.  Have you looked into that?  Similarly, have
> you
> > > > > tested that POST requests are handled the same?  (Or decided they
> > shouldn't
> > > > be,
> > > > > in which case the CL really should document that, and arguably that
> > behavior
> > > > > should be changed in a different CL.)
> > > > > 
> > > > > * I don't think the linkage with the Load Stale Cache flag makes
sense. 
> > If
> > > > you
> > > > > want a new flag/finch trial, that's awesome, but load stale cache is a
> > > > basically
> > > > > orthogonal feature to this change, and should be controllable
separately
> > > from
> > > > > it.
> > > > 
> > > > There is no difference in the functionality of the old ChromeOS error
> page.
> > > All
> > > > it
> > > >  does it just displays an error message a long with a button "Diagnose".
> > Which
> > > > is the 
> > > > same thing that is displayed by the page generated by NetErrorHelper.
> > > 
> > > Sorry, this still confuses me.  Two points:
> > > * I don't see any "Diagnose" button when I force an offline error in
Chrome
> > > desktop.  I see "Details" and "Reload", but no diagnose.
> > > * When trace out the source, the desktop error page source is
> > > chrome/renderer/resources/neterror.{html,js,css}, and the chromeos error
> page
> > > source is chrome/browser/resources/chromeos/offline_net_load.html and
> > > chrome/browser/resources/chromeos/neterror.{js,css}. 
> (OfflineResourceThrottle
> > > -> OfflineLoadPath -> IDR_OFFLINE_NET_LOAD_HTML -> offline_net_load.html).

> > Both
> > > HTMLs include a diagnose button (which makes me confused why I'm not
seeing
> it
> > > when I force an error page on the desktop), but the implementation of the
> > > diagnoseErrors() function in the JS is different.  Desktop:
> > > 
> > > function diagnoseErrors() {
> > >   var extensionId = 'idddmepepmjcgiedknnmlbadcokidhoa';
> > >   var diagnoseFrame = document.getElementById('diagnose-frame');
> > >   diagnoseFrame.innerHTML =
> > >       '<iframe src="chrome-extension://' + extensionId +
> > >       '/index.html"></iframe>';
> > > }
> > > 
> > > ChromeOS:
> > > 
> > > function diagnoseErrors() {
> > >   window.domAutomationController.setAutomationId(1);
> > >   window.domAutomationController.send('open_connectivity_diagnostics');
> > > }
> > > 
> > > I'm not a ChromeOS expert, and if oshima@ tells me that the
functionalities
> > are
> > > equivalent, I'll yield.  But it really looks to me like there's a
(possibly
> > two;
> > > not sure why I'm not seeing the button on desktop) questions here.
> > > 
> > > 
> > > > 
> > > > As I told you earlier, I thought the Load Stale Cache flag is linked
since
> > the
> > > 
> > > > OfflineResourceThrottle doesn't support the offline load from cache
> feature,
> > > so 
> > > > if that flag is turned on, then we never create the
> OfflineResourceThrottle.
> > I
> > > > can certainly
> > > > create a new flag if that would make more sense.
> > 
> > @rdsmith:
> > Exactly, when you click on the 'details' link in the ChromeOS error page,
the
> > diagnose button will be shown on the page. Clicking on the diagnose button
on
> > either the new or old error pages will take you to the same 'Connectivity
> > diagnostics' window.
> 
> But why is that button implemented differently?  It has been my belief that
the
> ChromeOS diagnostics hook into the OS in a different way from the desktop
> diagnostics; am I confused?

There might be differences under the hood that I'm not aware of, and for that I
think Oshima can probably tell us why they're implemented differently.

[oshima, 2015-01-21 18:34:57]

On 2015/01/20 18:28:41, afakhry wrote:
> On 2015/01/20 18:19:05, rdsmith wrote:
> > On 2015/01/20 18:17:32, afakhry wrote:
> > > On 2015/01/20 17:25:44, rdsmith wrote:
> > > > On 2015/01/16 01:31:36, afakhry wrote:
> > > > > On 2015/01/16 01:18:54, rdsmith wrote:
> > > > > > A couple of high level thoughts:
> > > > > > * What testing have you done with this?  As mentioned elsewhere, I'm
> > > > concerned
> > > > > > about making sure the functionality of the old ChromeOS error page
is
> > > still
> > > > > > available, and given that I don't see any changes to the network
error
> > > page,
> > > > > I'm
> > > > > > worried that this loses the functionality the ChromeOS error page
had
> > that
> > > > the
> > > > > > network error page lacks.  Have you looked into that?  Similarly,
have
> > you
> > > > > > tested that POST requests are handled the same?  (Or decided they
> > > shouldn't
> > > > > be,
> > > > > > in which case the CL really should document that, and arguably that
> > > behavior
> > > > > > should be changed in a different CL.)
> > > > > > 
> > > > > > * I don't think the linkage with the Load Stale Cache flag makes
> sense. 
> > > If
> > > > > you
> > > > > > want a new flag/finch trial, that's awesome, but load stale cache is
a
> > > > > basically
> > > > > > orthogonal feature to this change, and should be controllable
> separately
> > > > from
> > > > > > it.
> > > > > 
> > > > > There is no difference in the functionality of the old ChromeOS error
> > page.
> > > > All
> > > > > it
> > > > >  does it just displays an error message a long with a button
"Diagnose".
> > > Which
> > > > > is the 
> > > > > same thing that is displayed by the page generated by NetErrorHelper.
> > > > 
> > > > Sorry, this still confuses me.  Two points:
> > > > * I don't see any "Diagnose" button when I force an offline error in
> Chrome
> > > > desktop.  I see "Details" and "Reload", but no diagnose.
> > > > * When trace out the source, the desktop error page source is
> > > > chrome/renderer/resources/neterror.{html,js,css}, and the chromeos error
> > page
> > > > source is chrome/browser/resources/chromeos/offline_net_load.html and
> > > > chrome/browser/resources/chromeos/neterror.{js,css}. 
> > (OfflineResourceThrottle
> > > > -> OfflineLoadPath -> IDR_OFFLINE_NET_LOAD_HTML ->
offline_net_load.html).
> 
> > > Both
> > > > HTMLs include a diagnose button (which makes me confused why I'm not
> seeing
> > it
> > > > when I force an error page on the desktop), but the implementation of
the
> > > > diagnoseErrors() function in the JS is different.  Desktop:
> > > > 
> > > > function diagnoseErrors() {
> > > >   var extensionId = 'idddmepepmjcgiedknnmlbadcokidhoa';
> > > >   var diagnoseFrame = document.getElementById('diagnose-frame');
> > > >   diagnoseFrame.innerHTML =
> > > >       '<iframe src="chrome-extension://' + extensionId +
> > > >       '/index.html"></iframe>';
> > > > }
> > > > 
> > > > ChromeOS:
> > > > 
> > > > function diagnoseErrors() {
> > > >   window.domAutomationController.setAutomationId(1);
> > > >   window.domAutomationController.send('open_connectivity_diagnostics');
> > > > }
> > > > 
> > > > I'm not a ChromeOS expert, and if oshima@ tells me that the
> functionalities
> > > are
> > > > equivalent, I'll yield.  But it really looks to me like there's a
> (possibly
> > > two;
> > > > not sure why I'm not seeing the button on desktop) questions here.
> > > > 
> > > > 
> > > > > 
> > > > > As I told you earlier, I thought the Load Stale Cache flag is linked
> since
> > > the
> > > > 
> > > > > OfflineResourceThrottle doesn't support the offline load from cache
> > feature,
> > > > so 
> > > > > if that flag is turned on, then we never create the
> > OfflineResourceThrottle.
> > > I
> > > > > can certainly
> > > > > create a new flag if that would make more sense.
> > > 
> > > @rdsmith:
> > > Exactly, when you click on the 'details' link in the ChromeOS error page,
> the
> > > diagnose button will be shown on the page. Clicking on the diagnose button
> on
> > > either the new or old error pages will take you to the same 'Connectivity
> > > diagnostics' window.
> > 
> > But why is that button implemented differently?  It has been my belief that
> the
> > ChromeOS diagnostics hook into the OS in a different way from the desktop
> > diagnostics; am I confused?
> 
> There might be differences under the hood that I'm not aware of, and for that
I
> think Oshima can probably tell us why they're implemented differently.

These differences are most likely due to just a historical reason. ChromeOS's
offline page
predates Chrome's one, and used to have two options "open network settings",
"load anyway".
My guess is that this page has gone through a couple of redesign, which leads to
somewhat similar,
but slightly different page.

I think it's good time to consolidate them.  afakhry@, can you confirm with
kuscher@ that
this is OK from UX/PM perspective?

[afakhry, 2015-01-22 00:07:23]

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (left):

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:139: }
This code created inconsistency ... It used to check if the 'msg' of the summary
is empty and if so, hides the "details" button but then unhides the
'help-box-outer' div. so technically there are more details to show and they
should be hidden by default with the possibility of showing them with the
'details' link. That's why i removed this block of code.

[Randy Smith (Not in Mondays), 2015-01-22 18:22:09]

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (left):

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:139: }
On 2015/01/22 00:07:23, afakhry wrote:
> This code created inconsistency ... It used to check if the 'msg' of the
summary
> is empty and if so, hides the "details" button but then unhides the
> 'help-box-outer' div. so technically there are more details to show and they
> should be hidden by default with the possibility of showing them with the
> 'details' link. That's why i removed this block of code.

I don't understand; could you explain in more detail?  (You might want to just
send me some screen shots directly.)  If there aren't any details to show,
hiding the Details button makes sense to me, and it sounds like this change
means it'll be shown whether clicking on it will do anything or not?

[afakhry, 2015-01-22 19:26:07]

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (left):

https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:139: }
On 2015/01/22 18:22:09, rdsmith wrote:
> On 2015/01/22 00:07:23, afakhry wrote:
> > This code created inconsistency ... It used to check if the 'msg' of the
> summary
> > is empty and if so, hides the "details" button but then unhides the
> > 'help-box-outer' div. so technically there are more details to show and they
> > should be hidden by default with the possibility of showing them with the
> > 'details' link. That's why i removed this block of code.
> 
> I don't understand; could you explain in more detail?  (You might want to just
> send me some screen shots directly.)  If there aren't any details to show,
> hiding the Details button makes sense to me, and it sounds like this change
> means it'll be shown whether clicking on it will do anything or not?

There are screenshots on the Bug thread. The problem with this code is that it
decides whether there are details based only on the |summary.msg|, while details
are much more than that (suggestions, error code, error details, diagnose button
...). There's always details to show, and that's why it unhides the
|help-box-outer| div.

[Randy Smith (Not in Mondays), 2015-01-22 19:43:52]

On 2015/01/22 19:26:07, afakhry wrote:
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> File chrome/renderer/resources/neterror.js (left):
> 
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> chrome/renderer/resources/neterror.js:139: }
> On 2015/01/22 18:22:09, rdsmith wrote:
> > On 2015/01/22 00:07:23, afakhry wrote:
> > > This code created inconsistency ... It used to check if the 'msg' of the
> > summary
> > > is empty and if so, hides the "details" button but then unhides the
> > > 'help-box-outer' div. so technically there are more details to show and
they
> > > should be hidden by default with the possibility of showing them with the
> > > 'details' link. That's why i removed this block of code.
> > 
> > I don't understand; could you explain in more detail?  (You might want to
just
> > send me some screen shots directly.)  If there aren't any details to show,
> > hiding the Details button makes sense to me, and it sounds like this change
> > means it'll be shown whether clicking on it will do anything or not?
> 
> There are screenshots on the Bug thread. The problem with this code is that it
> decides whether there are details based only on the |summary.msg|, while
details
> are much more than that (suggestions, error code, error details, diagnose
button
> ...). There's always details to show, and that's why it unhides the
> |help-box-outer| div.

I don't see the screenshots that compare the before and after based on your
change--can you give me more precise pointers?

Going based on guesswork, from the code I'm suspecting that if "msg" is blank,
all we're showing is the single line error code, and the judgement was made that
we should just show that rather than hide it behind a details link that takes up
the same screen real estate.  If that's true, than the removal of that code is a
real UI change that I'd like to get UI buy-in for (and I'm gently opposed, but
then I think it's worth avoiding changing UI except when you are specifically
targeting that UI.)

[afakhry, 2015-01-22 20:16:22]

On 2015/01/22 19:43:52, rdsmith wrote:
> On 2015/01/22 19:26:07, afakhry wrote:
> >
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> > File chrome/renderer/resources/neterror.js (left):
> > 
> >
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> > chrome/renderer/resources/neterror.js:139: }
> > On 2015/01/22 18:22:09, rdsmith wrote:
> > > On 2015/01/22 00:07:23, afakhry wrote:
> > > > This code created inconsistency ... It used to check if the 'msg' of the
> > > summary
> > > > is empty and if so, hides the "details" button but then unhides the
> > > > 'help-box-outer' div. so technically there are more details to show and
> they
> > > > should be hidden by default with the possibility of showing them with
the
> > > > 'details' link. That's why i removed this block of code.
> > > 
> > > I don't understand; could you explain in more detail?  (You might want to
> just
> > > send me some screen shots directly.)  If there aren't any details to show,
> > > hiding the Details button makes sense to me, and it sounds like this
change
> > > means it'll be shown whether clicking on it will do anything or not?
> > 
> > There are screenshots on the Bug thread. The problem with this code is that
it
> > decides whether there are details based only on the |summary.msg|, while
> details
> > are much more than that (suggestions, error code, error details, diagnose
> button
> > ...). There's always details to show, and that's why it unhides the
> > |help-box-outer| div.
> 
> I don't see the screenshots that compare the before and after based on your
> change--can you give me more precise pointers?
> 
> Going based on guesswork, from the code I'm suspecting that if "msg" is blank,
> all we're showing is the single line error code, and the judgement was made
that
> we should just show that rather than hide it behind a details link that takes
up
> the same screen real estate.  If that's true, than the removal of that code is
a
> real UI change that I'd like to get UI buy-in for (and I'm gently opposed, but
> then I think it's worth avoiding changing UI except when you are specifically
> targeting that UI.)

Take a look at LocalizedError::GetStrings() here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/loca...

If we don't fill the summary.msg (in the case of net::ERR_INTERNET_DISCONNECTED 
or chrome_common_net::DNS_PROBE_FINISHED_NO_INTERNET in Linux and ChromeOS) that
doesn't mean that other details are empty.

Take a look at #30 in the bug thread. It seems from a PM perspective, we want
that details link always visible to hide details that may not be interesting to
the normal user.

I will send you some screenshots on your mail :)

[oshima, 2015-01-22 20:19:40]

On 2015/01/22 19:43:52, rdsmith wrote:
> On 2015/01/22 19:26:07, afakhry wrote:
> >
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> > File chrome/renderer/resources/neterror.js (left):
> > 
> >
>
https://codereview.chromium.org/856643002/diff/60001/chrome/renderer/resource...
> > chrome/renderer/resources/neterror.js:139: }
> > On 2015/01/22 18:22:09, rdsmith wrote:
> > > On 2015/01/22 00:07:23, afakhry wrote:
> > > > This code created inconsistency ... It used to check if the 'msg' of the
> > > summary
> > > > is empty and if so, hides the "details" button but then unhides the
> > > > 'help-box-outer' div. so technically there are more details to show and
> they
> > > > should be hidden by default with the possibility of showing them with
the
> > > > 'details' link. That's why i removed this block of code.
> > > 
> > > I don't understand; could you explain in more detail?  (You might want to
> just
> > > send me some screen shots directly.)  If there aren't any details to show,
> > > hiding the Details button makes sense to me, and it sounds like this
change
> > > means it'll be shown whether clicking on it will do anything or not?
> > 
> > There are screenshots on the Bug thread. The problem with this code is that
it
> > decides whether there are details based only on the |summary.msg|, while
> details
> > are much more than that (suggestions, error code, error details, diagnose
> button
> > ...). There's always details to show, and that's why it unhides the
> > |help-box-outer| div.
> 
> I don't see the screenshots that compare the before and after based on your
> change--can you give me more precise pointers?
> 
> Going based on guesswork, from the code I'm suspecting that if "msg" is blank,
> all we're showing is the single line error code, and the judgement was made
that
> we should just show that rather than hide it behind a details link that takes
up
> the same screen real estate.  If that's true, than the removal of that code is
a
> real UI change that I'd like to get UI buy-in for (and I'm gently opposed, but
> then I think it's worth avoiding changing UI except when you are specifically
> targeting that UI.)

I agree with rdsmith@. This modification will change the behavior, and
if you believe this is better UI, you need to discuss with PM/UX folks first.

My suggestion is still adding explicit boolean value to show/hide the link as we
discussed yesterday.

[afakhry, 2015-01-22 22:24:13]

afakhry@chromium.org changed reviewers:
+ arv@chromium.org, edwardjung@chromium.org

[afakhry, 2015-01-22 22:27:23]

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
To mimic the behavior of the old ChromeOS's offline error page, we will never
hide the details link when it's ChromeOS.

[afakhry, 2015-01-23 01:35:46]

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
On 2015/01/22 22:27:23, afakhry wrote:
> To mimic the behavior of the old ChromeOS's offline error page, we will never
> hide the details link when it's ChromeOS.

@edwardjung, @arv: Can you please confirm that this change won't result in a
weird behavior for ChromeOS?

[afakhry, 2015-01-23 01:42:01]

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
On 2015/01/23 01:35:46, afakhry wrote:
> On 2015/01/22 22:27:23, afakhry wrote:
> > To mimic the behavior of the old ChromeOS's offline error page, we will
never
> > hide the details link when it's ChromeOS.
> 
> @edwardjung, @arv: Can you please confirm that this change won't result in a
> weird behavior for ChromeOS?

A bit of context: ChromeOS used to use:
- src/chrome/browser/resources/chromeos/neterror.js
- src/chrome/browser/resources/chromeos/offline_net_load.html

Now we are trying to consolidate and use the same error page used on Chrome:
- src/chrome/renderer/resources/neterror.js
- src/chrome/renderer/resources/neterror.html

With the exception that we don't want to hide the 'details' link.

[edwardjung, 2015-01-23 11:18:28]

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
On 2015/01/23 01:42:01, afakhry wrote:
> On 2015/01/23 01:35:46, afakhry wrote:
> > On 2015/01/22 22:27:23, afakhry wrote:
> > > To mimic the behavior of the old ChromeOS's offline error page, we will
> never
> > > hide the details link when it's ChromeOS.
> > 
> > @edwardjung, @arv: Can you please confirm that this change won't result in a
> > weird behavior for ChromeOS?
> 
> A bit of context: ChromeOS used to use:
> - src/chrome/browser/resources/chromeos/neterror.js
> - src/chrome/browser/resources/chromeos/offline_net_load.html
> 
> Now we are trying to consolidate and use the same error page used on Chrome:
> - src/chrome/renderer/resources/neterror.js
> - src/chrome/renderer/resources/neterror.html
> 
> With the exception that we don't want to hide the 'details' link.

It makes sense to consolidate. Is src/chrome/renderer/resources/neterror.html
being updated to provide the 'diagnose' button you show on CrOS?

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
On 2015/01/23 01:35:46, afakhry wrote:
> On 2015/01/22 22:27:23, afakhry wrote:
> > To mimic the behavior of the old ChromeOS's offline error page, we will
never
> > hide the details link when it's ChromeOS.
> 
> @edwardjung, @arv: Can you please confirm that this change won't result in a
> weird behavior for ChromeOS?

As long as there will always be something in the details section to show, this
should be okay. I can't remember but is there a difference between CrOS and
ChromiumOS where there is sometimes no detailed info / diagnose button. If
that's the case, maybe the JS should be changed to check for a diagnose button,
and remove the "not chromeos" if expression.

Maybe something like:

if (loadTimeData.valueExists('summary') &&
    !loadTimeData.getValue('summary').msg &&
   (!loadTimeData.valueExists('diagnose') || !loadTimeData.getValue('diagnose'))
{
…
}

You'll need to check this for non CrOS platforms.

[afakhry, 2015-01-23 17:49:25]

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:141: </if>
On 2015/01/23 11:18:28, edwardjung wrote:
> On 2015/01/23 01:35:46, afakhry wrote:
> > On 2015/01/22 22:27:23, afakhry wrote:
> > > To mimic the behavior of the old ChromeOS's offline error page, we will
> never
> > > hide the details link when it's ChromeOS.
> > 
> > @edwardjung, @arv: Can you please confirm that this change won't result in a
> > weird behavior for ChromeOS?
> 
> As long as there will always be something in the details section to show, this
> should be okay. I can't remember but is there a difference between CrOS and
> ChromiumOS where there is sometimes no detailed info / diagnose button. If
> that's the case, maybe the JS should be changed to check for a diagnose
button,
> and remove the "not chromeos" if expression.
> 
> Maybe something like:
> 
> if (loadTimeData.valueExists('summary') &&
>     !loadTimeData.getValue('summary').msg &&
>    (!loadTimeData.valueExists('diagnose') ||
!loadTimeData.getValue('diagnose'))
> {
> …
> }
> 
> You'll need to check this for non CrOS platforms.

The diagnose button is only specific to ChromeOS. Setting its value is guarded
with an #if defined(OS_CHROMEOS) in LocalizedError::GetStrings(). Adding this
will change the behavior on other platforms. 

The idea is, on ChromeOS, we always want to show the 'details' link and hide the
extra details by default. But we are afraid that this check:

  if (loadTimeData.valueExists('summary') &&
          !loadTimeData.getValue('summary').msg)

is there for a reason, especially that we don't set summary.msg in
LocalizedError::GetStrings() at:

if (error_code == net::ERR_INTERNET_DISCONNECTED ||
      error_code == chrome_common_net::DNS_PROBE_FINISHED_NO_INTERNET) 

That's why we need your opinion.

[edwardjung, 2015-01-23 18:39:56]

> 
> The diagnose button is only specific to ChromeOS. Setting its value is guarded
> with an #if defined(OS_CHROMEOS) in LocalizedError::GetStrings(). Adding this
> will change the behavior on other platforms. 
> 
> The idea is, on ChromeOS, we always want to show the 'details' link and hide
the
> extra details by default. But we are afraid that this check:
> 
>   if (loadTimeData.valueExists('summary') &&
>           !loadTimeData.getValue('summary').msg)
> 
> is there for a reason, especially that we don't set summary.msg in
> LocalizedError::GetStrings() at:
> 
> if (error_code == net::ERR_INTERNET_DISCONNECTED ||
>       error_code == chrome_common_net::DNS_PROBE_FINISHED_NO_INTERNET) 
> 
> That's why we need your opinion.

Thanks for explaining.

This is UX decision. Overall all interstitials like SSL / safe browsing / net
error would have an icon, heading, primary paragraph with a brief summary of the
error. Under the details you would have extra details / technical / ways to
troubleshoot etc. Currently net error pages, don't have a primary paragraph. The
exception is the offline status. In this scenario the promotion of the summary
text to primary paragraph meant no summary text and hence nothing to display
under the details. The JS check is there to prevent the details button being
shown when there is no summary content. However as you point out that on CrOS
the diagnose button is always shown in net errors, then I think it's fine to
have this exception.

The UX team are currently looking at consolidating the network error messages
and also adding primary text in all cases. So in future this JS check wouldn't
be necessary at.

[afakhry, 2015-01-23 19:55:14]

On 2015/01/23 18:39:56, edwardjung wrote:
> > 
> > The diagnose button is only specific to ChromeOS. Setting its value is
guarded
> > with an #if defined(OS_CHROMEOS) in LocalizedError::GetStrings(). Adding
this
> > will change the behavior on other platforms. 
> > 
> > The idea is, on ChromeOS, we always want to show the 'details' link and hide
> the
> > extra details by default. But we are afraid that this check:
> > 
> >   if (loadTimeData.valueExists('summary') &&
> >           !loadTimeData.getValue('summary').msg)
> > 
> > is there for a reason, especially that we don't set summary.msg in
> > LocalizedError::GetStrings() at:
> > 
> > if (error_code == net::ERR_INTERNET_DISCONNECTED ||
> >       error_code == chrome_common_net::DNS_PROBE_FINISHED_NO_INTERNET) 
> > 
> > That's why we need your opinion.
> 
> Thanks for explaining.
> 
> This is UX decision. Overall all interstitials like SSL / safe browsing / net
> error would have an icon, heading, primary paragraph with a brief summary of
the
> error. Under the details you would have extra details / technical / ways to
> troubleshoot etc. Currently net error pages, don't have a primary paragraph.
The
> exception is the offline status. In this scenario the promotion of the summary
> text to primary paragraph meant no summary text and hence nothing to display
> under the details. The JS check is there to prevent the details button being
> shown when there is no summary content. However as you point out that on CrOS
> the diagnose button is always shown in net errors, then I think it's fine to
> have this exception.
> 
> The UX team are currently looking at consolidating the network error messages
> and also adding primary text in all cases. So in future this JS check wouldn't
> be necessary at.

Does that mean that this change sounds good to you at the moment?

[edwardjung, 2015-01-23 21:18:12]

> Does that mean that this change sounds good to you at the moment?

Yes for the change to the JS.

[edwardjung, 2015-01-23 21:18:13]

> Does that mean that this change sounds good to you at the moment?

Yes for the change to the JS.

[oshima, 2015-01-23 21:49:04]

https://codereview.chromium.org/856643002/diff/80001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2164: },
I still think we don't need about flags. We want to consolidate it sooner and
this isn't something that we want a user to play with.

I'm ok to have "disable" command line flag in case we need it for
troubleshooting though.

[afakhry, 2015-01-24 00:53:16]

Removed the new offline error page from the about:flags page, but kept the
disable-new-offline-error-page command line switch for debugging and testing.

https://codereview.chromium.org/856643002/diff/80001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/856643002/diff/80001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:2164: },
On 2015/01/23 21:49:03, oshima wrote:
> I still think we don't need about flags. We want to consolidate it sooner and
> this isn't something that we want a user to play with.
> 
> I'm ok to have "disable" command line flag in case we need it for
> troubleshooting though.

Done.

[oshima, 2015-01-24 04:55:47]

lgtm with nits

https://codereview.chromium.org/856643002/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/856643002/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2152: 
nit: revert this change?

https://codereview.chromium.org/856643002/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/856643002/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50285: +  <int value="-2107097252"
label="disable-new-offline-error-page"/>
do you need this change? (I'll leave this to the owner though)

[afakhry, 2015-01-26 16:53:25]

afakhry@chromium.org changed reviewers:
+ mpearson@chromium.org
- arv@chromium.org

[afakhry, 2015-01-26 16:55:19]

@mpearson: Do we need to keep the change in histograms.xml if we added a
command-line switch, but we're not using it in the about:flags?

[Randy Smith (Not in Mondays), 2015-01-26 17:30:44]

Were you going to send me screen shots of what the ChromeOS page used to look
like and what it looks like now?  Or did that get resolved in some other
fashion?

[Mark P, 2015-01-26 18:43:31]

mpearson@chromium.org changed reviewers:
+ asvitkine@chromium.org
- mpearson@chromium.org

[Mark P, 2015-01-26 18:44:47]

On 2015/01/26 16:55:19, afakhry wrote:
> @mpearson: Do we need to keep the change in histograms.xml if we added a
> command-line switch, but we're not using it in the about:flags?

I'm not sure, mainly because I don't recall the reason we added the flag
recording for ChromeOS in histograms in the first place.  Re-assigning to Alexei
to decide because I'm sure he knows about this.

--mark

[afakhry, 2015-01-27 00:06:47]

On 2015/01/26 18:44:47, Mark P wrote:
> On 2015/01/26 16:55:19, afakhry wrote:
> > @mpearson: Do we need to keep the change in histograms.xml if we added a
> > command-line switch, but we're not using it in the about:flags?
> 
> I'm not sure, mainly because I don't recall the reason we added the flag
> recording for ChromeOS in histograms in the first place.  Re-assigning to
Alexei
> to decide because I'm sure he knows about this.
> 
> --mark

@Alexei Can you help us with that please?

[afakhry, 2015-01-27 01:29:19]

On 2015/01/26 17:30:44, rdsmith wrote:
> Were you going to send me screen shots of what the ChromeOS page used to look
> like and what it looks like now?  Or did that get resolved in some other
> fashion?

@rdsmith: I just send you the screeshots on your email. Sorry for the delay!

[Alexei Svitkine (slow), 2015-01-27 16:15:44]

I think the test we have for this checks that all about_flags.cc flags are in
histograms.xml. So if it's not in about_flags.cc, it's probably fine to omit
from the XML file.

(I think the intention was to see which flags users are manually enabling -
which on CrOS can only be done through about:flags on end-user (non-dev mode)
devices.)

[afakhry, 2015-01-27 17:44:14]

https://codereview.chromium.org/856643002/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (left):

https://codereview.chromium.org/856643002/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2152: 
On 2015/01/24 04:55:47, oshima wrote:
> nit: revert this change?

Done.

https://codereview.chromium.org/856643002/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/856643002/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50285: +  <int value="-2107097252"
label="disable-new-offline-error-page"/>
On 2015/01/24 04:55:47, oshima wrote:
> do you need this change? (I'll leave this to the owner though)

As per Alexei, removed.

[Randy Smith (Not in Mondays), 2015-01-27 19:05:28]

rdsmith@chromium.org changed reviewers:
+ mmenke@chromium.org

[Randy Smith (Not in Mondays), 2015-01-27 19:05:29]

This is getting close enough for me to want to pull in mmenke@ as well for a
quick OWNERS skim over the change to the neterror.js code.  I'm getting close to
being ok with the change; I'm only concerned about the issues below.

https://codereview.chromium.org/856643002/diff/120001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/856643002/diff/120001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:367: //
"enable-new-offline-error-page" flag is enabled.
This comment seems out of date?

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1380: // command-line switch is not available.
I'm confused by this.  Maybe I just haven't been following the discussion on the
CL closely enough, but it seems to me if you're worried about unexpected
consequences leading to your wanting to fall back to the old behavior, you'd
want to hook that into a finch trial, so it could be turned off without action
on the users' part and without a new release of ChromeOS.  What am I missing?

https://codereview.chromium.org/856643002/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/neterror.js:134: <if expr="not chromeos">
Just confirming: This does result in the details button always being shown on
ChromeOS?  I ask because it doesn't look like the same format as the <if> tag
above, which had phrases like "is_macosx".  I would have expected "not
is_chromeos" or "not_chromeos" if the tags were consistent.

[Alexei Svitkine (slow), 2015-01-27 22:08:09]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[oshima, 2015-01-27 22:41:32]

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1380: // command-line switch is not available.
On 2015/01/27 19:05:29, rdsmith wrote:
> I'm confused by this.  Maybe I just haven't been following the discussion on
the
> CL closely enough, but it seems to me if you're worried about unexpected
> consequences leading to your wanting to fall back to the old behavior, you'd
> want to hook that into a finch trial, so it could be turned off without action
> on the users' part and without a new release of ChromeOS.  What am I missing?

Finch is for A/B testing, which is used to make decision which UI is better. We
definitely want to switch to the common page. This is to make it easy to turn
off and/or troubleshoot if we find a problem with it during beta testing.

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1383: ::switches::kDisableNewOfflineErrorPage))
You can just return

return !.. HasSwitch()

[Randy Smith (Not in Mondays), 2015-01-27 23:52:59]

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1380: // command-line switch is not available.
On 2015/01/27 22:41:31, oshima wrote:
> On 2015/01/27 19:05:29, rdsmith wrote:
> > I'm confused by this.  Maybe I just haven't been following the discussion on
> the
> > CL closely enough, but it seems to me if you're worried about unexpected
> > consequences leading to your wanting to fall back to the old behavior, you'd
> > want to hook that into a finch trial, so it could be turned off without
action
> > on the users' part and without a new release of ChromeOS.  What am I
missing?
> 
> Finch is for A/B testing, which is used to make decision which UI is better.
We
> definitely want to switch to the common page. This is to make it easy to turn
> off and/or troubleshoot if we find a problem with it during beta testing.

That's not the only thing finch is used for; it's also used as a centrally
controllable way to turn off features if they start misbehaving, or to launch
features gradually (1%->10%->100%).  I've definitely used it the second way, and
I've relied on being able to use it the first way.  Having said that, if you're
comfortable the risk of problems once it reaches the field is low, skipping it
is good by me--it's a bit of a pain to setup.

[afakhry, 2015-01-28 00:08:05]

https://codereview.chromium.org/856643002/diff/120001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/856643002/diff/120001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:367: //
"enable-new-offline-error-page" flag is enabled.
On 2015/01/27 19:05:29, rdsmith wrote:
> This comment seems out of date?

Indeed it was.

https://codereview.chromium.org/856643002/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/856643002/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/neterror.js:134: <if expr="not chromeos">
On 2015/01/27 19:05:29, rdsmith wrote:
> Just confirming: This does result in the details button always being shown on
> ChromeOS?  I ask because it doesn't look like the same format as the <if> tag
> above, which had phrases like "is_macosx".  I would have expected "not
> is_chromeos" or "not_chromeos" if the tags were consistent.

Yup, that's the flag for ChromeOS. I have no idea why it doesn't match the
others.

[oshima, 2015-01-28 00:13:08]

On 2015/01/27 23:52:59, rdsmith wrote:
>
https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
> File chrome/common/chrome_switches.cc (right):
> 
>
https://codereview.chromium.org/856643002/diff/120001/chrome/common/chrome_sw...
> chrome/common/chrome_switches.cc:1380: // command-line switch is not
available.
> On 2015/01/27 22:41:31, oshima wrote:
> > On 2015/01/27 19:05:29, rdsmith wrote:
> > > I'm confused by this.  Maybe I just haven't been following the discussion
on
> > the
> > > CL closely enough, but it seems to me if you're worried about unexpected
> > > consequences leading to your wanting to fall back to the old behavior,
you'd
> > > want to hook that into a finch trial, so it could be turned off without
> action
> > > on the users' part and without a new release of ChromeOS.  What am I
> missing?
> > 
> > Finch is for A/B testing, which is used to make decision which UI is better.
> We
> > definitely want to switch to the common page. This is to make it easy to
turn
> > off and/or troubleshoot if we find a problem with it during beta testing.
> 
> That's not the only thing finch is used for; it's also used as a centrally
> controllable way to turn off features if they start misbehaving, or to launch
> features gradually (1%->10%->100%).
>  I've definitely used it the second way, and
> I've relied on being able to use it the first way.  Having said that, if
you're
> comfortable the risk of problems once it reaches the field is low, skipping it
> is good by me--it's a bit of a pain to setup.

I understand such use case, but it's overkill for the work to consolidate the
code.
(It's not really new feature). We're definitely not interesting in graceful
roll.

[afakhry, 2015-01-28 04:20:53]

@edwardjung, @rdsmith, and @mmenke: Could you please take a look at the last
patch and say if it looks good to you, or if you need some modifications?
Thanks!

[edwardjung, 2015-01-28 12:18:21]

On 2015/01/28 04:20:53, afakhry wrote:
> @edwardjung, @rdsmith, and @mmenke: Could you please take a look at the last
> patch and say if it looks good to you, or if you need some modifications?
> Thanks!

LGTM for JS side of things.

[Randy Smith (Not in Mondays), 2015-01-28 18:47:40]

lgtm

[afakhry, 2015-01-29 02:14:51]

afakhry@chromium.org changed reviewers:
+ arv@chromium.org
- mmenke@chromium.org

[afakhry, 2015-01-29 02:14:51]

@arv: Could you please give us an OWNERS review?

[arv (Not doing code reviews), 2015-01-29 21:32:45]

LGTM

[afakhry, 2015-01-29 21:34:08]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-01-29 21:35:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856643002/140001

[commit-bot: I haz the power, 2015-01-29 21:42:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-29 21:42:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[afakhry, 2015-01-29 22:07:51]

afakhry@chromium.org changed reviewers:
+ sky@chromium.org

[afakhry, 2015-01-29 22:07:51]

@sky, Could you please give us OWNERS acceptance?

[sky, 2015-01-29 23:45:56]

LGTM

https://codereview.chromium.org/856643002/diff/140001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/856643002/diff/140001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:402: bool NewOfflineErrorPageEnabled();
nit: keep sort order.

[afakhry, 2015-01-30 00:24:39]

https://codereview.chromium.org/856643002/diff/140001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/856643002/diff/140001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:402: bool NewOfflineErrorPageEnabled();
On 2015/01/29 23:45:56, sky wrote:
> nit: keep sort order.

Done.

[afakhry, 2015-01-30 00:24:50]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-01-30 00:26:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/856643002/160001

[commit-bot: I haz the power, 2015-01-30 01:22:19]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-01-30 01:24:06]

Patchset 8 (id:??) landed as
https://crrev.com/b4fd13cb9dca2d60333e6e9211dd5b88b0ca9739
Cr-Commit-Position: refs/heads/master@{#313851}