Worker: Enable UseCounter for SharedWorkerGlobalScope

Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo8c/edit#heading=h.g1fo4m6ayjxf

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, its counter is
  synchronized with the current use counts of the worker.

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

[Rick Byers, 2017-01-02 21:46:39]

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

[Rick Byers, 2017-01-02 21:46:40]

This seems like a reasonable start, thanks!  A few high-level thoughts:

1. There should be no way for a SharedWorker to depend on the behavior of a CSS
property (since it has no DOM access), right?  We should probably add a CHECK to
verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID
property) code path is never hit in the worker case (since otherwise we try to
keep the CSS and Feature cases of UseCounter consistent).

2. Do you really need separate IPCs for the feature and deprecation?  They both
boil down to calls to UseCounter::recordMeasurement, differing only in the
additional console logging behavior.  If you handle the console logging from
within the worker's renderer (as I think you're already doing?) then you can
just have a single recordMeasurement IPC.

3. In general we try to avoid changing semantics of existing histograms (instead
versioning the histogram under a new name) so that an analysis can be performed
over time without conflating changes in metrics with changes in how we measure
(eg. our usage graphs on chromestatus.com).  In http://crbug.com/676837 I'm in
the process of transitioning already (see UseCounter::LegacyCounter for the code
that preserves the existing semantics).

Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it via
inspection)?  We'll need to add a hook into UseCounter::count somewhere that
checks to see if the Frame represents a SharedWorker, right?  It looks like
perhaps it wouldn't be too hard to follow my existing pattern and have this
change impact only the new histogram.  I.e. hook into
UseCounter::recordMeasurement such that we still call into the m_legacyCounter
path, but replace the other path with the call that boils down to an IPC to the
browser.  Then we can add a new optional "disable legacy" argument to
UseCounter::recordMeasurement which we can use to update only the new histogram
in the host renderers.  When (after a few months of having both) I remove all
the "legacy" code/histograms this should simplify nicely.

[Rick Byers, 2017-01-02 22:01:45]

On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote:
> This seems like a reasonable start, thanks!  A few high-level thoughts:
> 
> 1. There should be no way for a SharedWorker to depend on the behavior of a
CSS
> property (since it has no DOM access), right?  We should probably add a CHECK
to
> verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID
> property) code path is never hit in the worker case (since otherwise we try to
> keep the CSS and Feature cases of UseCounter consistent).
> 
> 2. Do you really need separate IPCs for the feature and deprecation?  They
both
> boil down to calls to UseCounter::recordMeasurement, differing only in the
> additional console logging behavior.  If you handle the console logging from
> within the worker's renderer (as I think you're already doing?) then you can
> just have a single recordMeasurement IPC.
> 
> 3. In general we try to avoid changing semantics of existing histograms
(instead
> versioning the histogram under a new name) so that an analysis can be
performed
> over time without conflating changes in metrics with changes in how we measure
> (eg. our usage graphs on http://chromestatus.com).  In http://crbug.com/676837
I'm in
> the process of transitioning already (see UseCounter::LegacyCounter for the
code
> that preserves the existing semantics).
> 
> Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it
via
> inspection)?

Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature).  And so
a change in histogram semantics has already landed in M57 for the
DedicatedWorker case.  I'll try whipping up a quick prototype of what I'm
thinking to keep the change in semantics to the new experimental histograms.

>  We'll need to add a hook into UseCounter::count somewhere that
> checks to see if the Frame represents a SharedWorker, right?  It looks like
> perhaps it wouldn't be too hard to follow my existing pattern and have this
> change impact only the new histogram.  I.e. hook into
> UseCounter::recordMeasurement such that we still call into the m_legacyCounter
> path, but replace the other path with the call that boils down to an IPC to
the
> browser.  Then we can add a new optional "disable legacy" argument to
> UseCounter::recordMeasurement which we can use to update only the new
histogram
> in the host renderers.  When (after a few months of having both) I remove all
> the "legacy" code/histograms this should simplify nicely.

[Rick Byers, 2017-01-06 19:50:03]

On 2017/01/02 22:01:45, Rick Byers wrote:
> On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote:
> > This seems like a reasonable start, thanks!  A few high-level thoughts:
> > 
> > 1. There should be no way for a SharedWorker to depend on the behavior of a
> CSS
> > property (since it has no DOM access), right?  We should probably add a
CHECK
> to
> > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID
> > property) code path is never hit in the worker case (since otherwise we try
to
> > keep the CSS and Feature cases of UseCounter consistent).
> > 
> > 2. Do you really need separate IPCs for the feature and deprecation?  They
> both
> > boil down to calls to UseCounter::recordMeasurement, differing only in the
> > additional console logging behavior.  If you handle the console logging from
> > within the worker's renderer (as I think you're already doing?) then you can
> > just have a single recordMeasurement IPC.
> > 
> > 3. In general we try to avoid changing semantics of existing histograms
> (instead
> > versioning the histogram under a new name) so that an analysis can be
> performed
> > over time without conflating changes in metrics with changes in how we
measure
> > (eg. our usage graphs on http://chromestatus.com).  In
http://crbug.com/676837
> I'm in
> > the process of transitioning already (see UseCounter::LegacyCounter for the
> code
> > that preserves the existing semantics).
> > 
> > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find it
> via
> > inspection)?
> 
> Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature).  And
so
> a change in histogram semantics has already landed in M57 for the
> DedicatedWorker case.  I'll try whipping up a quick prototype of what I'm
> thinking to keep the change in semantics to the new experimental histograms.
> 
> >  We'll need to add a hook into UseCounter::count somewhere that
> > checks to see if the Frame represents a SharedWorker, right?  It looks like
> > perhaps it wouldn't be too hard to follow my existing pattern and have this
> > change impact only the new histogram.  I.e. hook into
> > UseCounter::recordMeasurement such that we still call into the
m_legacyCounter
> > path, but replace the other path with the call that boils down to an IPC to
> the
> > browser.  Then we can add a new optional "disable legacy" argument to
> > UseCounter::recordMeasurement which we can use to update only the new
> histogram
> > in the host renderers.  When (after a few months of having both) I remove
all
> > the "legacy" code/histograms this should simplify nicely.

