Fix main frame detection in loading metrics.

Fix main frame detection in loading metrics.

The existing way to detect the main frame produces false positives
as described in https://crbug.com/692112#c10.

This patch uses a recently added trace event that marks the
main frame (category='loading', title='markAsMainFrame') to
make main frame detection accurate.

All detection logic is encapsulated in MainFrameHelper class.

BUG=chromium:692112
Review-Url: https://codereview.chromium.org/2862103002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/9e7bc18ce70595be32d8534b4309aff83ac78434

[ulan, 2017-05-05 12:54:50]

Description was changed from

==========
Detect main frame

BUG=catapult:#
==========

to

==========
Detect main frame
==========

[ulan, 2017-05-05 14:52:48]

Description was changed from

==========
Detect main frame
==========

to

==========
Fix main frame detection in loading metrics.

The existing way to detect the main frame produces false positives
as described in https://crbug.com/692112#c10.

This patch uses a recently added trace event that marks the
main frame (category='loading', title='markAsMainFrame') to
make main frame detection accurate.

All detection logic is encapsulated in MainFrameHelper class.

BUG=chromium:692112
==========

[ulan, 2017-05-05 14:54:54]

ulan@chromium.org changed reviewers:
+ dproy@chromium.org, lingyun.cai@intel.com, nednguyen@google.com,
tdresser@chromium.org

[ulan, 2017-05-05 14:54:55]

PTAL. The main change is in loading_metric.html and loading_metric_test.html.

All other changes are test adjustments.

[tdresser, 2017-05-08 18:12:46]

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:48: class
MainFrameHelper {
I haven't found any examples of using es6 classes in this repro (other than
estimated input latency which you wrote).

I'm not certain of the style here - let's get another opinion on how this should
work. (In particular, I'm not sure about trailing _'s for private methods.

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric_test.html (right):

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:821:
test('MainFrameHelper_isMainFrame_outsideLiveRange', function() {
I haven't found the difference between this and the test above in about 60
seconds of trying. Maybe add a comment highlighting the difference?

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:922:
test('MainFrameHelper_getURL_undefined', function() {
In this example as well, can you highlight what's different from what's above?
(This one didn't take me as long, but it's still a bit subtle)

[ulan, 2017-05-10 10:44:20]

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:48: class
MainFrameHelper {
On 2017/05/08 18:12:46, tdresser wrote:
> I haven't found any examples of using es6 classes in this repro (other than
> estimated input latency which you wrote).
> 
> I'm not certain of the style here - let's get another opinion on how this
should
> work. (In particular, I'm not sure about trailing _'s for private methods.

The style guide says that the ES6 classes are approved in the language feature
table:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

I am also not sure about private field naming. I'll wait for Ned's comments.

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric_test.html (right):

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:821:
test('MainFrameHelper_isMainFrame_outsideLiveRange', function() {
On 2017/05/08 18:12:46, tdresser wrote:
> I haven't found the difference between this and the test above in about 60
> seconds of trying. Maybe add a comment highlighting the difference?

I added comments explaining the scenario for each test.

https://codereview.chromium.org/2862103002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:922:
test('MainFrameHelper_getURL_undefined', function() {
On 2017/05/08 18:12:46, tdresser wrote:
> In this example as well, can you highlight what's different from what's above?
> (This one didn't take me as long, but it's still a bit subtle)

Done.

[nednguyen, 2017-05-10 10:47:09]

nednguyen@google.com changed reviewers:
+ benjhayden@chromium.org

[nednguyen, 2017-05-10 10:47:09]

+Ben is much better reviewer of tracing style than me.

[benjhayden, 2017-05-10 19:03:20]

Thanks, Ned!
A few comments then lgtm

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:44: * The public
interface of this class consists of two methods: isMainFrame and
foo_() is the correct way to indicate this, no comment necessary.

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:54:
rendererHelper.process.objects.instancesByTypeName_.FrameLoader || [];
Please use the public method objects.getAllInstancesNamed('FrameLoader') instead
of the private field.

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:568: for (let sample
of samples) {
const

[ulan, 2017-05-11 10:21:05]

Thanks

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:44: * The public
interface of this class consists of two methods: isMainFrame and
On 2017/05/10 19:03:19, benjhayden wrote:
> foo_() is the correct way to indicate this, no comment necessary.

Done.

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:54:
rendererHelper.process.objects.instancesByTypeName_.FrameLoader || [];
On 2017/05/10 19:03:19, benjhayden wrote:
> Please use the public method objects.getAllInstancesNamed('FrameLoader')
instead
> of the private field.

Done.

https://codereview.chromium.org/2862103002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:568: for (let sample
of samples) {
On 2017/05/10 19:03:19, benjhayden wrote:
> const

Done.

[ulan, 2017-05-11 10:21:23]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-11 10:21:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/2862103002/#ps100001
(title: "Address Ben's comments")

[commit-bot: I haz the power, 2017-05-11 10:21:35]

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-05-11 10:23:44]

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

[commit-bot: I haz the power, 2017-05-11 10:23:45]

Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[ulan, 2017-05-11 13:16:40]

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

[commit-bot: I haz the power, 2017-05-11 13:16:48]

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-05-11 13:40:41]

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

[commit-bot: I haz the power, 2017-05-11 13:40:42]

Dry run: This issue passed the CQ dry run.

[ulan, 2017-05-15 16:22:27]

Tim, the CL is ready to land. Do you have any more comments? :)

[tdresser, 2017-05-15 16:28:21]

Sorry, LGTM.

[ulan, 2017-05-15 16:34:50]

The CQ bit was checked by ulan@chromium.org

[ulan, 2017-05-15 16:34:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
benjhayden@chromium.org
Link to the patchset: https://codereview.chromium.org/2862103002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-15 16:34:57]

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-05-15 16:59:09]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494866090660750,
"parent_rev": "37015fb470d81b7a96a091127590e6e461705059", "commit_rev":
"9e7bc18ce70595be32d8534b4309aff83ac78434"}

[commit-bot: I haz the power, 2017-05-15 16:59:12]

Description was changed from

==========
Fix main frame detection in loading metrics.

The existing way to detect the main frame produces false positives
as described in https://crbug.com/692112#c10.

This patch uses a recently added trace event that marks the
main frame (category='loading', title='markAsMainFrame') to
make main frame detection accurate.

All detection logic is encapsulated in MainFrameHelper class.

BUG=chromium:692112
==========

to

==========
Fix main frame detection in loading metrics.

The existing way to detect the main frame produces false positives
as described in https://crbug.com/692112#c10.

This patch uses a recently added trace event that marks the
main frame (category='loading', title='markAsMainFrame') to
make main frame detection accurate.

All detection logic is encapsulated in MainFrameHelper class.

BUG=chromium:692112

Review-Url: https://codereview.chromium.org/2862103002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-05-15 16:59:13]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[ulan, 2017-05-16 08:36:11]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2883233002/ by ulan@chromium.org.

The reason for reverting is: Test failures on Mac:
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.12%20Tests....