Track parse time blocked on script in xml documents.

Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
Committed: https://crrev.com/4066a12bab519a1bb7afd04e0a1499c549bd491b
Cr-Commit-Position: refs/heads/master@{#411745}

[Bryan McQuade, 2016-08-10 16:49:48]

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

[commit-bot: I haz the power, 2016-08-10 16:50:34]

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, 2016-08-10 19:05:57]

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

[commit-bot: I haz the power, 2016-08-10 19:05:58]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-08-11 15:46:05]

Description was changed from

==========
Track parse time blocked on script in xml documents.

BUG=632343
==========

to

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

[Bryan McQuade, 2016-08-11 15:47:11]

bmcquade@chromium.org changed reviewers:
+ kouhei@chromium.org

[Bryan McQuade, 2016-08-11 15:47:12]

PTAL

[kouhei (in TOK), 2016-08-12 01:13:29]

https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:483:
DocumentParserTiming::from(*document()).recordParserBlockedOnScriptLoadDuration(monotonicallyIncreasingTime()
- scriptParserBlockingTime, scriptLoader->wasCreatedDuringDocumentWrite());
Can we record this to separate UMA?
XMLDocumentParser script loader code is quite different from HTMLDocumentParser.

[Bryan McQuade, 2016-08-12 14:01:40]

On 2016/08/12 at 01:13:29, kouhei wrote:
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:483:
DocumentParserTiming::from(*document()).recordParserBlockedOnScriptLoadDuration(monotonicallyIncreasingTime()
- scriptParserBlockingTime, scriptLoader->wasCreatedDuringDocumentWrite());
> Can we record this to separate UMA?
> XMLDocumentParser script loader code is quite different from
HTMLDocumentParser.

I agree that the behavior for defer and async is a bit different between the 2
parsers, but in the common case, the behavior should be pretty comparable. Are
there other differences besides defer and async?

Do you anticipate getting specific value out of keeping them separate? If we'll
definitely learn something interesting from separating them out, I can do that,
but otherwise, I'm concerned that we'll be recording too many fine grained data
points in the PageLoadTiming struct.

Do you have a sense for the timeline for moving XHTML documents to the
HTMLDocumentParser? If that's happening soonish (~a few months) I think we could
just forego this change. Otherwise I do think it's important that we track this
time for all documents.

[kouhei (in TOK), 2016-08-12 14:19:00]

Description was changed from

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

to

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

[kouhei (in TOK), 2016-08-12 14:19:00]

kouhei@chromium.org changed reviewers:
+ dominicc@chromium.org

[Bryan McQuade, 2016-08-12 14:23:51]

On 2016/08/12 at 14:01:40, Bryan McQuade wrote:
> On 2016/08/12 at 01:13:29, kouhei wrote:
> >
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp
(right):
> > 
> >
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:483:
DocumentParserTiming::from(*document()).recordParserBlockedOnScriptLoadDuration(monotonicallyIncreasingTime()
- scriptParserBlockingTime, scriptLoader->wasCreatedDuringDocumentWrite());
> > Can we record this to separate UMA?
> > XMLDocumentParser script loader code is quite different from
HTMLDocumentParser.
> 
> I agree that the behavior for defer and async is a bit different between the 2
parsers, but in the common case, the behavior should be pretty comparable. Are
there other differences besides defer and async?
> 
> Do you anticipate getting specific value out of keeping them separate? If
we'll definitely learn something interesting from separating them out, I can do
that, but otherwise, I'm concerned that we'll be recording too many fine grained
data points in the PageLoadTiming struct.
> 
> Do you have a sense for the timeline for moving XHTML documents to the
HTMLDocumentParser? If that's happening soonish (~a few months) I think we could
just forego this change. Otherwise I do think it's important that we track this
time for all documents.

Thinking about this a bit more, since other parse metrics such as parse start
and parse stop are already tracked for both html and xml documents, I'd like to
keep the other parser metrics like the one in this change consistent.

Separately, we can segment page loads by html vs xhtml using
WebLoadingBehaviorFlag, which would allow us to track html vs xhtml stats
separately for all types of metrics, should we need to do that.

So I'd like to proceed with this change as is and we can consider adding support
to break out by xhtml in a future change, via WebLoadingBehaviorFlag.

WDYT?

[kouhei (in TOK), 2016-08-12 14:24:06]

