[Offline pages] Add new Error page failure status inside the MHTML archiver and update correspondin…

chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc

 // Copyright 2015 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.
 
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/guid.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "components/security_state/core/security_state.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/mhtml_generation_params.h"
+#include "content/public/common/page_type.h"
 #include "net/base/filename_util.h"
 
 namespace offline_pages {
 namespace {
 const base::FilePath::CharType kMHTMLExtension[] = FILE_PATH_LITERAL("mhtml");
 
 void DeleteFileOnFileThread(const base::FilePath& file_path,
                             const base::Closure& callback) {
   content::BrowserThread::PostTaskAndReply(
       content::BrowserThread::FILE, FROM_HERE,
@@ skipping 25 matching lines @@
     const CreateArchiveCallback& callback) {
   DCHECK(callback_.is_null());
   DCHECK(!callback.is_null());
   callback_ = callback;
 
   if (HasConnectionSecurityError()) {
     ReportFailure(ArchiverResult::ERROR_SECURITY_CERTIFICATE);
     return;
   }
 
+  if (web_contents_->GetController().GetVisibleEntry()->GetPageType() !=
+      content::PageType::PAGE_TYPE_NORMAL) {

[chili, 2017/04/10 20:27:18]

The error page types are: NORMAL, ERROR, INTERSTITIAL, so this will prevent
saving of interstitial pages as well. Alternatively we can fine-grain this more
with different enums or only keep track of error pages.

[romax, 2017/04/10 21:19:34]

would it be possible that the INTERSTITIAL page is a 'maybe' page rather than an
error page?

[Pete Williamson, 2017/04/11 17:08:50]

Good comment, please add it to the code instead of just the code review.  

By the way, what exactly is an INTERSTITIAL page?  Is it a redirect page, or
something different?

[chili, 2017/04/11 20:40:16]

As the code is currently implemented, that's not very possible. Interstitial
page here means that we detected something potentially bad, and injected an
additional information page in the middle. It's probably not good to download
the page in that instance.

We will track it separately though, to keep an eye on the numbers and confirm
numbers somewhat make sense.

+    ReportFailure(ArchiverResult::ERROR_PAGE);
+    return;
+  }
+
   GenerateMHTML(archives_dir, create_archive_params);
 }
 
 void OfflinePageMHTMLArchiver::GenerateMHTML(
     const base::FilePath& archives_dir,
     const CreateArchiveParams& create_archive_params) {
   if (archives_dir.empty()) {
     DVLOG(1) << "Archive path was empty. Can't create archive.";
     ReportFailure(ArchiverResult::ERROR_ARCHIVE_CREATION_FAILED);
     return;
@@ skipping 55 matching lines @@
 }
 
 void OfflinePageMHTMLArchiver::ReportFailure(ArchiverResult result) {
   DCHECK(result != ArchiverResult::SUCCESSFULLY_CREATED);
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(callback_, this, result, GURL(), base::FilePath(),
                             base::string16(), 0));
 }
 
 }  // namespace offline_pages