Add "SHOW ALL SAVED PAGES" button to offline error page

components/error_page/renderer/net_error_helper_core.cc

Index: components/error_page/renderer/net_error_helper_core.cc
diff --git a/components/error_page/renderer/net_error_helper_core.cc b/components/error_page/renderer/net_error_helper_core.cc
index e193073e1dec42a1b5d94b7358610539722663da..f93f66dc9338c2e551f2212b80894c714e850534 100644
--- a/components/error_page/renderer/net_error_helper_core.cc
+++ b/components/error_page/renderer/net_error_helper_core.cc
@@ -377,6 +377,7 @@ struct NetErrorHelperCore::ErrorPageInfo {
         reload_button_in_page(false),
         show_saved_copy_button_in_page(false),
         show_cached_copy_button_in_page(false),
+        show_saved_pages_button_in_page(false),
         is_finished_loading(false),
         auto_reload_triggered(false) {
   }
@@ -411,6 +412,7 @@ struct NetErrorHelperCore::ErrorPageInfo {
   bool reload_button_in_page;
   bool show_saved_copy_button_in_page;
   bool show_cached_copy_button_in_page;
+  bool show_saved_pages_button_in_page;
 
   // True if a page has completed loading, at which point it can receive
   // updates.
@@ -465,7 +467,8 @@ NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate,
       online_(true),
       visible_(is_visible),
       auto_reload_count_(0),
-      navigation_from_button_(NO_BUTTON) {
+      navigation_from_button_(NO_BUTTON),
+      has_offline_pages_(false) {
 }
 
 NetErrorHelperCore::~NetErrorHelperCore() {
@@ -590,7 +593,11 @@ void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
   if (committed_error_page_info_->show_saved_copy_button_in_page) {
     RecordEvent(NETWORK_ERROR_PAGE_SHOW_SAVED_COPY_BUTTON_SHOWN);
   }
-  if (committed_error_page_info_->reload_button_in_page &&
+  if (committed_error_page_info_->show_saved_pages_button_in_page) {
+    RecordEvent(NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_SHOWN);
+  }
+  if ((committed_error_page_info_->reload_button_in_page ||
+       committed_error_page_info_->show_saved_pages_button_in_page) &&
       committed_error_page_info_->show_saved_copy_button_in_page) {
     RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN);

[mmenke, 2015/11/05 15:55:50]

This changes the meaning of the old histograms significantly, and is confusing. 
What exactly is the intention here?

[jianli, 2015/11/06 00:04:40]

I am not changing the meaning of the old histograms, but indeed we're adding
some new events to this to accommodate the new cases we have now. Now we're
showing "Show saved pages" button. So I need to track
NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_SHOWN. Also since "Show saved pages"
button could also appear together with "Show saved copies" button, I want to
track NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN when this is the case.

[mmenke, 2015/11/06 17:43:02]

You are absolutely changing the meaning of
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN.

It used to be that with
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_COPY_CLICKED,
NETWORK_ERROR_PAGE_BOTH_BUTTONS_RELOAD_CLICKED, and
NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN, you could tell how often the reload vs
the show save pages button were pressed, when they were both shown.

With your modification, it's impossible to get this value.  In fact, I can't see
how any of the "BUTTONS_SHOWN" numbers could be reasonably interpreted after
this lands.

If show saved pages and show saved copy had the same number of clicks when
"both" buttons were shown, what would that mean?  It could be one was shown 5
times more often than the other, and more rarely clicked, for instance.

[jianli, 2015/11/09 19:19:09]

Thanks for explanation. I reverted to the previous code. Since it is not very
likely to have both saved pages button and saved copy button turned on at the
same time, I think recording just saved pages action should be enough.

   }
@@ -643,13 +650,15 @@ void NetErrorHelperCore::GetErrorHTML(
     bool reload_button_in_page;
     bool show_saved_copy_button_in_page;
     bool show_cached_copy_button_in_page;
+    bool show_saved_pages_button_in_page;
 
     delegate_->GenerateLocalizedErrorPage(
         error, is_failed_post,
         false /* No diagnostics dialogs allowed for subframes. */,
+        false /* No "show saved pages" provided in subframes */,
         scoped_ptr<ErrorPageParams>(), &reload_button_in_page,
         &show_saved_copy_button_in_page, &show_cached_copy_button_in_page,
-        error_html);
+        &show_saved_pages_button_in_page, error_html);
   }
 }
 
@@ -685,6 +694,10 @@ void NetErrorHelperCore::OnSetNavigationCorrectionInfo(
   navigation_correction_params_.search_url = search_url;
 }
 
+void NetErrorHelperCore::OnSetHasOfflinePages(bool has_offline_pages) {
+  has_offline_pages_ = has_offline_pages;
+}
+
 void NetErrorHelperCore::GetErrorHtmlForMainFrame(
     ErrorPageInfo* pending_error_page_info,
     std::string* error_html) {
@@ -712,10 +725,12 @@ void NetErrorHelperCore::GetErrorHtmlForMainFrame(
   delegate_->GenerateLocalizedErrorPage(
       error, pending_error_page_info->was_failed_post,
       can_show_network_diagnostics_dialog_,
+      has_offline_pages_,
       scoped_ptr<ErrorPageParams>(),
       &pending_error_page_info->reload_button_in_page,
       &pending_error_page_info->show_saved_copy_button_in_page,
       &pending_error_page_info->show_cached_copy_button_in_page,
+      &pending_error_page_info->show_saved_pages_button_in_page,
       error_html);
 }
 
@@ -739,7 +754,8 @@ void NetErrorHelperCore::UpdateErrorPage() {
   delegate_->UpdateErrorPage(
       GetUpdatedError(committed_error_page_info_->error),
       committed_error_page_info_->was_failed_post,
-      can_show_network_diagnostics_dialog_);
+      can_show_network_diagnostics_dialog_,
+      has_offline_pages_);
 }
 
 void NetErrorHelperCore::OnNavigationCorrectionsFetched(
@@ -776,10 +792,12 @@ void NetErrorHelperCore::OnNavigationCorrectionsFetched(
         pending_error_page_info_->error,
         pending_error_page_info_->was_failed_post,
         can_show_network_diagnostics_dialog_,
+        has_offline_pages_,
         params.Pass(),
         &pending_error_page_info_->reload_button_in_page,
         &pending_error_page_info_->show_saved_copy_button_in_page,
         &pending_error_page_info_->show_cached_copy_button_in_page,
+        &pending_error_page_info_->show_saved_pages_button_in_page,
         &error_html);
   } else {
     // Since |navigation_correction_params| in |pending_error_page_info_| is
@@ -926,6 +944,9 @@ void NetErrorHelperCore::ExecuteButtonPress(Button button) {
       if (committed_error_page_info_->reload_button_in_page) {
         RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_COPY_CLICKED);
       }
+      if (committed_error_page_info_->show_saved_pages_button_in_page) {
+        RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_PAGES_CLICKED);

[mmenke, 2015/11/05 15:55:50]

This seems wrong.

[jianli, 2015/11/06 00:04:40]

Fixed.

+      }
       delegate_->LoadPageFromCache(
           committed_error_page_info_->error.unreachableURL);
       return;
@@ -944,6 +965,14 @@ void NetErrorHelperCore::ExecuteButtonPress(Button button) {
       delegate_->DiagnoseError(
           committed_error_page_info_->error.unreachableURL);
       return;
+    case SHOW_SAVED_PAGES_BUTTON:
+      RecordEvent(NETWORK_ERROR_PAGE_SHOW_SAVED_PAGES_BUTTON_CLICKED);
+      navigation_from_button_ = SHOW_SAVED_PAGES_BUTTON;
+      if (committed_error_page_info_->show_saved_copy_button_in_page) {
+        RecordEvent(NETWORK_ERROR_PAGE_BOTH_BUTTONS_SHOWN_SAVED_PAGES_CLICKED);
+      }
+      delegate_->ShowOfflinePages();
+      return;
     case NO_BUTTON:
       NOTREACHED();
       return;