UseCounter: Introduce UseCounter::Observer for layout tests

UseCounter: Introduce UseCounter::Observer for layout tests

This is a follow-up CL for [1]. We added some layout tests in the CL. They
assume MessagePort and UseCounter share the same IPC channel and their messages
are handled in order. However, this assumption will be untrue by MessagePort
mojofication[2] and the layout tests will be flaky.

To fix this, this CL stops assuming the message order and instead introduces a
mechanism to observe UseCounter changes.

[1] https://codereview.chromium.org/2586863002/
[2] https://codereview.chromium.org/2422793002/

BUG=376039
Review-Url: https://codereview.chromium.org/2680423006
Cr-Commit-Position: refs/heads/master@{#450241}
Committed: https://chromium.googlesource.com/chromium/src/+/452a477ff34a2af79a882b44ccc1939230c9e527

[nhiroki, 2017-02-10 11:34:44]

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

[nhiroki, 2017-02-10 11:34:54]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-10 11:34:58]

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

[nhiroki, 2017-02-10 11:35:23]

Description was changed from

==========
UseCounter: Introduce UseCounter::Observer for layout tests

This is a follow-up CL for [1]. We added some layout tests in the CL. They
assume MessagePort and UseCounter shares the same IPC channel and their messages
are handled in order. However, this assumption will be untrue by MessagePort
mojofication[2] and the layout tests will be flaky.

To fix this, this CL stops assuming the message order and instead introduces a
mechanism to observe UseCounter changes.

[1] https://codereview.chromium.org/2586863002/
[2] https://codereview.chromium.org/2422793002/

BUG=376039
==========

to

==========
UseCounter: Introduce UseCounter::Observer for layout tests

This is a follow-up CL for [1]. We added some layout tests in the CL. They
assume MessagePort and UseCounter share the same IPC channel and their messages
are handled in order. However, this assumption will be untrue by MessagePort
mojofication[2] and the layout tests will be flaky.

To fix this, this CL stops assuming the message order and instead introduces a
mechanism to observe UseCounter changes.

[1] https://codereview.chromium.org/2586863002/
[2] https://codereview.chromium.org/2422793002/

BUG=376039
==========

[nhiroki, 2017-02-10 12:02:05]

nhiroki@chromium.org changed reviewers:
+ rbyers@chromium.org

[nhiroki, 2017-02-10 12:03:56]

Hi, can you review this? Thanks!

Context: https://codereview.chromium.org/2658603003/#msg95

[commit-bot: I haz the power, 2017-02-10 13:05:36]

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

[commit-bot: I haz the power, 2017-02-10 13:05:37]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-02-11 15:01:52]

Interesting.  Seems like generally useful infrastructure, thanks!  I guess this
means that common bugs (UseCounter not working) would result in a test timeout
instead of a more easily debuggable assertion failure, but I don't see any
easy/obvious way around that.

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1180:
HeapHashSet<Member<Observer>> toBeRemoved;
IIRC, this code path isn't triggered on the deprecation case.  If that's
correct, why is the test passing?  Is it just relying on the race and could
still be flaky?  I think you want to move this logic down into
recordMeasurement.

In fact, do you really want the semantics to be that the observer is invoked for
EVERY call to 'count'?  That can be a LOT of calls .  Perhaps the semantics
should be that it's invoked when the histogram is updated - i.e. the first time
a feature is used on a page.  That's much rarer and so less performance
sensitive.  So maybe you want to put this logic in a helper function that's
called at the same time as we generate the blink.feature_usage TRACE?

Regardless, this choice of observation semantics should be documented in a
comment in UseCounter.h.

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.h:496: ScriptPromise
observeUseCounter(ScriptState*, Document*, uint32_t feature);
Please add a comment describing the semantics.  In particular, it wasn't
originally obvious to me that the promise would resolve immediately if 'feature'
was already true (but I think that makes sense).

[nhiroki, 2017-02-13 03:25:49]

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

[commit-bot: I haz the power, 2017-02-13 03:26:08]

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-02-13 03:27:43]

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

