Prefer using ContextClient over ContextLifecycleObserver

Prefer using ContextClient over ContextLifecycleObserver

ContextClient is more lightweight than ContextLifecycleObserver.
Classes that don't override contextDestroyed should use ContextClient.

BUG=610176
Committed: https://crrev.com/46dcc19eebc7a63cee5b9aeb4e1284fd5f9adf6d
Cr-Commit-Position: refs/heads/master@{#440822}

[haraken, 2016-12-27 12:12:06]

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

[haraken, 2016-12-27 12:12:06]

PTAL

[dcheng, 2016-12-27 19:07:24]

lgtm

[haraken, 2016-12-28 00:59:47]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-28 01:00:00]

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-28 02:25:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1482886787295930, "parent_rev":
"5fed5297359b6f33e528f712a9b5dcbca0743ca5", "commit_rev":
"b6c1479a68e1b06eca199e9b6805bb62eb44a3bb"}

[commit-bot: I haz the power, 2016-12-28 02:26:08]

Description was changed from

==========
Prefer using ContextClient over ContextLifecycleObserver

ContextClient is more lightweight than ContextLifecycleObserver.
Classes that don't override contextDestroyed should use ContextClient.

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

to

==========
Prefer using ContextClient over ContextLifecycleObserver

ContextClient is more lightweight than ContextLifecycleObserver.
Classes that don't override contextDestroyed should use ContextClient.

BUG=610176

Review-Url: https://codereview.chromium.org/2607533002
==========

[commit-bot: I haz the power, 2016-12-28 02:26:09]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2017-01-02 15:48:16]

Description was changed from

==========
Prefer using ContextClient over ContextLifecycleObserver

ContextClient is more lightweight than ContextLifecycleObserver.
Classes that don't override contextDestroyed should use ContextClient.

BUG=610176

Review-Url: https://codereview.chromium.org/2607533002
==========

to

==========
Prefer using ContextClient over ContextLifecycleObserver

ContextClient is more lightweight than ContextLifecycleObserver.
Classes that don't override contextDestroyed should use ContextClient.

BUG=610176

Committed: https://crrev.com/46dcc19eebc7a63cee5b9aeb4e1284fd5f9adf6d
Cr-Commit-Position: refs/heads/master@{#440822}
==========

[commit-bot: I haz the power, 2017-01-02 15:48:17]

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

[sof, 2017-01-05 15:12:07]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-01-05 15:12:08]

https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:100: :
ContextClient(scriptState->getExecutionContext()),
This one is flakily repro'ing on the bots,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_11_...

who ensures that ExecutionContext::notifyContextDestroyed() is always called in
a worker setting?

[haraken, 2017-01-05 15:52:02]

I'll revert the CL at the moment.

On 2017/01/05 15:12:08, sof wrote:
>
https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):
> 
>
https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:100: :
> ContextClient(scriptState->getExecutionContext()),
> This one is flakily repro'ing on the bots,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_11_...
> 
> who ensures that ExecutionContext::notifyContextDestroyed() is always called
in
> a worker setting?

I was assuming that WorkerThread::prepareForShutdownOnWorkerThread is
unconditionally called (even if the worker is forcibly terminated) and it calls
notifyContextDestroyed(). Hmm.

[haraken, 2017-01-05 15:52:44]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2618753002/ by haraken@chromium.org.

The reason for reverting is: This CL caused a flaky failure:

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_11_...
.

[haraken, 2017-01-13 09:46:30]

On 2017/01/05 15:12:08, sof wrote:
>
https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):
> 
>
https://codereview.chromium.org/2607533002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:100: :
> ContextClient(scriptState->getExecutionContext()),
> This one is flakily repro'ing on the bots,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_11_...
> 
> who ensures that ExecutionContext::notifyContextDestroyed() is always called
in
> a worker setting?

I cannot reproduce the flaky failure, but I suspect that it's related to the
following difference:

- ContextLifecycleObserver::getExecutionContext returns a valid context while
notifyContextDestroyed is being called.
- ContextClient::getExecutionContext returns nullptr while
notifyContextDestroyed is being called.

Let me investigate what I can do there.