Oilpan: move WebPluginLoadObserver to the heap.

Oilpan: move WebPluginLoadObserver to the heap.

R=haraken
BUG=553613
Committed: https://crrev.com/4bfd8504c631d5aa6b06e53e7c09a8078242faf6
Cr-Commit-Position: refs/heads/master@{#359556}

[sof, 2015-11-12 15:17:54]

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

[sof, 2015-11-12 15:17:54]

please take a look.

this is ready for consideration, but cannot be landed until there's webkit_tests
oilpan trybot coverage again.

[haraken, 2015-11-12 15:50:59]

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

[haraken, 2015-11-12 15:51:00]

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:476:
ScopedDisposal<WebPluginLoadObserver> disposeScope(observer.get());

Help me understand: Is there any reason we need to promptly call dispose()?
Wouldn't pre-finalizers be enough?

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:44: return
nextPluginLoadObserver;

Nit: I remember we discussed before that the nextPluginLoadObserver is super
nasty...

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPluginLoadObserver.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPluginLoadObserver.cpp:41: #if !ENABLE(OILPAN)

Can we add ASSERT(!m_pluginContainer) to ENABLE(OILPAN)?

[sof, 2015-11-12 18:15:35]

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:476:
ScopedDisposal<WebPluginLoadObserver> disposeScope(observer.get());
On 2015/11/12 15:51:00, haraken wrote:
> 
> Help me understand: Is there any reason we need to promptly call dispose()?
> Wouldn't pre-finalizers be enough?

I just wanted to emulate what happens !OILPAN, which is possible without much
incident here. Having them be GCed and swept out later would also work.

Actually, thinking about this some more, we can do a lot better with Oilpan --
there's no need for WebPluginContainerImpl to track the load observers. The only
purpose of doing is to be able to invalidate the weak references that
WebPluginLoadObservers have when WebPluginContainerImpl is finalized. We already
have mechanisms for that with Oilpan..WeakMember<>. Updated to do so instead.

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:44: return
nextPluginLoadObserver;
On 2015/11/12 15:51:00, haraken wrote:
> 
> Nit: I remember we discussed before that the nextPluginLoadObserver is super
> nasty...

The code using it in WebPluginContainerImpl::loadFrameRequest() does have a
FIXME acknowledging its dubious nature :)

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebPluginLoadObserver.cpp (right):

https://codereview.chromium.org/1442643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebPluginLoadObserver.cpp:41: #if !ENABLE(OILPAN)
On 2015/11/12 15:51:00, haraken wrote:
> 
> Can we add ASSERT(!m_pluginContainer) to ENABLE(OILPAN)?

Don't think so; with this and WebPluginContainerImpl dying at the same time, no
weak processing happens.

[haraken, 2015-11-13 13:12:11]

Looks much better! LGTM.

[sof, 2015-11-13 14:56:16]

Description was changed from

==========
Oilpan: move WebPluginLoadObserver to the heap.

R=
BUG=553613
==========

to

==========
Oilpan: move WebPluginLoadObserver to the heap.

R=haraken
BUG=553613
==========

[sof, 2015-11-13 14:57:31]

The plugins/ layout tests pass locally on Windows w/Oilpan, so will attempt to
land this without trybot coverage.

[sof, 2015-11-13 14:57:39]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-13 14:57:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1442643003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1442643003/80001

[commit-bot: I haz the power, 2015-11-13 15:57:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-13 15:57:45]

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

[sof, 2015-11-19 06:19:29]

On 2015/11/13 13:12:11, haraken wrote:
> Looks much better! LGTM.

Even better still, https://codereview.chromium.org/1426923007/ :-)

[haraken, 2015-11-19 07:53:06]

On 2015/11/19 06:19:29, sof wrote:
> On 2015/11/13 13:12:11, haraken wrote:
> > Looks much better! LGTM.
> 
> Even better still, https://codereview.chromium.org/1426923007/ :-)

Awesome!