I played with this a bit and it's pretty ugly.  I don't think it's worth the
cost - see my comment here:
https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32.  So you should
just keep doing what you were doing before with changing semantics of the
existing histograms - sorry for the noise!

[nhiroki, 2017-01-20 11:42:09]

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

[commit-bot: I haz the power, 2017-01-20 11:42:23]

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

[nhiroki, 2017-01-20 12:28:40]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

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

to

==========
(WIP) Worker: Enable UseCounter for SharedWorkerGlobalScope

This CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to
  Renderer(Document) via Browser.
- keeps the current counts in Browser so that a document newly connected to a
  worker can counts the API use so far.

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

[commit-bot: I haz the power, 2017-01-20 13:03:28]

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

[commit-bot: I haz the power, 2017-01-20 13:03:29]

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

[nhiroki, 2017-01-23 08:10:08]

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

[commit-bot: I haz the power, 2017-01-23 08:10:18]

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-01-23 08:45:38]

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

[commit-bot: I haz the power, 2017-01-23 08:45:40]

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

[nhiroki, 2017-01-23 11:24:47]

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

[commit-bot: I haz the power, 2017-01-23 11:25:01]

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-01-23 11:57:46]

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

[commit-bot: I haz the power, 2017-01-23 11:57:47]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[nhiroki, 2017-01-24 02:57:52]

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

[commit-bot: I haz the power, 2017-01-24 02:58:15]

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-01-24 05:01:18]

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

[commit-bot: I haz the power, 2017-01-24 05:01:19]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-01-24 07:27:19]

Description was changed from

==========
(WIP) Worker: Enable UseCounter for SharedWorkerGlobalScope

This CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- notifies API use on SharedWorkerGlobalScope from Renderer(Worker) to
  Renderer(Document) via Browser.
- keeps the current counts in Browser so that a document newly connected to a
  worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

  [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
    (calls API X)                   ---> [Renderer(Document2)]
                                    ---> [Renderer(Document3)]
                                    (record use of API X; the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

  [Browser] ---> [Renderer(Document1)]
            ---> [Renderer(Document2)]
            ---> [Renderer(Document3)]

            ---> [Renderer(Document4)]
                   (new document, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- makes IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-01-24 07:40:28]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

  [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
    (calls API X)                   ---> [Renderer(Document2)]
                                    ---> [Renderer(Document3)]
                                    (record use of API X; the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

  [Browser] ---> [Renderer(Document1)]
            ---> [Renderer(Document2)]
            ---> [Renderer(Document3)]

            ---> [Renderer(Document4)]
                   (new document, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- makes IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document1)]
              ---> [Renderer(Document2)]
              ---> [Renderer(Document3)]

              ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-01-24 07:42:24]

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

[commit-bot: I haz the power, 2017-01-24 07:42:36]

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-01-24 07:55:33]

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

[commit-bot: I haz the power, 2017-01-24 07:55:34]

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

[nhiroki, 2017-01-24 08:29:25]

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

[commit-bot: I haz the power, 2017-01-24 08:29:41]

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

[nhiroki, 2017-01-24 08:45:47]

Thank you for your early comments and sorry for my delayed responses. I think
this is now ready to review :)

On 2017/01/06 19:50:03, Rick Byers wrote:
> On 2017/01/02 22:01:45, Rick Byers wrote:
> > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote:
> > > This seems like a reasonable start, thanks!  A few high-level thoughts:
> > > 
> > > 1. There should be no way for a SharedWorker to depend on the behavior of
a
> > CSS
> > > property (since it has no DOM access), right?  We should probably add a
> CHECK
> > to
> > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID
> > > property) code path is never hit in the worker case (since otherwise we
try
> to
> > > keep the CSS and Feature cases of UseCounter consistent).

I took a quick look at this but didn't understand how to check if it's on
WorkerGlobalScope or not. How do we access an execution context in the function?

> > > 2. Do you really need separate IPCs for the feature and deprecation?  They
> > both
> > > boil down to calls to UseCounter::recordMeasurement, differing only in the
> > > additional console logging behavior.  If you handle the console logging
from
> > > within the worker's renderer (as I think you're already doing?) then you
can
> > > just have a single recordMeasurement IPC.

Good point. Yes, console logging is already done in the worker-side. Removed the
message for countDeprecation.

