Remove ContextLifecycleObserver from V8V0CustomElementLifecycleCallbacks

Remove ContextLifecycleObserver from V8V0CustomElementLifecycleCallbacks

It's not needed because V8V0CustomElementLifecycleCallbacks already holds
ScriptState.

BUG=610176
Committed: https://crrev.com/12d08d987d13508e34d9abbd01f062927f2eee90
Cr-Commit-Position: refs/heads/master@{#436960}

[haraken, 2016-12-07 11:48:03]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-12-07 11:48:03]

PTAL

[sof, 2016-12-07 12:14:59]

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
(left):

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp:154:
if (!getExecutionContext() || getExecutionContext()->isContextDestroyed())
Can you clarify why the explicit checks here for isContextDestroyed() are no
longer needed?

[haraken, 2016-12-07 12:17:21]

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
(left):

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp:154:
if (!getExecutionContext() || getExecutionContext()->isContextDestroyed())
On 2016/12/07 12:14:59, sof wrote:
> Can you clarify why the explicit checks here for isContextDestroyed() are no
> longer needed?

Because it's equivalent to the m_scriptState->contextIsValid() below. Both start
returning false after the context gets detached.

[sof, 2016-12-07 12:25:14]

lgtm

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
(left):

https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp:154:
if (!getExecutionContext() || getExecutionContext()->isContextDestroyed())
On 2016/12/07 12:17:21, haraken wrote:
> On 2016/12/07 12:14:59, sof wrote:
> > Can you clarify why the explicit checks here for isContextDestroyed() are no
> > longer needed?
> 
> Because it's equivalent to the m_scriptState->contextIsValid() below. Both
start
> returning false after the context gets detached.

ok, if the interval between context() becoming null and notifyContextDestroyed()
is called, isn't observable, then this is fine.

[haraken, 2016-12-07 12:50:56]

On 2016/12/07 12:25:14, sof wrote:
> lgtm
> 
>
https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
> File
>
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp
> (left):
> 
>
https://codereview.chromium.org/2561573002/diff/1/third_party/WebKit/Source/b...
>
third_party/WebKit/Source/bindings/core/v8/V8V0CustomElementLifecycleCallbacks.cpp:154:
> if (!getExecutionContext() || getExecutionContext()->isContextDestroyed())
> On 2016/12/07 12:17:21, haraken wrote:
> > On 2016/12/07 12:14:59, sof wrote:
> > > Can you clarify why the explicit checks here for isContextDestroyed() are
no
> > > longer needed?
> > 
> > Because it's equivalent to the m_scriptState->contextIsValid() below. Both
> start
> > returning false after the context gets detached.
> 
> ok, if the interval between context() becoming null and
notifyContextDestroyed()
> is called, isn't observable, then this is fine.

Yeah, good point. We're already assuming in many places that
!scriptState->contextIsValid() and executionContext->isContextDestroyed() are
equivalent, but it's not that clear from the code base (although it looks mostly
correct). Maybe we should make ScriptState inherit from ContextLifecycleObserver
so that scriptState->contextIsValid() can return false at the same timing as
executionContext->isContextDestroyed(). I'll put it on my TODO.

[haraken, 2016-12-07 12:50:59]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-07 12:51:28]

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-12-07 15:20:54]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1481115059729840, "parent_rev":
"3d7e184136e8ee7b08cd714dad3ad013814bfc88", "commit_rev":
"b4b1ab2c9e18f78a71c39dd935d6e1a90a7b6e8e"}

[commit-bot: I haz the power, 2016-12-07 15:21:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-07 15:22:57]

Description was changed from

==========
Remove ContextLifecycleObserver from V8V0CustomElementLifecycleCallbacks

It's not needed because V8V0CustomElementLifecycleCallbacks already holds
ScriptState.

BUG=610176
==========

to

==========
Remove ContextLifecycleObserver from V8V0CustomElementLifecycleCallbacks

It's not needed because V8V0CustomElementLifecycleCallbacks already holds
ScriptState.

BUG=610176

Committed: https://crrev.com/12d08d987d13508e34d9abbd01f062927f2eee90
Cr-Commit-Position: refs/heads/master@{#436960}
==========

[commit-bot: I haz the power, 2016-12-07 15:22:58]

Patchset 1 (id:??) landed as
https://crrev.com/12d08d987d13508e34d9abbd01f062927f2eee90
Cr-Commit-Position: refs/heads/master@{#436960}