Introduce ContextObserver

Introduce ContextObserver

There are many ContextLifecycleObservers that want to use getExecutionContext()
and frame() but don't need contextDestroyed() notifications. Since ContextLifecycleObserver
is heavy, it seems better to introduce a lightweight version of ContextLifecycleObserver
that only provides getExecutionContext() and frame().

This CL introduces ContextObserver and uses it in BarProp.

ContextObserver can also be used in classes that are currently storing Member<ExecutionContext>
or Member<LocalFrame> and using isContextDestroyed() to check if the observing context is alive.

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

[haraken, 2016-12-19 02:07:48]

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

[haraken, 2016-12-19 02:07:49]

PTAL

[haraken, 2016-12-19 02:09:25]

https://codereview.chromium.org/2585143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):

https://codereview.chromium.org/2585143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:60:
Member<ExecutionContext> m_executionContext;

I don't think this needs to be a weak member because it is explicitly cleared
when the context is detached.

Does it probably mean that ContextLifecycleObserver can also use
HeapHashSet<Member<Context>> to track the contexts...?

[haraken, 2016-12-19 05:38:45]

On 2016/12/19 02:09:25, haraken wrote:
>
https://codereview.chromium.org/2585143002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):
> 
>
https://codereview.chromium.org/2585143002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:60:
> Member<ExecutionContext> m_executionContext;
> 
> I don't think this needs to be a weak member because it is explicitly cleared
> when the context is detached.
> 
> Does it probably mean that ContextLifecycleObserver can also use
> HeapHashSet<Member<Context>> to track the contexts...?

I changed it to a WeakMember in PS2. Using a Member will cause memory leaks like
https://codereview.chromium.org/2555873004/

[dcheng, 2016-12-19 07:16:14]

Is there a lot of overhead to having a contextDestroyed()? Since we've been
trying to reduce the number of observer classes, I'm a bit hesitant to add a new
one if the cost isn't too bad.

[haraken, 2016-12-19 07:31:47]

On 2016/12/19 07:16:14, dcheng wrote:
> Is there a lot of overhead to having a contextDestroyed()? Since we've been
> trying to reduce the number of observer classes, I'm a bit hesitant to add a
new
> one if the cost isn't too bad.

I'm trying to reduce # of observers by making the object directly hold
Member<ExecutionContext> & use m_executionContext->isContextDestroyed().
Sigbjorn pointed out that it would be better to have a helper class that does
the work -- which is ContextObserver.

If we use ContextLifecycleObserver, it increases the size of m_observers on
LifecycleNotifier. This becomes a problem if it's used for a class that creates
many objects (e.g., HTMLImageElement caused the problem before).

[sof, 2016-12-19 07:31:57]

On 2016/12/19 07:16:14, dcheng wrote:
> Is there a lot of overhead to having a contextDestroyed()? Since we've been
> trying to reduce the number of observer classes, I'm a bit hesitant to add a
new
> one if the cost isn't too bad.

There's been a recent push to replace ContextLifecycleObserver uses with local
Member<ExecutionContext>s for the cases which don't need contextDestroyed(),
i.e., reduce ContextLifecycleObserver uses.

In some cases that fits ok, in other places it would just require adding code
boilerplate - BarProp & ActiveDOMCallback are two examples. ContextObserver
(which isn't an "observer", arguably) taking care of the details for those.

[haraken, 2016-12-19 08:10:41]

Just to clarify, the goal is to unify everything into ContextLifecycleObserver
and ContextObserver. Code that holds Member<ExecutionContext> or
Member<LocalFrame> just to check isContextDestroyed will be removed (i.e.,
replaced with ContextObserver).

[dcheng, 2016-12-19 08:25:26]

OK, makes sense. I wonder if we can think of a different name than Observer
though: at this point, since it's not really observing anything anymore. Maybe
that would help reduce confusion on how to use it.

[haraken, 2016-12-19 08:34:26]

On 2016/12/19 08:25:26, dcheng wrote:
> OK, makes sense. I wonder if we can think of a different name than Observer
> though: at this point, since it's not really observing anything anymore. Maybe
> that would help reduce confusion on how to use it.

ContextClient ?

[sof, 2016-12-19 09:14:03]

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.cpp (right):

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.cpp:13: :
m_executionContext(frame ? frame->document() : nullptr) {}
Can BarProp be changed to be over Document* instead? then we don't need this
constructor.

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:60:
WeakMember<ExecutionContext> m_executionContext;
For clarity, what motivates the weakness?

[sof, 2016-12-19 09:15:17]

On 2016/12/19 08:34:26, haraken wrote:
> On 2016/12/19 08:25:26, dcheng wrote:
> > OK, makes sense. I wonder if we can think of a different name than Observer
> > though: at this point, since it's not really observing anything anymore.
Maybe
> > that would help reduce confusion on how to use it.
> 
> ContextClient ?

A Client suffix would be consistent - ExecutionContextClient?

[haraken, 2016-12-19 09:58:16]

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.cpp (right):

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.cpp:13: :
m_executionContext(frame ? frame->document() : nullptr) {}
On 2016/12/19 09:14:03, sof wrote:
> Can BarProp be changed to be over Document* instead? then we don't need this
> constructor.

I can, but as far as I grep the code base, there are many classes that need to
retrieve a document from a frame. So it looks better to have a helper.

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):