> > > 3. In general we try to avoid changing semantics of existing histograms
> > (instead
> > > versioning the histogram under a new name) so that an analysis can be
> > performed
> > > over time without conflating changes in metrics with changes in how we
> measure
> > > (eg. our usage graphs on http://chromestatus.com).  In
> http://crbug.com/676837
> > I'm in
> > > the process of transitioning already (see UseCounter::LegacyCounter for
the
> > code
> > > that preserves the existing semantics).
> > > 
> > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd find
it
> > via
> > > inspection)?
> > 
> > Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). 
And
> so
> > a change in histogram semantics has already landed in M57 for the
> > DedicatedWorker case.  I'll try whipping up a quick prototype of what I'm
> > thinking to keep the change in semantics to the new experimental histograms.
> > 
> > >  We'll need to add a hook into UseCounter::count somewhere that
> > > checks to see if the Frame represents a SharedWorker, right?  It looks
like
> > > perhaps it wouldn't be too hard to follow my existing pattern and have
this
> > > change impact only the new histogram.  I.e. hook into
> > > UseCounter::recordMeasurement such that we still call into the
> m_legacyCounter
> > > path, but replace the other path with the call that boils down to an IPC
to
> > the
> > > browser.  Then we can add a new optional "disable legacy" argument to
> > > UseCounter::recordMeasurement which we can use to update only the new
> > histogram
> > > in the host renderers.  When (after a few months of having both) I remove
> all
> > > the "legacy" code/histograms this should simplify nicely.
> 
> I played with this a bit and it's pretty ugly.  I don't think it's worth the
> cost - see my comment here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32.  So you
should
> just keep doing what you were doing before with changing semantics of the
> existing histograms - sorry for the noise!

Acknowledged.

[nhiroki, 2017-01-24 08:46:02]

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

[nhiroki, 2017-01-24 08:46:03]

Patchset #11 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-01-24 08:46:15]

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

[nhiroki, 2017-01-24 08:46:22]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document1)]
              ---> [Renderer(Document2)]
              ---> [Renderer(Document3)]

              ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document1)]
              ---> [Renderer(Document2)]
              ---> [Renderer(Document3)]

              ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-01-24 08:46:22]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org, kinuko@chromium.org

[nhiroki, 2017-01-24 08:46:38]

+horo, kinuko, can you review this? Thanks.

[nhiroki, 2017-01-24 09:54:11]

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html
(right):

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71:
assert_true(isUseCounted(frame3, FetchFeature));
Question for rbyers@: Do iframes in a document share the use counter? |frame3|
is created immediately before this line, but its use counter already counted
FetchFeature here. Is this expected behavior?

I assumed each iframe has its own use counter. To emulate the case where
multiple documents connect to the same shared worker, this test creates multiple
iframes. If this is expected behavior, we may not be able to test this case by
layout tests.

[commit-bot: I haz the power, 2017-01-24 11:21:19]

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

[commit-bot: I haz the power, 2017-01-24 11:21:20]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-24 13:56:31]

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
File content/common/worker_use_counter.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes =
kNumberOfFeatures / kBitsPerByte;
Do we need to use vector<char> here?  Maybe we could just use std::bitset, and
bitset::to_string and bitset(string) for serialization?

https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share...
File content/renderer/shared_worker/websharedworker_proxy.h (right):

https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share...
content/renderer/shared_worker/websharedworker_proxy.h:31: // connection is
established.
The comment needs to be updated.

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:249:
static_cast<uint32_t>(feature)));
nit: if we do exactly same as countFeature, we can just call it

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSharedWorkerConnectListener.h (right):

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSharedWorkerConnectListener.h:39: class
WebSharedWorkerConnectListener {
Maybe we should rename this as it's no longer for listening 'connect'

(Ok to do it in a separate CL)

[nhiroki, 2017-01-24 15:02:27]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document1)]
              ---> [Renderer(Document2)]
              ---> [Renderer(Document3)]

              ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-01-24 15:07:30]

Thank you for the comments. Comment reply only; I'll address them tomorrow.

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
File content/common/worker_use_counter.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes =
kNumberOfFeatures / kBitsPerByte;
On 2017/01/24 13:56:31, kinuko wrote:
> Do we need to use vector<char> here?  Maybe we could just use std::bitset, and
> bitset::to_string and bitset(string) for serialization?

I think std::bitset is a good option. Before moving to that, let me confirm 2
things...

std::bitset::to_string returns flags as a character string, that is, 1 bit is
converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the
string is 2KB and every SharedWorker connection / ServiceWorker association
causes 2KB data transfer over IPC. Is this acceptable in terms of performance? I
feel it'd be acceptable, but I'm not really sure.

std::bitset requires a bit length as a type parameter like std::bitset<N>, so
it's necessary to get the number of UseCounter::Feature on compile time here. Do
you think adding a public header (WebUseCounter.h??) to expose the size of
UseCounter::Feature is reasonable?

[horo, 2017-01-25 03:45:10]

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:353:
ScopedWorkerDependencyChecker checker(this);
You don't need this?

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:355:
FindSharedWorkerHost(filter->render_process_id(), worker_route_id))
nit: brackets

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:373:
FindSharedWorkerHost(filter->render_process_id(), worker_route_id))
nit: brackets

