Explicitly track the lifecycle of PendingScript.

Explicitly track the lifecycle of PendingScript.

This moves the lifecycle tracking of PendingScript from relying
on querying its parts (the resource, streamer, etc.) to maintaining
an explicit state machine.

Firstly, this means that the apparent state to change is timed to
coincide with when clients are notified (as opposed to a client being
able to notice that a PendingScript is finished before being notified).

It also paves the way for additional steps (e.g. separately scheduled
compile of the script) without making IsReady and ErrorOccurred yet
more complicated.

BUG=591558
Review-Url: https://codereview.chromium.org/2828193003
Cr-Commit-Position: refs/heads/master@{#468676}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9f0aa85bc1e34ad03dc963ef589dc3fbed21bd

[jbroman, 2017-04-20 18:45:10]

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

[commit-bot: I haz the power, 2017-04-20 18:45:57]

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

[jbroman, 2017-04-20 20:44:04]

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

[commit-bot: I haz the power, 2017-04-20 20:45:01]

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

[jbroman, 2017-04-20 21:14:13]

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

[commit-bot: I haz the power, 2017-04-20 21:14:54]

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-04-20 22:59:57]

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

[commit-bot: I haz the power, 2017-04-20 22:59:58]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-04-20 23:14:30]

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

[commit-bot: I haz the power, 2017-04-20 23:14: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-04-21 01:04:41]

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

[commit-bot: I haz the power, 2017-04-21 01:04:41]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-04-21 15:41:25]

Description was changed from

==========
WIP: Explicitly track the lifecycle of PendingScript.

BUG=591558
==========

to

==========
Explicitly track the lifecycle of PendingScript.

This moves the lifecycle tracking of PendingScript from relying
on querying its parts (the resource, streamer, etc.) to maintaining
an explicit state machine.

Firstly, this means that the apparent state to change is timed to
coincide with when clients are notified (as opposed to a client being
able to notice that a PendingScript is finished before being notified).

It also paves the way for additional steps (e.g. separately scheduled
compile of the script) without making IsReady and ErrorOccurred yet
more complicated.

BUG=591558
==========

[jbroman, 2017-04-21 15:41:26]

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

[jbroman, 2017-04-21 15:42:31]

WDYT? I found this change useful in tracking the state of the PendingScript
while I was working on followup patches to this, to add a separate
"[pre-]compile" step.

[kouhei (in TOK), 2017-04-24 01:12:52]

kouhei@chromium.org changed reviewers:
+ hiroshige@chromium.org, japhet@chromium.org

[kouhei (in TOK), 2017-04-24 01:12:52]

+hiroshige,japhet
I'm a big fan of explicit lifecycles and I really like where this is going, but
I'm afraid this would likely collide w/ hiroshige@'s
{Classic,Module}PendingScript support work (which just went in, and got
reverted).

Is this CL purely code-health? or is this CL blocking your future CLs?

[jbroman, 2017-04-24 15:54:43]

On 2017/04/24 at 01:12:52, kouhei wrote:
> +hiroshige,japhet
> I'm a big fan of explicit lifecycles and I really like where this is going,
but I'm afraid this would likely collide w/ hiroshige@'s
{Classic,Module}PendingScript support work (which just went in, and got
reverted).

Yeah, I realize this may cause merge conflicts.

> Is this CL purely code-health? or is this CL blocking your future CLs?

A little of each?

I found my followup CLs easier to write with explicit lifetime management,
so...kinda blocking. But I can wait for a few CLs on the modules stuff to land
if that's helpful. (I'd rather not wait, say, a quarter, though.)

[hiroshige, 2017-04-24 19:16:53]

> I'm a big fan of explicit lifecycles and I really like where this is going
+1!

> I'm afraid this would likely collide w/ hiroshige@'s
> {Classic,Module}PendingScript support work
> But I can wait for a few CLs on the modules stuff to land
> if that's helpful.
Could you wait for [1] to be landed and rebase?
[1] is also blocking my CLs and I'd like to land it within a couple of days.
[1] https://codereview.chromium.org/2653923008/



