Simplify lifecycle notifiers and observers.

Simplify lifecycle notifiers and observers.

The following changes are made to the implementation of 
lifecycle notifiers and observers:

 - Avoid keeping duplicate hash sets of registered observers.

 - Let the C++ type system handle type safety of the observer
   set, letting go of the notion of an 'observer type' which
   was dynamically checked for.

 - Extend parameterization of LifecycleNotifier<> and
   LifecycleObserver<> to more closely couple the notifier
   and observer pairs. This allows the retirement of the
   ad-hoc function templates {un}observeContext().

 - Devirtualize the add/removeObserver() notifier methods.

R=haraken
BUG=462949
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191062

[sof, 2015-02-28 18:47:12]

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

[sof, 2015-02-28 18:47:12]

Please take a look

(No rush; suggestions for other folks it'd be good to ask for feedback here?)

[haraken, 2015-03-01 09:18:40]

This is a great clean-up! LGTM.

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
File Source/core/dom/ContextLifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
Source/core/dom/ContextLifecycleNotifier.h:51: const ActiveDOMObjectSet&
activeDOMObjects() const { return m_activeDOMObjects; }

Why do we need to keep m_activeDOMObjects while we can remove other hash sets?

https://codereview.chromium.org/968633002/diff/1/Source/platform/LifecycleNot...
File Source/platform/LifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/platform/LifecycleNot...
Source/platform/LifecycleNotifier.h:70: IteratingNone,

It looks like IteratingNone is unused.

[sof, 2015-03-01 10:00:39]

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
File Source/core/dom/ContextLifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
Source/core/dom/ContextLifecycleNotifier.h:51: const ActiveDOMObjectSet&
activeDOMObjects() const { return m_activeDOMObjects; }
On 2015/03/01 09:18:39, haraken wrote:
> 
> Why do we need to keep m_activeDOMObjects while we can remove other hash sets?

We can remove it also if we accept iterating over the entire set of observers
when evaluating hasPendingActivity() -- there are a number of
non-ActiveDOMObject ExecutionContext observers -- which I didn't feel
comfortable doing right here. We can evaluate doing that in a followup?

[haraken, 2015-03-01 10:47:56]

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
File Source/core/dom/ContextLifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
Source/core/dom/ContextLifecycleNotifier.h:51: const ActiveDOMObjectSet&
activeDOMObjects() const { return m_activeDOMObjects; }
On 2015/03/01 10:00:39, sof wrote:
> On 2015/03/01 09:18:39, haraken wrote:
> > 
> > Why do we need to keep m_activeDOMObjects while we can remove other hash
sets?
> 
> We can remove it also if we accept iterating over the entire set of observers
> when evaluating hasPendingActivity() -- there are a number of
> non-ActiveDOMObject ExecutionContext observers -- which I didn't feel
> comfortable doing right here. We can evaluate doing that in a followup?

Sure. Also I'm wondering if a right fix would be to move hasPendingActivity() to
ScriptWrappable because hasPendingActivity() is a way to keep wrappers alive.

[sof, 2015-03-01 17:21:37]

Thanks for the review.

The lifetime objects are of a delicate nature, so I've created a separate bug to
track landing this change on trunk; http://crbug.com/462949

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
File Source/core/dom/ContextLifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/core/dom/ContextLifec...
Source/core/dom/ContextLifecycleNotifier.h:51: const ActiveDOMObjectSet&
activeDOMObjects() const { return m_activeDOMObjects; }
On 2015/03/01 10:47:56, haraken wrote:
> On 2015/03/01 10:00:39, sof wrote:
> > On 2015/03/01 09:18:39, haraken wrote:
> > > 
> > > Why do we need to keep m_activeDOMObjects while we can remove other hash
> sets?
> > 
> > We can remove it also if we accept iterating over the entire set of
observers
> > when evaluating hasPendingActivity() -- there are a number of
> > non-ActiveDOMObject ExecutionContext observers -- which I didn't feel
> > comfortable doing right here. We can evaluate doing that in a followup?
> 
> Sure. Also I'm wondering if a right fix would be to move hasPendingActivity()
to
> ScriptWrappable because hasPendingActivity() is a way to keep wrappers alive.

Once this change has proven itself and settled well, more can be done.

https://codereview.chromium.org/968633002/diff/1/Source/platform/LifecycleNot...
File Source/platform/LifecycleNotifier.h (right):

https://codereview.chromium.org/968633002/diff/1/Source/platform/LifecycleNot...
Source/platform/LifecycleNotifier.h:70: IteratingNone,
On 2015/03/01 09:18:39, haraken wrote:
> 
> It looks like IteratingNone is unused.

It represents the default no-current-iteration state.

[sof, 2015-03-01 17:22:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-01 17:28:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/968633002/1

[commit-bot: I haz the power, 2015-03-01 17:31:50]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191062

[sof, 2015-03-01 17:50:20]

https://codereview.chromium.org/968633002/diff/1/Source/core/frame/DOMWindowL...
File Source/core/frame/DOMWindowLifecycleNotifier.cpp (right):

https://codereview.chromium.org/968633002/diff/1/Source/core/frame/DOMWindowL...
Source/core/frame/DOMWindowLifecycleNotifier.cpp:60:
static_cast<DOMWindowLifecycleObserver*>(observer)->didRemoveEventListener(window,
eventType);
Will tidy up this redundant cast.