https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me...
content/common/view_messages.h:542: IPC_MESSAGE_ROUTED1(ViewMsg_WorkerConnected,
Please write comment about |features|.

[kinuko, 2017-01-25 05:12:52]

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
File content/common/worker_use_counter.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes =
kNumberOfFeatures / kBitsPerByte;
On 2017/01/24 15:07:30, nhiroki wrote:
> On 2017/01/24 13:56:31, kinuko wrote:
> > Do we need to use vector<char> here?  Maybe we could just use std::bitset,
and
> > bitset::to_string and bitset(string) for serialization?
> 
> I think std::bitset is a good option. Before moving to that, let me confirm 2
> things...
> 
> std::bitset::to_string returns flags as a character string, that is, 1 bit is
> converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the
> string is 2KB and every SharedWorker connection / ServiceWorker association
> causes 2KB data transfer over IPC. Is this acceptable in terms of performance?
I
> feel it'd be acceptable, but I'm not really sure.

Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that it
might sound a little wasteful.

How many features are usually flipped on or off?  Could using SmallMap<int,
bool> be better? (Then we get IPC serializers for free)

> std::bitset requires a bit length as a type parameter like std::bitset<N>, so
> it's necessary to get the number of UseCounter::Feature on compile time here.
Do
> you think adding a public header (WebUseCounter.h??) to expose the size of
> UseCounter::Feature is reasonable?

I thought about just use kNumberOfFeatures and asserts on the size so that the
code can be synchronized.

[kinuko, 2017-01-25 07:47:47]

On 2017/01/25 05:12:52, kinuko wrote:
>
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
> File content/common/worker_use_counter.cc (right):
> 
>
https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
> content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes =
> kNumberOfFeatures / kBitsPerByte;
> On 2017/01/24 15:07:30, nhiroki wrote:
> > On 2017/01/24 13:56:31, kinuko wrote:
> > > Do we need to use vector<char> here?  Maybe we could just use std::bitset,
> and
> > > bitset::to_string and bitset(string) for serialization?
> > 
> > I think std::bitset is a good option. Before moving to that, let me confirm
2
> > things...
> > 
> > std::bitset::to_string returns flags as a character string, that is, 1 bit
is
> > converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the
> > string is 2KB and every SharedWorker connection / ServiceWorker association
> > causes 2KB data transfer over IPC. Is this acceptable in terms of
performance?
> I
> > feel it'd be acceptable, but I'm not really sure.
> 
> Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that
it
> might sound a little wasteful.
> 
> How many features are usually flipped on or off?  Could using SmallMap<int,
> bool> be better? (Then we get IPC serializers for free)

Or could this be just std::set<int> for the 'count'ed features?

> > std::bitset requires a bit length as a type parameter like std::bitset<N>,
so
> > it's necessary to get the number of UseCounter::Feature on compile time
here.
> Do
> > you think adding a public header (WebUseCounter.h??) to expose the size of
> > UseCounter::Feature is reasonable?
> 
> I thought about just use kNumberOfFeatures and asserts on the size so that the
> code can be synchronized.

[nhiroki, 2017-01-25 09:55:11]

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

[commit-bot: I haz the power, 2017-01-25 09:55:30]

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

[nhiroki, 2017-01-25 10:16:12]

Thanks for your comments. Updated.

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
File content/browser/shared_worker/shared_worker_service_impl.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:353:
ScopedWorkerDependencyChecker checker(this);
On 2017/01/25 03:45:10, horo wrote:
> You don't need this?

Done.

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:355:
FindSharedWorkerHost(filter->render_process_id(), worker_route_id))
On 2017/01/25 03:45:10, horo wrote:
> nit: brackets

Done.

https://codereview.chromium.org/2586863002/diff/220001/content/browser/shared...
content/browser/shared_worker/shared_worker_service_impl.cc:373:
FindSharedWorkerHost(filter->render_process_id(), worker_route_id))
On 2017/01/25 03:45:10, horo wrote:
> nit: brackets

Done.

https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me...
File content/common/view_messages.h (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/view_me...
content/common/view_messages.h:542: IPC_MESSAGE_ROUTED1(ViewMsg_WorkerConnected,
On 2017/01/25 03:45:10, horo wrote:
> Please write comment about |features|.

Done.

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
File content/common/worker_use_counter.cc (right):

https://codereview.chromium.org/2586863002/diff/220001/content/common/worker_...
content/common/worker_use_counter.cc:17: constexpr size_t kNumberOfBytes =
kNumberOfFeatures / kBitsPerByte;
On 2017/01/25 05:12:52, kinuko wrote:
> On 2017/01/24 15:07:30, nhiroki wrote:
> > On 2017/01/24 13:56:31, kinuko wrote:
> > > Do we need to use vector<char> here?  Maybe we could just use std::bitset,
> and
> > > bitset::to_string and bitset(string) for serialization?
> > 
> > I think std::bitset is a good option. Before moving to that, let me confirm
2
> > things...
> > 
> > std::bitset::to_string returns flags as a character string, that is, 1 bit
is
> > converted to 1 byte. If UseCoutner::Feature has 2000 features, size of the
> > string is 2KB and every SharedWorker connection / ServiceWorker association
> > causes 2KB data transfer over IPC. Is this acceptable in terms of
performance?
> I
> > feel it'd be acceptable, but I'm not really sure.
> 
> Usually sending IPC up to 2KB doesn't make a lot difference, but I agree that
it
> might sound a little wasteful.
> 
> How many features are usually flipped on or off?  Could using SmallMap<int,
> bool> be better? (Then we get IPC serializers for free)

I think it's not so big for now because most of features are available only on
Document (e.g., DOM, CSS).

std::set SGTM. Replaced WorkerUseCounter with std::set.

> > std::bitset requires a bit length as a type parameter like std::bitset<N>,
so
> > it's necessary to get the number of UseCounter::Feature on compile time
here.
> Do
> > you think adding a public header (WebUseCounter.h??) to expose the size of
> > UseCounter::Feature is reasonable?
> 
> I thought about just use kNumberOfFeatures and asserts on the size so that the
> code can be synchronized.

Thanks. std::set does not need the max number, so I didn't add a public header
or an assertion.

https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share...
File content/renderer/shared_worker/websharedworker_proxy.h (right):

https://codereview.chromium.org/2586863002/diff/220001/content/renderer/share...
content/renderer/shared_worker/websharedworker_proxy.h:31: // connection is
established.
On 2017/01/24 13:56:31, kinuko wrote:
> The comment needs to be updated.

Done.

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:249:
static_cast<uint32_t>(feature)));
On 2017/01/24 13:56:31, kinuko wrote:
> nit: if we do exactly same as countFeature, we can just call it

