Adding MHTML support into Page Load Metrics

Adding MHTML support into Page Load Metrics

Offline Previews metrics will not be reported currently because the
MIME-type sniffing only allows text/html or application/xhtml+xml.
MHTML, entires pages saved as one file are mime-type multipart/related
(https://en.wikipedia.org/wiki/MIME#Multipart_subtypes). Offline
previews are treated as HTTP/HTTPS MHTML pages from the perspective of
the chrome page load metrics harness, and treated as HTML documents in
blink. This should mean that offline pages are the only class of MHTML
pages that will be tracked in the harness.

BUG=693776
Review-Url: https://codereview.chromium.org/2699213002
Cr-Commit-Position: refs/heads/master@{#451906}
Committed: https://chromium.googlesource.com/chromium/src/+/07205c9be7b8fa35aa6079b6027f6261761b62c3

[RyanSturm, 2017-02-17 23:17:50]

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

[commit-bot: I haz the power, 2017-02-17 23:19:02]

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

[RyanSturm, 2017-02-17 23:23:48]

ryansturm@chromium.org changed reviewers:
+ bmcquade@chromium.org, csharrison@chromium.org

[RyanSturm, 2017-02-17 23:23:48]

bmcquade, csharrison: PTAL

[Bryan McQuade, 2017-02-18 00:50:03]

Thanks for working on this!

I wonder, should we track mhtml pages for other observers, or only for the
offline previews observer? Not totally sure.

On Fri, Feb 17, 2017 at 6:23 PM <ryansturm@chromium.org> wrote:

> Reviewers: Bryan McQuade, Charlie Harrison
> CL: https://codereview.chromium.org/2699213002/
>
> Message:
> bmcquade, csharrison: PTAL
>
> Description:
> Adding MHTML support into Page Load Metrics
>
> Offline Previews metrics will not be reported currently because the
> MIME-type sniffing only allows text/html or application/xhtml+xml.
> MHTML, entires pages saved as one file are mime-type multipart/related
> (https://en.wikipedia.org/wiki/MIME#Multipart_subtypes). Offline
> previews are treated as HTTP/HTTPS MHTML pages from the perspective of
> the chrome page load metrics harness, and treated as HTML documents in
> blink. This should mean that offline pages are the only class of MHTML
> pages that will be tracked in the harness.
>
> BUG=693776
>
> Affected files (+5, -3 lines):
> M chrome/browser/page_load_metrics/browser_page_track_decider.cc
> M chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
>
>
> Index: chrome/browser/page_load_metrics/browser_page_track_decider.cc
> diff --git
> a/chrome/browser/page_load_metrics/browser_page_track_decider.cc
> b/chrome/browser/page_load_metrics/browser_page_track_decider.cc
> index
>
f78ed42901ecd41e9d8ae6b7da94dbdd5a36e450..6b52b6b0fb0bae2d275e349281c7661619b2e8c7
> 100644
> --- a/chrome/browser/page_load_metrics/browser_page_track_decider.cc
> +++ b/chrome/browser/page_load_metrics/browser_page_track_decider.cc
> @@ -52,7 +52,8 @@ int BrowserPageTrackDecider::GetHttpStatusCode() {
> bool BrowserPageTrackDecider::IsHtmlOrXhtmlPage() {
> DCHECK(HasCommitted());
> const std::string& mime_type = web_contents_->GetContentsMimeType();
> - return mime_type == "text/html" || mime_type == "application/xhtml+xml";
> + return mime_type == "text/html" || mime_type == "application/xhtml+xml"
> ||
> + mime_type == "multipart/related";
> }
>
> } // namespace page_load_metrics
> Index: chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
> diff --git
> a/chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
> b/chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
> index
>
861b13c351a121c6a60d991b23e97ff5e9e1a194..3340b792f14729cb531b129e66fc1220f7a1152a
> 100644
> --- a/chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
> +++ b/chrome/renderer/page_load_metrics/renderer_page_track_decider.cc
> @@ -45,13 +45,14 @@ int RendererPageTrackDecider::GetHttpStatusCode() {
> bool RendererPageTrackDecider::IsHtmlOrXhtmlPage() {
> // Ignore non-HTML documents (e.g. SVG). Note that images are treated by
> // Blink as HTML documents, so to exclude images, we must perform
> - // additional mime type checking below.
> + // additional mime type checking below. MHTML is tracked as HTML in
> blink.
> if (!document_->isHTMLDocument() && !document_->isXHTMLDocument())
> return false;
>
> // Ignore non-HTML mime types (e.g. images).
> blink::WebString mime_type = data_source_->response().mimeType();
> - return mime_type == "text/html" || mime_type == "application/xhtml+xml";
> + return mime_type == "text/html" || mime_type == "application/xhtml+xml"
> ||
> + mime_type == "multipart/related";
> }
>
> } // namespace page_load_metrics
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[RyanSturm, 2017-02-18 00:52:37]

On 2017/02/18 00:50:03, Bryan McQuade wrote:
> Thanks for working on this!
> 
> I wonder, should we track mhtml pages for other observers, or only for the
> offline previews observer? Not totally sure.
> 

I was hoping you would have a strong opinion either way, since I'm also not
sure. I believe, but am not terribly confident, that HTTP/HTTPS and MHTML means
that the page must be an offline page or offline preview. If that were the case
it would make sense to me to include it for all observer types, but for analysis
sake, I would be fine with only tracking MHTML for the previews observer.

[commit-bot: I haz the power, 2017-02-18 01:17:38]

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

[commit-bot: I haz the power, 2017-02-18 01:17:39]

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

[Charlie Harrison, 2017-02-18 04:45:27]

I don't mind one way or the other either *shrug*. If counts for offline pages
were very high I might lean towards tracking it in a separate observer to avoid
muddying up our existing metrics though.

[RyanSturm, 2017-02-18 19:43:31]

I'm not sure which is cleaner, adding a something like SupportsMHTML() method to
the page load metrics observer interface and pruning just before commit is
called, or adding an MHTML check inside each observers commit method that
returns STOP_OBSERVING. It would be an interesting observer change to start
defining default behavior in separate SupportsX() methods for common checks like
SupportsStartedInBackground(), etc. That approach might make adding new
observers easier if they want to follow default behavior, and they just need to
override the non-default.

Either way, it would be good to merge this back to M-57, so I'm trying to keep
the change small and also trying to keep it not too impactful.

[Charlie Harrison, 2017-02-18 20:39:15]

If you want to measure offline pages in M57 shouldn't we have an observer that
only triggers for those pages? Otherwise the metrics aren't going to be very
meaningful.

I think SupportsMHTML() is a good solution.

[RyanSturm, 2017-02-18 20:46:28]

On 2017/02/18 20:39:15, Charlie Harrison wrote:
> If you want to measure offline pages in M57 shouldn't we have an observer that
> only triggers for those pages? Otherwise the metrics aren't going to be very
> meaningful.
> 
> I think SupportsMHTML() is a good solution.


Previews observer only tracks offline page loads via previews. I'm not sure how
I missed the filtering when landing it (or if something else changed that caused
the observer to not track these).

https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...

[Charlie Harrison, 2017-02-18 21:28:22]

Ah okay great, just making sure.

LGTM from me to land this and merge to M57 but I would appreciate a regression
test in a follow up.

[Bryan McQuade, 2017-02-21 13:45:58]

On 2017/02/18 at 21:28:22, csharrison wrote:
> Ah okay great, just making sure.
> 
> LGTM from me to land this and merge to M57 but I would appreciate a regression
test in a follow up.

Ok, given Kenji's feedback, let's find a way to limit logging for these to just
the previews observer. One thing you could do is have each observer have a
policy method indicating whether it wants to process a response of a given
content type, and have the default implementation use only html or xhtml. I
think this might be best served by a separate policy object that each observer
can return - perhaps an implementation of PageTrackDecider? Happy to discuss
more over email.

[RyanSturm, 2017-02-21 17:21:03]

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

[commit-bot: I haz the power, 2017-02-21 17:21:52]

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

[RyanSturm, 2017-02-21 17:24:27]

bmcquade: PTAL

csharrison@ and I briefly talked about this approach. It brings up the question
of whether should change the PageTrackDecider interface to not check MIME-type,
but I think for now, it should only track HTML, XHTML, and MHTML until another
observer wants to do something similar.

[commit-bot: I haz the power, 2017-02-21 18:30:59]

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

[commit-bot: I haz the power, 2017-02-21 18:31:00]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-21 18:50:15]

nice, LGTM, thanks for this! one request to reduce logic duplication slightly by
chaining to base class impl.

https://codereview.chromium.org/2699213002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2699213002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/previews_page_load_metrics_observer.cc:56:
return mime_type == "text/html" || mime_type == "application/xhtml+xml" ||
let's have this chain to the base class impl first, and if true, return true,
then also allow multipart/related:

if (PageLoadMetricsObserver::ShouldObserveMimeType(mime_type) ==
CONTINUE_OBSERVING)
  return CONTINUE_OBSERVING;
return mime_type == "multipart/related" ? CONTINUE_OBSERVING : STOP_OBSERVING;

or some variation of this.

https://codereview.chromium.org/2699213002/diff/20001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2699213002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:450:
INVOKE_AND_PRUNE_OBSERVERS(
ah, nice, this actually works very well. i like this approach.

[Charlie Harrison, 2017-02-21 19:07:10]

still lgtm assuming a follow up test

[RyanSturm, 2017-02-21 19:25:40]

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

[commit-bot: I haz the power, 2017-02-21 19:26:39]

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

[RyanSturm, 2017-02-21 19:26:47]

On 2017/02/21 19:07:10, Charlie Harrison wrote:
> still lgtm assuming a follow up test

Thanks.

Here is the bug for the regression test:
crbug.com/694683

[commit-bot: I haz the power, 2017-02-21 21:29:01]

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

[commit-bot: I haz the power, 2017-02-21 21:29:02]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[RyanSturm, 2017-02-21 23:01:44]

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

[commit-bot: I haz the power, 2017-02-21 23:04:49]

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

[RyanSturm, 2017-02-21 23:24:07]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2017-02-21 23:24:08]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-02-21 23:24:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2699213002/#ps40001
(title: "bmcquade comment")

[commit-bot: I haz the power, 2017-02-21 23:25:34]

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-02-22 01:08:24]

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

[commit-bot: I haz the power, 2017-02-22 01:08:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[RyanSturm, 2017-02-22 01:32:15]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2017-02-22 01:35:17]

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-02-22 07:08:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487727135066250,
"parent_rev": "a6beb2c8e9ec942cf97135996217d32aa37c789f", "commit_rev":
"07205c9be7b8fa35aa6079b6027f6261761b62c3"}

