Simplify handling of LocalFrame lifecycle notifications.

Simplify handling of LocalFrame lifecycle notifications.

LocalFrame provides a registration interface for observers to observe
its detachment and destruction, by way of FrameDestructionObserver.

Just like the derived objects of LifecycleNotifier<> do, hence it makes
sense for LocalFrame to also use that interface and implementation. Done
so here by introducing LocalFrameLifecycleNotifier<>.

On the observer side, the FrameDestructionObserver interface now derives
from LifecycleObserver<>, reusing its implementation. Instead of
canonicalizing its name as LocalFrameLifecycleObserver also, keep the
established FrameDestructionObserver.

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

[sof, 2015-03-19 22:05:53]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[sof, 2015-03-19 22:05:54]

please take a look.

i _think_ this is the final Lifecycle* related change for the time being. i.e.,
now that the observer maps are on the heap, we can simplify LocalFrame's
observer tracking.

[haraken, 2015-03-19 23:42:00]

Thanks for doing this! LGTM.

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameDestructionObserver.h (right):

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameDestructionObserver.h:38: class FrameDestructionObserver
: public LifecycleObserver<LocalFrame, FrameDestructionObserver,
LocalFrameLifecycleNotifier> {

Would it make sense to rename FrameDestructionObserver to
LocalFrameLifecycleObserver (for consistency with other observers)? A separate
CL is fine.

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (left):

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:295: m_destructionObservers.clear();

We lose this logic in the new code, but it would be OK.

https://codereview.chromium.org/1017313002/diff/100001/Source/core/page/PageL...
File Source/core/page/PageLifecycleObserver.cpp (right):

https://codereview.chromium.org/1017313002/diff/100001/Source/core/page/PageL...
Source/core/page/PageLifecycleObserver.cpp:32:
PageLifecycleObserver::PageLifecycleObserver(Page* page)

Nit: Can we probably inline this method and remove this file?

[sof, 2015-03-20 06:22:05]

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameDestructionObserver.h (right):

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameDestructionObserver.h:38: class FrameDestructionObserver
: public LifecycleObserver<LocalFrame, FrameDestructionObserver,
LocalFrameLifecycleNotifier> {
On 2015/03/19 23:42:00, haraken wrote:
> 
> Would it make sense to rename FrameDestructionObserver to
> LocalFrameLifecycleObserver (for consistency with other observers)? A separate
> CL is fine.

I think it makes sense to be consistent, but will have to check if we should
align naming if RemoteFrame will need something similar.

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (left):

https://codereview.chromium.org/1017313002/diff/100001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:295: m_destructionObservers.clear();
On 2015/03/19 23:42:00, haraken wrote:
> 
> We lose this logic in the new code, but it would be OK.

That was what I thought too, but as observers call setContext(nullptr) upon
contextDestroyed(), they're all removing themselves. On trunk and with this CL.

iow, this is a no-op clear() call. They _do_ have to call setContext() to make
frame()'s lifetime be observably the same Oilpan and non-Oilpan.

[sof, 2015-03-20 07:30:52]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-20 07:31:01]

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

[sof, 2015-03-20 07:31:42]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-03-20 07:31:47]

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

[commit-bot: I haz the power, 2015-03-20 07:34:55]

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