Done.

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSharedWorkerConnectListener.h (right):

https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSharedWorkerConnectListener.h:39: class
WebSharedWorkerConnectListener {
On 2017/01/24 13:56:31, kinuko wrote:
> Maybe we should rename this as it's no longer for listening 'connect'
> 
> (Ok to do it in a separate CL)

Acknowledged.

[commit-bot: I haz the power, 2017-01-25 11:29:46]

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

[commit-bot: I haz the power, 2017-01-25 11:29:47]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-01-25 12:32:41]

lgtm

[horo, 2017-01-26 01:52:36]

lgtm

[Rick Byers, 2017-01-26 02:28:06]

On 2017/01/24 08:45:47, nhiroki wrote:
> Thank you for your early comments and sorry for my delayed responses. I think
> this is now ready to review :)
> 
> On 2017/01/06 19:50:03, Rick Byers wrote:
> > On 2017/01/02 22:01:45, Rick Byers wrote:
> > > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote:
> > > > This seems like a reasonable start, thanks!  A few high-level thoughts:
> > > > 
> > > > 1. There should be no way for a SharedWorker to depend on the behavior
of
> a
> > > CSS
> > > > property (since it has no DOM access), right?  We should probably add a
> > CHECK
> > > to
> > > > verify the UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID
> > > > property) code path is never hit in the worker case (since otherwise we
> try
> > to
> > > > keep the CSS and Feature cases of UseCounter consistent).
> 
> I took a quick look at this but didn't understand how to check if it's on
> WorkerGlobalScope or not. How do we access an execution context in the
function?

Yeah it looks not easy - we'd probably need to instrument something more
upstream in CSSParserContext.  As long as you're confident we shouldn't be
parsing CSS in workers then that's good enough for me.