I think we can split this CL into two parts:
1. Replacing watching_for_load_ with client_ (trivial), and
2. The other parts, i.e. introducing the state machine.
The Part 2 is not trivial and changes the behavior slightly.
e.g. currently IsReady() can return false after finish is notified, but after
this CL that never happens (Issue 692856). I think this behavior change is
probably good (doesn't fix Issue 692856 completely though).

Also, checking states by CHECK() (not DCHECK()) might be better, at least for a
while, as this kind of state machines are relatively fragile, and confirming the
invariants strongly around ScriptLoader/PendingScript is useful for upcoming
changes by us (for modules and cleanups) and you.
(I initially introduced CHECK()s in CheckState() and planned to make them
DCHECK(), but I've changed my mind and am keeping it CHECK() after the CHECK()s
caught two regression bugs on canary)

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:129: DCHECK(GetResource());
DCHECK_EQ(ready_state_, kWaitingForStreaming);

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.h:143: // Advances the current
state of the script, reporting to the client is
nit: reporting to the client *if*?

[hiroshige, 2017-04-28 18:06:22]

On 2017/04/24 19:16:53, hiroshige wrote:
> > I'm a big fan of explicit lifecycles and I really like where this is going
> +1!
> 
> > I'm afraid this would likely collide w/ hiroshige@'s
> > {Classic,Module}PendingScript support work
> > But I can wait for a few CLs on the modules stuff to land
> > if that's helpful.
> Could you wait for [1] to be landed and rebase?
> [1] is also blocking my CLs and I'd like to land it within a couple of days.
> [1] https://codereview.chromium.org/2653923008/
> 
> 
> 
> I think we can split this CL into two parts:
> 1. Replacing watching_for_load_ with client_ (trivial), and
> 2. The other parts, i.e. introducing the state machine.
> The Part 2 is not trivial and changes the behavior slightly.
> e.g. currently IsReady() can return false after finish is notified, but after
> this CL that never happens (Issue 692856). I think this behavior change is
> probably good (doesn't fix Issue 692856 completely though).
> 
> Also, checking states by CHECK() (not DCHECK()) might be better, at least for
a
> while, as this kind of state machines are relatively fragile, and confirming
the
> invariants strongly around ScriptLoader/PendingScript is useful for upcoming
> changes by us (for modules and cleanups) and you.
> (I initially introduced CHECK()s in CheckState() and planned to make them
> DCHECK(), but I've changed my mind and am keeping it CHECK() after the
CHECK()s
> caught two regression bugs on canary)
> 
>
https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):
> 
>
https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/PendingScript.cpp:129:
DCHECK(GetResource());
> DCHECK_EQ(ready_state_, kWaitingForStreaming);
> 
>
https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/PendingScript.h (right):
> 
>
https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/PendingScript.h:143: // Advances the
current
> state of the script, reporting to the client is
> nit: reporting to the client *if*?

My changes have been landed (and not reverted this time), so feel free to
proceed!

[jbroman, 2017-04-28 19:34:57]

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

[commit-bot: I haz the power, 2017-04-28 19:36:03]

Dry run: CQ is trying da patch.

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

[jbroman, 2017-04-28 19:37:16]

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

[commit-bot: I haz the power, 2017-04-28 19:38:16]

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-04-28 21:50:08]

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

[commit-bot: I haz the power, 2017-04-28 21:50:09]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-01 15:08:02]

PTAL.

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.cpp:129: DCHECK(GetResource());
On 2017/04/24 at 19:16:53, hiroshige wrote:
> DCHECK_EQ(ready_state_, kWaitingForStreaming);

Done.

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/2828193003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/PendingScript.h:143: // Advances the current
state of the script, reporting to the client is
On 2017/04/24 at 19:16:53, hiroshige wrote:
> nit: reporting to the client *if*?

Done.

[hiroshige, 2017-05-01 23:43:21]

lgtm!

[kouhei (in TOK), 2017-05-02 02:17:39]

lgtm

[jbroman, 2017-05-02 15:01:17]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2017-05-02 15:02: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, 2017-05-02 16:35:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493737277915430,
"parent_rev": "6b0131a01da5149cd7acb81973f881a0d85fbe59", "commit_rev":
"ce9f0aa85bc1e34ad03dc963ef589dc3fbed21bd"}

[commit-bot: I haz the power, 2017-05-02 16:35:47]

Description was changed from

==========
Explicitly track the lifecycle of PendingScript.

This moves the lifecycle tracking of PendingScript from relying
on querying its parts (the resource, streamer, etc.) to maintaining
an explicit state machine.

Firstly, this means that the apparent state to change is timed to
coincide with when clients are notified (as opposed to a client being
able to notice that a PendingScript is finished before being notified).

It also paves the way for additional steps (e.g. separately scheduled
compile of the script) without making IsReady and ErrorOccurred yet
more complicated.

BUG=591558
==========

to

==========
Explicitly track the lifecycle of PendingScript.

This moves the lifecycle tracking of PendingScript from relying
on querying its parts (the resource, streamer, etc.) to maintaining
an explicit state machine.

Firstly, this means that the apparent state to change is timed to
coincide with when clients are notified (as opposed to a client being
able to notice that a PendingScript is finished before being notified).

It also paves the way for additional steps (e.g. separately scheduled
compile of the script) without making IsReady and ErrorOccurred yet
more complicated.

BUG=591558

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

[commit-bot: I haz the power, 2017-05-02 16:35:48]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ce9f0aa85bc1e34ad03dc963ef58...