[commit-bot: I haz the power, 2017-02-13 03:27:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[nhiroki, 2017-02-13 04:05:48]

Patchset #3 (id:60001) has been deleted

[nhiroki, 2017-02-13 04:16:07]

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

[commit-bot: I haz the power, 2017-02-13 04:16:31]

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

[nhiroki, 2017-02-13 04:35:07]

Thank you for your comments. Updated.

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1180:
HeapHashSet<Member<Observer>> toBeRemoved;
On 2017/02/11 15:01:51, Rick Byers wrote:
> IIRC, this code path isn't triggered on the deprecation case.  If that's
> correct, why is the test passing?  Is it just relying on the race and could
> still be flaky?

Good point. This is because both countFeature() and countDeprecation() on
SharedWorkerGlobalScope get to countFeature() on Document.

> I think you want to move this logic down into
> recordMeasurement.
> 
> In fact, do you really want the semantics to be that the observer is invoked
for
> EVERY call to 'count'?  That can be a LOT of calls .  Perhaps the semantics
> should be that it's invoked when the histogram is updated - i.e. the first
time
> a feature is used on a page.  That's much rarer and so less performance
> sensitive.  So maybe you want to put this logic in a helper function that's
> called at the same time as we generate the blink.feature_usage TRACE?

Yes, it's preferable to call it only one time per feature. Moved the call into
recordMeasurement.

> Regardless, this choice of observation semantics should be documented in a
> comment in UseCounter.h.

Added.

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/2680423006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.h:496: ScriptPromise
observeUseCounter(ScriptState*, Document*, uint32_t feature);
On 2017/02/11 15:01:51, Rick Byers wrote:
> Please add a comment describing the semantics.  In particular, it wasn't
> originally obvious to me that the promise would resolve immediately if
'feature'
> was already true (but I think that makes sense).

Done.

[commit-bot: I haz the power, 2017-02-13 05:59:49]

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

[commit-bot: I haz the power, 2017-02-13 05:59:49]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-02-13 20:46:31]

LGTM with (optional) nit

https://codereview.chromium.org/2680423006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2680423006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1201: void
UseCounter::addObserver(Document& document, Observer* observer) {
nit: IMHO this would make more sense as an instance method on UseCounter instead
of a static method.  We have a few static helper methods to factor out some
biolerplate for the really common cases, but this is something that's only going
to be used in one or two places so I wouldn't both with a static personally.

This is minor though - if there's a reason you prefer this to be static then
feel free to leave it.

[nhiroki, 2017-02-14 02:03:29]

Thank you for reviewing this :)

https://codereview.chromium.org/2680423006/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2680423006/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1201: void
UseCounter::addObserver(Document& document, Observer* observer) {
On 2017/02/13 20:46:31, Rick Byers wrote:
> nit: IMHO this would make more sense as an instance method on UseCounter
instead
> of a static method.  We have a few static helper methods to factor out some
> biolerplate for the really common cases, but this is something that's only
going
> to be used in one or two places so I wouldn't both with a static personally.
> 
> This is minor though - if there's a reason you prefer this to be static then
> feel free to leave it.

There is no special reason. I just aligned it with other helpers when I made an
initial patch. I agree that an instance method makes more sense. Done.

[nhiroki, 2017-02-14 02:04:11]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-02-14 02:04:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2680423006/#ps100001
(title: "instance method")

[commit-bot: I haz the power, 2017-02-14 02:05:13]

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

[nhiroki, 2017-02-14 02:08:12]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2017-02-14 02:09:25]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2017-02-14 02:10:11]

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-14 04:20:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487038165071170,
"parent_rev": "d07584db64b31ca46f90b54b8cb65948d6dbc768", "commit_rev":
"452a477ff34a2af79a882b44ccc1939230c9e527"}

[commit-bot: I haz the power, 2017-02-14 04:20:50]

Description was changed from

==========
UseCounter: Introduce UseCounter::Observer for layout tests

This is a follow-up CL for [1]. We added some layout tests in the CL. They
assume MessagePort and UseCounter share the same IPC channel and their messages
are handled in order. However, this assumption will be untrue by MessagePort
mojofication[2] and the layout tests will be flaky.

To fix this, this CL stops assuming the message order and instead introduces a
mechanism to observe UseCounter changes.

[1] https://codereview.chromium.org/2586863002/
[2] https://codereview.chromium.org/2422793002/

BUG=376039
==========

to

==========
UseCounter: Introduce UseCounter::Observer for layout tests

This is a follow-up CL for [1]. We added some layout tests in the CL. They
assume MessagePort and UseCounter share the same IPC channel and their messages
are handled in order. However, this assumption will be untrue by MessagePort
mojofication[2] and the layout tests will be flaky.

To fix this, this CL stops assuming the message order and instead introduces a
mechanism to observe UseCounter changes.

[1] https://codereview.chromium.org/2586863002/
[2] https://codereview.chromium.org/2422793002/

BUG=376039

Review-Url: https://codereview.chromium.org/2680423006
Cr-Commit-Position: refs/heads/master@{#450241}
Committed:
https://chromium.googlesource.com/chromium/src/+/452a477ff34a2af79a882b44ccc1...
==========

[commit-bot: I haz the power, 2017-02-14 04:20:51]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/452a477ff34a2af79a882b44ccc1...