> > > > 2. Do you really need separate IPCs for the feature and deprecation? 
They
> > > both
> > > > boil down to calls to UseCounter::recordMeasurement, differing only in
the
> > > > additional console logging behavior.  If you handle the console logging
> from
> > > > within the worker's renderer (as I think you're already doing?) then you
> can
> > > > just have a single recordMeasurement IPC.
> 
> Good point. Yes, console logging is already done in the worker-side. Removed
the
> message for countDeprecation.
> 
> > > > 3. In general we try to avoid changing semantics of existing histograms
> > > (instead
> > > > versioning the histogram under a new name) so that an analysis can be
> > > performed
> > > > over time without conflating changes in metrics with changes in how we
> > measure
> > > > (eg. our usage graphs on http://chromestatus.com).  In
> > http://crbug.com/676837
> > > I'm in
> > > > the process of transitioning already (see UseCounter::LegacyCounter for
> the
> > > code
> > > > that preserves the existing semantics).
> > > > 
> > > > Does anything call WorkerGlobalScope::countFeature today (I couldn'd
find
> it
> > > via
> > > > inspection)?
> > > 
> > > Oh sorry, I see it now in UseCounter::count(ExecutionContext*, Feature). 
> And
> > so
> > > a change in histogram semantics has already landed in M57 for the
> > > DedicatedWorker case.  I'll try whipping up a quick prototype of what I'm
> > > thinking to keep the change in semantics to the new experimental
histograms.
> > > 
> > > >  We'll need to add a hook into UseCounter::count somewhere that
> > > > checks to see if the Frame represents a SharedWorker, right?  It looks
> like
> > > > perhaps it wouldn't be too hard to follow my existing pattern and have
> this
> > > > change impact only the new histogram.  I.e. hook into
> > > > UseCounter::recordMeasurement such that we still call into the
> > m_legacyCounter
> > > > path, but replace the other path with the call that boils down to an IPC
> to
> > > the
> > > > browser.  Then we can add a new optional "disable legacy" argument to
> > > > UseCounter::recordMeasurement which we can use to update only the new
> > > histogram
> > > > in the host renderers.  When (after a few months of having both) I
remove
> > all
> > > > the "legacy" code/histograms this should simplify nicely.
> > 
> > I played with this a bit and it's pretty ugly.  I don't think it's worth the
> > cost - see my comment here:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=376039#c32.  So you
> should
> > just keep doing what you were doing before with changing semantics of the
> > existing histograms - sorry for the noise!
> 
> Acknowledged.

[Rick Byers, 2017-01-26 02:30:35]

On 2017/01/24 09:54:11, nhiroki wrote:
>
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html
> (right):
> 
>
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71:
> assert_true(isUseCounted(frame3, FetchFeature));
> Question for rbyers@: Do iframes in a document share the use counter? |frame3|
> is created immediately before this line, but its use counter already counted
> FetchFeature here. Is this expected behavior?
> 
> I assumed each iframe has its own use counter. To emulate the case where
> multiple documents connect to the same shared worker, this test creates
multiple
> iframes. If this is expected behavior, we may not be able to test this case by
> layout tests.

Yes iframes share a single UseCounter (note Page::m_useCounter - page refers to
all the frames in a tab).

Conceptually we want to measure what fraction of "page views" may be impacted by
a change (since that's what matters to users) - whether a page has 0 or 100
iframes that shouldn't change it's weight.

[Rick Byers, 2017-01-26 03:02:37]

This looks good, thank you!  Just a couple issues with the test.

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js
(right):

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js:5:
fetch('notfound').catch(() => port.postMessage('fetched'));
please use a feature which is Deprecated (or even one of each).  I think you're
going to hit a DCHECK for the deprecated case (since UseCounter.cpp today
doesn't expect deprecated features to come through the normal path).  Perhaps we
should just relax that DCHECK if there's not an easy way to separate this case
out...

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html
(right):

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:12:
return frame.contentWindow.internals.isUseCounted(
Note that this test is pretty limited - doesn't verify how many counts were
increased and what the PageVisits impact was.  There are ways of getting the
full histogram data out to JavaScript, but honestly for UseCounters I don't
think it's worth a huge effort to test more than the basics which you're doing
here.

Probably better to run a few manual test cases and make sure the histograms look
right.  For manual testing use either
chrome://histograms/Blink.UseCounter.Features (knowing that it can be a little
finicky to get it to update - may need to reload the page), or for more precise
control run with --enable-stats-collection-bindings and from the devtools
console issue commands like
JSON.parse(statsCollectionController.getHistogram("Blink.UseCounter.Features"))
to poke at the entire histogram for THAT renderer.

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:70:
//assert_false(isUseCounted(frame3, FetchFeature));
Instead of using frames (which share a UseCounter) perhaps you can test this
with popups (window.open) that you communicate with?  IIRC there's some
window.internals settings for disabling all the pop-up blocker stuff and
ensuring all such new windows get closed when the test finishes.

[nhiroki, 2017-01-27 12:07:46]

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

[commit-bot: I haz the power, 2017-01-27 12:07:56]

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-01-27 13:41:51]

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

[commit-bot: I haz the power, 2017-01-27 13:41:52]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-01-27 16:54:24]

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

[commit-bot: I haz the power, 2017-01-27 16:54:46]

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

[nhiroki, 2017-01-27 16:58:16]

Thanks for your comments. Updated.

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js
(right):

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/resources/shared-worker-usecounter.js:5:
fetch('notfound').catch(() => port.postMessage('fetched'));
On 2017/01/26 03:02:37, Rick Byers wrote:
> please use a feature which is Deprecated (or even one of each).  I think
you're
> going to hit a DCHECK for the deprecated case (since UseCounter.cpp today
> doesn't expect deprecated features to come through the normal path).  Perhaps
we
> should just relax that DCHECK if there's not an easy way to separate this case
> out...

You're right! Calling a deprecated feature hits DCHECK. Instead of relaxing the
check, the latest patchset ccheck whether a given feature is deprecated and
calls countDeprecate in the case. See
SharedWorkerRepositoryClientImpl.cpp::countFeature.

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html
(right):

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:12:
return frame.contentWindow.internals.isUseCounted(
On 2017/01/26 03:02:37, Rick Byers wrote:
> Note that this test is pretty limited - doesn't verify how many counts were
> increased and what the PageVisits impact was.  There are ways of getting the
> full histogram data out to JavaScript, but honestly for UseCounters I don't
> think it's worth a huge effort to test more than the basics which you're doing
> here.
> 
> Probably better to run a few manual test cases and make sure the histograms
look
> right.  For manual testing use either
> chrome://histograms/Blink.UseCounter.Features (knowing that it can be a little
> finicky to get it to update - may need to reload the page), or for more
precise
> control run with --enable-stats-collection-bindings and from the devtools
> console issue commands like
>
JSON.parse(statsCollectionController.getHistogram("Blink.UseCounter.Features"))
> to poke at the entire histogram for THAT renderer.

Thank you for the guide!

Using the command line flag, I confirmed that (1) PageVisits counts only real
page views (not including SharedWorker loads), (2) API use on
SharedWorkerGlobalScope is recorded only one time on each connected document,
and (3) a counter of a newly connected document is correctly synced with a
counter of an existing shared worker.

In conclusion, this seems to be working well :)

https://codereview.chromium.org/2586863002/diff/240001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:70:
//assert_false(isUseCounted(frame3, FetchFeature));
On 2017/01/26 03:02:37, Rick Byers wrote:
> Instead of using frames (which share a UseCounter) perhaps you can test this
> with popups (window.open) that you communicate with?  IIRC there's some
> window.internals settings for disabling all the pop-up blocker stuff and
> ensuring all such new windows get closed when the test finishes.

Good idea. Done.

It looks like window.open() works without window.internals settings.
add_completion_callback() ensures all windows get closed when the test finishes.

[nhiroki, 2017-01-27 17:05:04]

On 2017/01/26 02:28:06, Rick Byers wrote:
> On 2017/01/24 08:45:47, nhiroki wrote:
> > Thank you for your early comments and sorry for my delayed responses. I
think
> > this is now ready to review :)
> > 
> > On 2017/01/06 19:50:03, Rick Byers wrote:
> > > On 2017/01/02 22:01:45, Rick Byers wrote:
> > > > On 2017/01/02 21:46:40, Rick Byers OOO until 1-2 wrote:
> > > > > This seems like a reasonable start, thanks!  A few high-level
thoughts:
> > > > > 
> > > > > 1. There should be no way for a SharedWorker to depend on the behavior
> of
> > a
> > > > CSS
> > > > > property (since it has no DOM access), right?  We should probably add
a
> > > CHECK
> > > > to
> > > > > verify the UseCounter::count(CSSParserMode cssParserMode,
CSSPropertyID
> > > > > property) code path is never hit in the worker case (since otherwise
we
> > try
> > > to
> > > > > keep the CSS and Feature cases of UseCounter consistent).
> > 
> > I took a quick look at this but didn't understand how to check if it's on
> > WorkerGlobalScope or not. How do we access an execution context in the
> function?
> 
> Yeah it looks not easy - we'd probably need to instrument something more
> upstream in CSSParserContext.  As long as you're confident we shouldn't be
> parsing CSS in workers then that's good enough for me.