https://codereview.chromium.org/2585143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:60:
WeakMember<ExecutionContext> m_executionContext;
On 2016/12/19 09:14:03, sof wrote:
> For clarity, what motivates the weakness?

For example, this CL (https://codereview.chromium.org/2555873004/) caused memory
leaks.

Currently it's not guaranteed that contextDestroyed is called for all contexts
e.g., documents created by DOMImplementation::createDocument. So we cannot
assume that m_executionConntext is cleared before the context is detached.

[haraken, 2016-12-19 09:58:57]

On 2016/12/19 09:15:17, sof wrote:
> On 2016/12/19 08:34:26, haraken wrote:
> > On 2016/12/19 08:25:26, dcheng wrote:
> > > OK, makes sense. I wonder if we can think of a different name than
Observer
> > > though: at this point, since it's not really observing anything anymore.
> Maybe
> > > that would help reduce confusion on how to use it.
> > 
> > ContextClient ?
> 
> A Client suffix would be consistent - ExecutionContextClient?

Yeah, but the name needs to be consistent with ContextLifecycleObserver and
contextDestroyed. We could rename them to ExecutionContextLifecycleObserber and
executionContextDestroyed though (if it's not too long).

[haraken, 2016-12-19 12:51:12]

On 2016/12/19 09:58:57, haraken wrote:
> On 2016/12/19 09:15:17, sof wrote:
> > On 2016/12/19 08:34:26, haraken wrote:
> > > On 2016/12/19 08:25:26, dcheng wrote:
> > > > OK, makes sense. I wonder if we can think of a different name than
> Observer
> > > > though: at this point, since it's not really observing anything anymore.
> > Maybe
> > > > that would help reduce confusion on how to use it.
> > > 
> > > ContextClient ?
> > 
> > A Client suffix would be consistent - ExecutionContextClient?
> 
> Yeah, but the name needs to be consistent with ContextLifecycleObserver and
> contextDestroyed. We could rename them to ExecutionContextLifecycleObserber
and
> executionContextDestroyed though (if it's not too long).

Renamed it to ContextClient at the moment. (I don't have any strong opinion on
this.)

[sof, 2016-12-19 13:03:44]

On 2016/12/19 12:51:12, haraken wrote:
> On 2016/12/19 09:58:57, haraken wrote:
> > On 2016/12/19 09:15:17, sof wrote:
> > > On 2016/12/19 08:34:26, haraken wrote:
> > > > On 2016/12/19 08:25:26, dcheng wrote:
> > > > > OK, makes sense. I wonder if we can think of a different name than
> > Observer
> > > > > though: at this point, since it's not really observing anything
anymore.
> > > Maybe
> > > > > that would help reduce confusion on how to use it.
> > > > 
> > > > ContextClient ?
> > > 
> > > A Client suffix would be consistent - ExecutionContextClient?
> > 
> > Yeah, but the name needs to be consistent with ContextLifecycleObserver and
> > contextDestroyed. We could rename them to ExecutionContextLifecycleObserber
> and
> > executionContextDestroyed though (if it's not too long).
> 
> Renamed it to ContextClient at the moment. (I don't have any strong opinion on
> this.)

lgtm from my side.

[dcheng, 2016-12-20 07:20:43]

LGTM with an optional comment nit (my theory is the getters are pretty
straightforward and already documented twice in this header, while
contextDestroyed() is maybe slightly less obvious, since it is inherited from
another header. I don't feel strongly about this though)

https://codereview.chromium.org/2585143002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):

https://codereview.chromium.org/2585143002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:39: // If you just
want to use getExecutionContext() and frame(), use
Nit: I've heard some people prefer to avoid pronouns in comments.

One alternative wording:
ContextClient and ContextLifecycleObserver are helpers to associate a
class with an ExecutionContext. ContextLifecycleObserver provides an
additional contextDestroyed() hook to execute cleanup code when a
context is destroyed. Prefer the simpler ContextClient when possible.

[haraken, 2016-12-21 01:18:06]

On 2016/12/20 07:20:43, dcheng wrote:
> LGTM with an optional comment nit (my theory is the getters are pretty
> straightforward and already documented twice in this header, while
> contextDestroyed() is maybe slightly less obvious, since it is inherited from
> another header. I don't feel strongly about this though)
> 
>
https://codereview.chromium.org/2585143002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h (right):
> 
>
https://codereview.chromium.org/2585143002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/ContextLifecycleObserver.h:39: // If you
just
> want to use getExecutionContext() and frame(), use
> Nit: I've heard some people prefer to avoid pronouns in comments.
> 
> One alternative wording:
> ContextClient and ContextLifecycleObserver are helpers to associate a
> class with an ExecutionContext. ContextLifecycleObserver provides an
> additional contextDestroyed() hook to execute cleanup code when a
> context is destroyed. Prefer the simpler ContextClient when possible.

Done.

[haraken, 2016-12-21 01:18:10]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-21 01:18:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2585143002/#ps60001
(title: "temp")

[commit-bot: I haz the power, 2016-12-21 01:18:59]

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-21 03:03:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482283090895260,
"parent_rev": "856dc2a596ed6d25391355f211ee04b0d58c64f5", "commit_rev":
"09514b02d29553d3fedec22dfe395440d6b887e1"}

[commit-bot: I haz the power, 2016-12-21 03:04:16]

Description was changed from

==========
Introduce ContextObserver

There are many ContextLifecycleObservers that want to use getExecutionContext()
and frame() but don't need contextDestroyed() notifications. Since
ContextLifecycleObserver
is heavy, it seems better to introduce a lightweight version of
ContextLifecycleObserver
that only provides getExecutionContext() and frame().

This CL introduces ContextObserver and uses it in BarProp.

ContextObserver can also be used in classes that are currently storing
Member<ExecutionContext>
or Member<LocalFrame> and using isContextDestroyed() to check if the observing
context is alive.

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

to

==========
Introduce ContextObserver

There are many ContextLifecycleObservers that want to use getExecutionContext()
and frame() but don't need contextDestroyed() notifications. Since
ContextLifecycleObserver
is heavy, it seems better to introduce a lightweight version of
ContextLifecycleObserver
that only provides getExecutionContext() and frame().

This CL introduces ContextObserver and uses it in BarProp.

ContextObserver can also be used in classes that are currently storing
Member<ExecutionContext>
or Member<LocalFrame> and using isContextDestroyed() to check if the observing
context is alive.

BUG=610176

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

[commit-bot: I haz the power, 2016-12-21 03:04:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-21 03:07:37]

Description was changed from

==========
Introduce ContextObserver

There are many ContextLifecycleObservers that want to use getExecutionContext()
and frame() but don't need contextDestroyed() notifications. Since
ContextLifecycleObserver
is heavy, it seems better to introduce a lightweight version of
ContextLifecycleObserver
that only provides getExecutionContext() and frame().

This CL introduces ContextObserver and uses it in BarProp.

ContextObserver can also be used in classes that are currently storing
Member<ExecutionContext>
or Member<LocalFrame> and using isContextDestroyed() to check if the observing
context is alive.

BUG=610176

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

to

==========
Introduce ContextObserver

There are many ContextLifecycleObservers that want to use getExecutionContext()
and frame() but don't need contextDestroyed() notifications. Since
ContextLifecycleObserver
is heavy, it seems better to introduce a lightweight version of
ContextLifecycleObserver
that only provides getExecutionContext() and frame().

This CL introduces ContextObserver and uses it in BarProp.

ContextObserver can also be used in classes that are currently storing
Member<ExecutionContext>
or Member<LocalFrame> and using isContextDestroyed() to check if the observing
context is alive.

BUG=610176

Committed: https://crrev.com/e873a8e81ec1427467406cbb47293afc35dc1e16
Cr-Commit-Position: refs/heads/master@{#439990}
==========

[commit-bot: I haz the power, 2016-12-21 03:07:38]

Patchset 4 (id:??) landed as
https://crrev.com/e873a8e81ec1427467406cbb47293afc35dc1e16
Cr-Commit-Position: refs/heads/master@{#439990}