On 2016/08/12 14:01:40, Bryan McQuade wrote:
> On 2016/08/12 at 01:13:29, kouhei wrote:
> >
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp
(right):
> > 
> >
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:483:
>
DocumentParserTiming::from(*document()).recordParserBlockedOnScriptLoadDuration(monotonicallyIncreasingTime()
> - scriptParserBlockingTime, scriptLoader->wasCreatedDuringDocumentWrite());
> > Can we record this to separate UMA?
> > XMLDocumentParser script loader code is quite different from
> HTMLDocumentParser.
> 
> I agree that the behavior for defer and async is a bit different between the 2
> parsers, but in the common case, the behavior should be pretty comparable. Are
> there other differences besides defer and async?
> 
> Do you anticipate getting specific value out of keeping them separate? If
we'll
> definitely learn something interesting from separating them out, I can do
that,
> but otherwise, I'm concerned that we'll be recording too many fine grained
data
> points in the PageLoadTiming struct.
For example, you mentioned in crbug.com/632343 about possibility of doc.write
intervention.
However, XHTML docs don't support doc.write, so if we use the same UMA,
we can't correctly estimate the coverage.
We don't have preloadscanner in XHTML docs, which would also skew the metric.

> Do you have a sense for the timeline for moving XHTML documents to the
> HTMLDocumentParser? If that's happening soonish (~a few months) I think we
could
> just forego this change. Otherwise I do think it's important that we track
this
> time for all documents.
+dominicc?

[kouhei (in TOK), 2016-08-12 14:25:13]

On 2016/08/12 14:23:51, Bryan McQuade wrote:
> On 2016/08/12 at 14:01:40, Bryan McQuade wrote:
> > On 2016/08/12 at 01:13:29, kouhei wrote:
> > >
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp
> (right):
> > > 
> > >
>
https://codereview.chromium.org/2235733004/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:483:
>
DocumentParserTiming::from(*document()).recordParserBlockedOnScriptLoadDuration(monotonicallyIncreasingTime()
> - scriptParserBlockingTime, scriptLoader->wasCreatedDuringDocumentWrite());
> > > Can we record this to separate UMA?
> > > XMLDocumentParser script loader code is quite different from
> HTMLDocumentParser.
> > 
> > I agree that the behavior for defer and async is a bit different between the
2
> parsers, but in the common case, the behavior should be pretty comparable. Are
> there other differences besides defer and async?
> > 
> > Do you anticipate getting specific value out of keeping them separate? If
> we'll definitely learn something interesting from separating them out, I can
do
> that, but otherwise, I'm concerned that we'll be recording too many fine
grained
> data points in the PageLoadTiming struct.
> > 
> > Do you have a sense for the timeline for moving XHTML documents to the
> HTMLDocumentParser? If that's happening soonish (~a few months) I think we
could
> just forego this change. Otherwise I do think it's important that we track
this
> time for all documents.
> 
> Thinking about this a bit more, since other parse metrics such as parse start
> and parse stop are already tracked for both html and xml documents, I'd like
to
> keep the other parser metrics like the one in this change consistent.
> 
> Separately, we can segment page loads by html vs xhtml using
> WebLoadingBehaviorFlag, which would allow us to track html vs xhtml stats
> separately for all types of metrics, should we need to do that.
> 
> So I'd like to proceed with this change as is and we can consider adding
support
> to break out by xhtml in a future change, via WebLoadingBehaviorFlag.
> 
> WDYT?
This sgtm. lgtm

[Bryan McQuade, 2016-08-12 18:14:00]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-08-12 18:14:21]

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, 2016-08-12 19:59:18]

Description was changed from

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

to

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

[commit-bot: I haz the power, 2016-08-12 19:59:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-12 20:00:51]

Description was changed from

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343
==========

to

==========
Track parse time blocked on script in xml documents.

This adds the existing equivalent logic from HTMLDocumentParser to
XMLDocumentParser.

BUG=632343

Committed: https://crrev.com/4066a12bab519a1bb7afd04e0a1499c549bd491b
Cr-Commit-Position: refs/heads/master@{#411745}
==========

[commit-bot: I haz the power, 2016-08-12 20:00:52]

Patchset 1 (id:??) landed as
https://crrev.com/4066a12bab519a1bb7afd04e0a1499c549bd491b
Cr-Commit-Position: refs/heads/master@{#411745}