I'm not 100% sure but I don't think it's called on workers because It's called
only from CSSPropertyParser and workers cannot access CSS properties.

[nhiroki, 2017-01-27 17:06:39]

On 2017/01/26 02:30:35, Rick Byers wrote:
> On 2017/01/24 09:54:11, nhiroki wrote:
> >
>
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2586863002/diff/220001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/fast/workers/shared-worker-usecounter.html:71:
> > assert_true(isUseCounted(frame3, FetchFeature));
> > Question for rbyers@: Do iframes in a document share the use counter?
|frame3|
> > is created immediately before this line, but its use counter already counted
> > FetchFeature here. Is this expected behavior?
> > 
> > I assumed each iframe has its own use counter. To emulate the case where
> > multiple documents connect to the same shared worker, this test creates
> multiple
> > iframes. If this is expected behavior, we may not be able to test this case
by
> > layout tests.
> 
> Yes iframes share a single UseCounter (note Page::m_useCounter - page refers
to
> all the frames in a tab).
> 
> Conceptually we want to measure what fraction of "page views" may be impacted
by
> a change (since that's what matters to users) - whether a page has 0 or 100
> iframes that shouldn't change it's weight.

I see. Thank you for the clarification.

[nhiroki, 2017-01-27 17:09:11]

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

[nhiroki, 2017-01-27 17:09:13]

+dcheng@, can you review following files as an owner?

- content/common/view_messages.h
- content/common/worker_messages.h
- third_party/WebKit/Source/web/*

Thanks!

[commit-bot: I haz the power, 2017-01-27 18:19:24]

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

[commit-bot: I haz the power, 2017-01-27 18:19:25]

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

[nhiroki, 2017-01-30 14:46:54]

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

[commit-bot: I haz the power, 2017-01-30 14:47:07]

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-01-30 16:25:44]

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

[commit-bot: I haz the power, 2017-01-30 16:25:45]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-01-30 18:00:01]

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

[commit-bot: I haz the power, 2017-01-30 18:00:45]

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-01-30 20:07:42]

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

[commit-bot: I haz the power, 2017-01-30 20:07:43]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-03 00:40:41]

https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share...
File content/renderer/shared_worker/websharedworker_proxy.h (right):

https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share...
content/renderer/shared_worker/websharedworker_proxy.h:48: void
OnWorkerConnected(std::set<uint32_t> features);
Nit: const std::set<uint32_t>&

https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:226:
static_assert(UseCounter::NumberOfFeatures < UINT32_MAX,
Another option is just to write this in UseCounter.h

enum Features : uint32_t {
  ...
};

[nhiroki, 2017-02-03 16:32:13]

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

[commit-bot: I haz the power, 2017-02-03 16:32:59]

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-03 17:08:26]

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

[commit-bot: I haz the power, 2017-02-03 17:08:55]

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-03 17:18:05]

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

[commit-bot: I haz the power, 2017-02-03 17:18:34]

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-03 17:28:22]

Thanks for your comments. Updated.

https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share...
File content/renderer/shared_worker/websharedworker_proxy.h (right):

https://codereview.chromium.org/2586863002/diff/340001/content/renderer/share...
content/renderer/shared_worker/websharedworker_proxy.h:48: void
OnWorkerConnected(std::set<uint32_t> features);
On 2017/02/03 00:40:40, dcheng wrote:
> Nit: const std::set<uint32_t>&

Done.

https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2586863002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:226:
static_assert(UseCounter::NumberOfFeatures < UINT32_MAX,
On 2017/02/03 00:40:40, dcheng wrote:
> Another option is just to write this in UseCounter.h
> 
> enum Features : uint32_t {
>   ...
> };

Good idea! Added it and removed static_asserts.

[dcheng, 2017-02-03 18:27:50]

LGTM with a naming nit.

I know the latest PS renamed feature -> useCounterId, but I think it's clearer
to call it feature. For example, the std::set<uint32_t> is currently named
|use_counters| but the purpose of this isn't very clear from the variable name.

If we called referred to this as feature, we can name the set |used_features|,
which makes the meaning of the sets clearer.

https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared...
content/browser/shared_worker/shared_worker_host.h:146: std::set<uint32_t>
use_counter_;
Nit: add a comment on this and note that this is the set of features that the
shared worker has used.

[commit-bot: I haz the power, 2017-02-03 19:11:40]

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

[commit-bot: I haz the power, 2017-02-03 19:11:41]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-02-06 00:30:55]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected to 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (WorkerUseCounter) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected from 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-02-06 00:35:57]

https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared...
File content/browser/shared_worker/shared_worker_host.h (right):

https://codereview.chromium.org/2586863002/diff/400001/content/browser/shared...
content/browser/shared_worker/shared_worker_host.h:146: std::set<uint32_t>
use_counter_;
On 2017/02/03 18:27:50, dcheng wrote:
> Nit: add a comment on this and note that this is the set of features that the
> shared worker has used.

Done.

[nhiroki, 2017-02-06 00:36:04]

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

[commit-bot: I haz the power, 2017-02-06 00:36:15]

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-06 00:37:07]

Thank you.

On 2017/02/03 18:27:50, dcheng wrote:
> LGTM with a naming nit.
> 
> I know the latest PS renamed feature -> useCounterId, but I think it's clearer
> to call it feature. For example, the std::set<uint32_t> is currently named
> |use_counters| but the purpose of this isn't very clear from the variable
name.
> 
> If we called referred to this as feature, we can name the set |used_features|,
> which makes the meaning of the sets clearer.

Yeah, |used_features| sounds much clearer. Renamed them.

[nhiroki, 2017-02-06 00:39:22]

rbyers@, ping :)

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

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

[commit-bot: I haz the power, 2017-02-06 02:10:35]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-02-07 09:10:32]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, a shared worker connected from 3 documents calls some
  API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-02-07 09:26:16]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, the worker
  notifies the document of the current use counts so that the document can
  synchronize with its UseCounter with others:

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, its counter is
  synchronized with the current use counts of the worker.

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

[nhiroki, 2017-02-08 06:37:21]

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

[commit-bot: I haz the power, 2017-02-08 06:37:44]

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-08 08:44:33]

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

[commit-bot: I haz the power, 2017-02-08 08:44:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Rick Byers, 2017-02-09 01:43:45]

Shoot, sorry I missed this.  The test looks awesome now, thank you!
LGTM

[nhiroki, 2017-02-09 01:53:42]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-02-09 01:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
horo@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2586863002/#ps460001
(title: "simplify")

[commit-bot: I haz the power, 2017-02-09 01:54: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-09 03:56:57]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1486605222529510,
"parent_rev": "8499b1584c9c4a0c3b018355f0301a3596d93c2f", "commit_rev":
"efd1adc4978287cb9f36a91694fc21c482f996e1"}

[commit-bot: I haz the power, 2017-02-09 03:57:38]

Description was changed from

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, its counter is
  synchronized with the current use counts of the worker.

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

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

to

==========
Worker: Enable UseCounter for SharedWorkerGlobalScope

DesignDoc:
https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo...

This CL enables UseCounter for SharedWorker. This works as follows:

- API use on SharedWorkerGlobalScope is recorded in all documents connecting to
  the worker. For example, when a shared worker connected from 3 documents calls
  some API, it's recorded in their UseCounter as if they all called the API:

    [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
      (calls API X)                   ---> [Renderer(Document2)]
                                      ---> [Renderer(Document3)]
                                         (each document records use of API X;
                                          the total count of API X is 3)

- When a new document connects to the existing shared worker, its counter is
  synchronized with the current use counts of the worker.

    [Browser] ---> [Renderer(Document4)]
                     (new doc, but has the same counts with documents1-3;
                      the total count of API X is 4)

To achieve the above, this CL...

- extends the lifetime of SharedWorkerConenctListener and uses it as a proxy
  between Renderer(Document) and Browser.
- introduces new IPC path to notifies API use on SharedWorkerGlobalScope from
  Renderer(Worker) to Renderer(Document) via Browser.
- keeps the current counts in Browser (SharedWorkerHost) so that a document
  newly connected to a worker can counts the API use so far.

BUG=376039

Review-Url: https://codereview.chromium.org/2586863002
Cr-Commit-Position: refs/heads/master@{#449196}
Committed:
https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc...
==========

[commit-bot: I haz the power, 2017-02-09 03:57:41]

Committed patchset #23 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/efd1adc4978287cb9f36a91694fc...

[darin (slow to review), 2017-02-09 20:33:58]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2017-02-09 20:33:59]

It looks like the tests here assume that MessagePort communication shares the
same FIFO as Chrome IPC messages. That happens to be true in the current
implementation. I'm trying to make it untrue in a new implementation here:
https://codereview.chromium.org/2422793002

There are performance benefits to doing so. I'm trying to figure out the best
way to support the use counter tests here that rely on the use counter IPCs
happening before MessagePort IPCs. Suggestions welcome :)

[nhiroki, 2017-02-10 00:26:00]

On 2017/02/09 20:33:59, darin (slow to review) wrote:
> It looks like the tests here assume that MessagePort communication shares the
> same FIFO as Chrome IPC messages. That happens to be true in the current
> implementation. I'm trying to make it untrue in a new implementation here:
> https://codereview.chromium.org/2422793002
> 
> There are performance benefits to doing so. I'm trying to figure out the best
> way to support the use counter tests here that rely on the use counter IPCs
> happening before MessagePort IPCs. Suggestions welcome :)

Please see: https://codereview.chromium.org/2658603003/#msg97

I'll make a patch for a mechanism to observe UseCounter changes.