Viewing cached webpages while offline is not available on Chrome OS

chrome/renderer/resources/neterror.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 function toggleHelpBox() {
   var helpBoxOuter = document.getElementById('help-box-outer');
   helpBoxOuter.classList.toggle('hidden');
   var detailsButton = document.getElementById('details-button');
   if (helpBoxOuter.classList.contains('hidden'))
     detailsButton.innerText = detailsButton.detailsText;
@@ skipping 113 matching lines @@
   } else {
     buttons.classList.add('suggested-right');
     controlButtonDiv.insertBefore(secondaryButton, primaryButton);
   }
 
   if (reloadButton.style.display == 'none' &&
       staleLoadButton.style.display == 'none') {
     detailsButton.classList.add('singular');
   }
 
+<if expr="not chromeos">
   // Hide the details button if there are no details to show.
   if (loadTimeData.valueExists('summary') &&
           !loadTimeData.getValue('summary').msg) {
     detailsButton.style.display = 'none';
     document.getElementById('help-box-outer').style.display = 'block';
   }
+</if>

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

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]

@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]

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]

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

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

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]

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.

 
   // Show control buttons.
   if (loadTimeData.valueExists('reloadButton') &&
           loadTimeData.getValue('reloadButton').msg ||
       loadTimeData.valueExists('staleLoadButton') &&
           loadTimeData.getValue('staleLoadButton').msg) {
     controlButtonDiv.hidden = false;
   }
 
   // Add a main message paragraph.
   if (loadTimeData.valueExists('primaryParagraph')) {
     var p = document.querySelector('#main-message p');
     p.innerHTML = loadTimeData.getString('primaryParagraph');
     p.hidden = false;
   }
 }
 
 document.addEventListener('DOMContentLoaded', onDocumentLoad);