[commit-bot: I haz the power, 2017-02-22 07:09:11]

Description was changed from

==========
Adding MHTML support into Page Load Metrics

Offline Previews metrics will not be reported currently because the
MIME-type sniffing only allows text/html or application/xhtml+xml.
MHTML, entires pages saved as one file are mime-type multipart/related
(https://en.wikipedia.org/wiki/MIME#Multipart_subtypes). Offline
previews are treated as HTTP/HTTPS MHTML pages from the perspective of
the chrome page load metrics harness, and treated as HTML documents in
blink. This should mean that offline pages are the only class of MHTML
pages that will be tracked in the harness.

BUG=693776
==========

to

==========
Adding MHTML support into Page Load Metrics

Offline Previews metrics will not be reported currently because the
MIME-type sniffing only allows text/html or application/xhtml+xml.
MHTML, entires pages saved as one file are mime-type multipart/related
(https://en.wikipedia.org/wiki/MIME#Multipart_subtypes). Offline
previews are treated as HTTP/HTTPS MHTML pages from the perspective of
the chrome page load metrics harness, and treated as HTML documents in
blink. This should mean that offline pages are the only class of MHTML
pages that will be tracked in the harness.

BUG=693776

Review-Url: https://codereview.chromium.org/2699213002
Cr-Commit-Position: refs/heads/master@{#451906}
Committed:
https://chromium.googlesource.com/chromium/src/+/07205c9be7b8fa35aa6079b6027f...
==========

[commit-bot: I haz the power, 2017-02-22 07:09:12]

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