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

[Offline pages] Add new Error page failure status inside the MHTML archiver and update corresponding histogram.

This should record number of error pages we are seeing being recorded by Prerender and also eliminate this altogether.

BUG=709639, 692688
Review-Url: https://codereview.chromium.org/2810913002
Cr-Commit-Position: refs/heads/master@{#464087}
Committed: https://chromium.googlesource.com/chromium/src/+/2d54317a97881484713af4069f01677e924dc87e

[chili, 2017-04-10 20:24:38]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 20:25:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2017-04-10 20:25:41]

chili@chromium.org changed reviewers:
+ fgorski@chromium.org, petewil@chromium.org

[chili, 2017-04-10 20:25:41]

Double checking on tests right now. Can you take a look to see if this is a good
intermediate direction to go?

[chili, 2017-04-10 20:27:19]

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:69:
content::PageType::PAGE_TYPE_NORMAL) {
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.

[Pete Williamson, 2017-04-10 20:54:16]

A question on the approach - Should this code live in the offliner instead of
the archiver?  I would expect that we can know from within the offliner if we
have an error page before we call the archiver (I would expect the HTTP return
code to be available somewhere).

It seems a bit out of place to me in the archiver, but this might be OK if we
can't do it from the offliner, or it is a lot more complex to do from the
offliner.

[romax, 2017-04-10 21:19:06]

On 2017/04/10 20:54:16, Pete Williamson wrote:
> A question on the approach - Should this code live in the offliner instead of
> the archiver?  I would expect that we can know from within the offliner if we
> have an error page before we call the archiver (I would expect the HTTP return
> code to be available somewhere).
> 
> It seems a bit out of place to me in the archiver, but this might be OK if we
> can't do it from the offliner, or it is a lot more complex to do from the
> offliner.

I also think it's better in Offliner scope (if it can be there). Reasons:
1. It can provide more consistent/be able to be compared with other offliner
results.
2. I prefer not to call MHTMLarchiver since we already have an error page and we
know we're not going to save it.

But it's also fine if you'd like to keep it here for other reasons.

[fgorski, 2017-04-10 21:19:17]

Not saving error pages in general makes sense, regardless if we are going
through offliner or not (and which offliner is used).

Will receiving this failure result in attempt retry?

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_archiver.h (right):

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_archiver.h:53: ERROR_PAGE,           
         // We detected an error page.
ERROR_WRONG_PAGE_TYPE
(you mention that both error pages and interstitial pages qualify)

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_model_impl.cc:76: result =
SavePageResult::ERROR_PAGE_DETECTED;
fix this somehow as well, please.

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

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:69:
content::PageType::PAGE_TYPE_NORMAL) {
On 2017/04/10 20:27:18, chili wrote:
> 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.

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

[commit-bot: I haz the power, 2017-04-10 21:55:44]

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

[commit-bot: I haz the power, 2017-04-10 21:55:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pete Williamson, 2017-04-11 17:08:32]

On 2017/04/10 20:54:16, Pete Williamson wrote:
> A question on the approach - Should this code live in the offliner instead of
> the archiver?  I would expect that we can know from within the offliner if we
> have an error page before we call the archiver (I would expect the HTTP return
> code to be available somewhere).
> 
> It seems a bit out of place to me in the archiver, but this might be OK if we
> can't do it from the offliner, or it is a lot more complex to do from the
> offliner.

After we spoke offline, it was brought up that changing this in the archiver
would
allow us to avoid changing the pre-renderer.  Chili has filed a bug to remove
this from the archiver after we transition from the pre-renderer to the
background loader offliner.

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

lgtm with nit

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:69:
content::PageType::PAGE_TYPE_NORMAL) {
On 2017/04/10 20:27:18, chili wrote:
> 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.

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:34:25]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:35:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc
(right):

https://codereview.chromium.org/2810913002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc:69:
content::PageType::PAGE_TYPE_NORMAL) {
On 2017/04/10 21:19:34, romax wrote:
> On 2017/04/10 20:27:18, chili wrote:
> > 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.
> 
> would it be possible that the INTERSTITIAL page is a 'maybe' page rather than
an
> error page?

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.

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_archiver.h (right):

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_archiver.h:53: ERROR_PAGE,           
         // We detected an error page.
On 2017/04/10 21:19:17, fgorski wrote:
> ERROR_WRONG_PAGE_TYPE
> (you mention that both error pages and interstitial pages qualify)

Done.

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
File components/offline_pages/core/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2810913002/diff/1/components/offline_pages/co...
components/offline_pages/core/offline_page_model_impl.cc:76: result =
SavePageResult::ERROR_PAGE_DETECTED;
On 2017/04/10 21:19:17, fgorski wrote:
> fix this somehow as well, please.

Done.

[chili, 2017-04-11 20:40:26]

The CQ bit was checked by chili@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 20:41:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 21:45:34]

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

[commit-bot: I haz the power, 2017-04-11 21:45:34]

Dry run: This issue passed the CQ dry run.

[chili, 2017-04-11 22:07:32]

chili@chromium.org changed reviewers:
+ holte@chromium.org

[chili, 2017-04-11 22:07:32]

+holte
Sorry! Added you to the wrong CL >_<

[Steven Holte, 2017-04-11 23:21:26]

histograms lgtm

[fgorski, 2017-04-12 16:05:57]

lgtm

[chili, 2017-04-12 17:08:27]

The CQ bit was checked by chili@chromium.org

[chili, 2017-04-12 17:08:28]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2810913002/#ps40001
(title: "add comments and fix tests")

[commit-bot: I haz the power, 2017-04-12 17:08:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 18:10:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492016907505630,
"parent_rev": "5f418b53fdc5b34cf7a1b8b2fa7dc99ff5d7624d", "commit_rev":
"2d54317a97881484713af4069f01677e924dc87e"}

[commit-bot: I haz the power, 2017-04-12 18:11:58]

Description was changed from

==========
[Offline pages] Add new Error page failure status inside the MHTML archiver and
update corresponding histogram.

This should record number of error pages we are seeing being recorded by
Prerender and also eliminate this altogether.

BUG=709639,692688
==========

to

==========
[Offline pages] Add new Error page failure status inside the MHTML archiver and
update corresponding histogram.

This should record number of error pages we are seeing being recorded by
Prerender and also eliminate this altogether.

BUG=709639,692688

Review-Url: https://codereview.chromium.org/2810913002
Cr-Commit-Position: refs/heads/master@{#464087}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d54317a97881484713af4069f01...
==========

[commit-bot: I haz the power, 2017-04-12 18:12:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2d54317a97881484713af4069f01...