Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843
Committed: https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3
Cr-Commit-Position: refs/heads/master@{#420546}

[nhiroki, 2016-09-21 06:56:00]

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

[nhiroki, 2016-09-21 06:56:07]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-21 06:56:30]

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

[nhiroki, 2016-09-21 07:00:14]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() is called during WorkerThread
initialization. This looks late, but it is necessary because one of observers
(ServiceWorker) wants to access WorkerGlobalScope in the event that is created
during WorkerThread initialization to record size of worker scripts.

After this CL, recording of the size is done in willEvaluateWorkerScript() that
would be more appropriate for the operation. This is also good for unit tests
because willEvaluateWorkerScript() enables them to wait until script evaluation
more precisely.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was necessary because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization to record size of a worker
script.

After this CL, recording the size is done in willEvaluateWorkerScript() that
should be more appropriate for the operation. This notification is also good for
unit tests because they can wait until script evaluation more precisely.

BUG=640843
==========

[nhiroki, 2016-09-21 07:03:10]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was necessary because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization to record size of a worker
script.

After this CL, recording the size is done in willEvaluateWorkerScript() that
should be more appropriate for the operation. This notification is also good for
unit tests because they can wait until script evaluation more precisely.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was necessary because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization to record size of a worker
script.

After this CL, recording the size is done in willEvaluateWorkerScript() that
should be more appropriate for the operation. This notification is also good for
unit tests. They wanted to wait until script evaluation, but instead they waited
until didLoadWorkerScript() because of the lack of the notification. This CL
enablest them to wait can wait until the more precise timing.

BUG=640843
==========

[nhiroki, 2016-09-21 07:09:51]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was necessary because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization to record size of a worker
script.

After this CL, recording the size is done in willEvaluateWorkerScript() that
should be more appropriate for the operation. This notification is also good for
unit tests. They wanted to wait until script evaluation, but instead they waited
until didLoadWorkerScript() because of the lack of the notification. This CL
enablest them to wait can wait until the more precise timing.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait.

BUG=640843
==========

[nhiroki, 2016-09-21 07:10:44]

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

[nhiroki, 2016-09-21 07:10:45]

PTAL, thanks!

[commit-bot: I haz the power, 2016-09-21 08:53:19]

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

[commit-bot: I haz the power, 2016-09-21 08:53:19]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-21 14:10:17]

https://codereview.chromium.org/2355723005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/2355723005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerReportingProxy.h:64: // evaluated.
nit: the "Evaluate" function names and comments are a bit inconsistent.
didEvaluateWorkerScript is only called for the main script.
willEvaluateWorkerScript is called for the main script and every imported
script.

Maybe the function names should be something like "willEvaluateWorkerScript" and
"didEvaluateMainWorkerScript"?

[nhiroki, 2016-09-21 15:37:57]

Thank you! Updated.

https://codereview.chromium.org/2355723005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/2355723005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerReportingProxy.h:64: // evaluated.
On 2016/09/21 14:10:17, falken wrote:
> nit: the "Evaluate" function names and comments are a bit inconsistent.
> didEvaluateWorkerScript is only called for the main script.
> willEvaluateWorkerScript is called for the main script and every imported
> script.

Good point. Yeah, this looks strange...

> Maybe the function names should be something like "willEvaluateWorkerScript"
and
> "didEvaluateMainWorkerScript"?

I'm now thinking that each event should have only single responsibility, that
is, willEvaluateWorkerScript should be separated into willEvaluateWorkerScript()
for the main worker script and willEvaluateImportedScript() for imported
scripts. The patchset2 does this. WDYT?

[nhiroki, 2016-09-21 15:38:04]

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

[commit-bot: I haz the power, 2016-09-21 15:38:25]

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

[commit-bot: I haz the power, 2016-09-21 17:14:53]

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

[commit-bot: I haz the power, 2016-09-21 17:14:53]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-23 00:22:06]

I like that. lgtm

https://codereview.chromium.org/2355723005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/2355723005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerReportingProxy.h:67: // Invoked
when the imported script is about to be evaluated. This is called
nit: "an" since there can be multiple imported scripts.

[nhiroki, 2016-09-23 00:25:13]

Thank you!

https://codereview.chromium.org/2355723005/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/2355723005/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerReportingProxy.h:67: // Invoked
when the imported script is about to be evaluated. This is called
On 2016/09/23 00:22:05, falken wrote:
> nit: "an" since there can be multiple imported scripts.

Done.

[nhiroki, 2016-09-23 00:26:41]

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

[nhiroki, 2016-09-23 00:26:42]

+haraken, can you review Source/web/? Thanks!

[haraken, 2016-09-23 00:27:32]

On 2016/09/23 00:26:42, nhiroki wrote:
> +haraken, can you review Source/web/? Thanks!

LGTM

[nhiroki, 2016-09-23 00:40:26]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843
==========

[nhiroki, 2016-09-23 00:40:32]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-09-23 00:40:33]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2355723005/#ps60001
(title: "fix comment")

[commit-bot: I haz the power, 2016-09-23 00:41:13]

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

[commit-bot: I haz the power, 2016-09-23 02:22:12]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843
==========

[commit-bot: I haz the power, 2016-09-23 02:22:13]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-23 02:24:27]

Description was changed from

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843
==========

to

==========
Worker: Introduce WorkerReportingProxy::willEvaluateWorkerScript()

This introduces WorkerReportingProxy::willEvaluateWorkerScript() and removes
WorkerReportingProxy::didLoadWorkerScript().

Before this CL, didLoadWorkerScript() was called during WorkerThread
initialization. This timing was late, but it was intended because one of
observers (ServiceWorker) wanted to access WorkerGlobalScope in the notification
that is created during WorkerThread initialization in order to record size of
worker scripts.

After this CL, recording the size is done in willEvaluateWorkerScript() that
would be more appropriate timing for the operation. willEvaluateWorkerScript()
is also useful for unit tests. It enables them to wait until the timing that
they really wait for.

BUG=640843

Committed: https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3
Cr-Commit-Position: refs/heads/master@{#420546}
==========

[commit-bot: I haz the power, 2016-09-23 02:24:28]

Patchset 3 (id:??) landed as
https://crrev.com/97f2c9054251e15ae8520cc7bf193e18c32eb6b3
Cr-Commit-Position: refs/heads/master@{#420546}