Oilpan: Build fix after r170454

Oilpan: Build fix after r170454

r170454 was almost Oilpan-ready, add missing bits.

R=mvanouwerkerk@chromium.org
BUG=
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170475

[sof, 2014-03-31 18:35:40]

Please take a look.

[Michael van Ouwerkerk, 2014-03-31 18:53:43]

On 2014/03/31 18:35:40, sof wrote:
> Please take a look.

My apologies. lgtm

[sof, 2014-03-31 19:01:24]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-31 19:01:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/219753004/1

[commit-bot: I haz the power, 2014-03-31 19:01:44]

Change committed as 170475

[Peter Beverloo, 2014-03-31 19:08:07]

Thanks for the follow-up, Sigbjorn! Sorry for missing this in the review.

https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:18: class PushManager FINAL : public
RefCountedWillBeGarbageCollectedFinalized<PushManager>, public ScriptWrappable {
Could you confirm my understanding of this change?

Very similar classes, e.g. Notification and BatteryManager, are
RefCountedWillBeRefCountedGarbageCollected, but they have to be given that
events can be fired on them, and they thus have to be kept alive.  This is not
the case for PushManager, which is not an EventTarget, and can therefore be GCed
the moment a call to navigator.push.register() (or other future methods) is
done. Therefore the reference counting is not needed, and we can rely on just
GC.

[sof, 2014-03-31 19:08:56]

On 2014/03/31 18:53:43, Michael van Ouwerkerk wrote:
> On 2014/03/31 18:35:40, sof wrote:
> > Please take a look.
> 
> My apologies. lgtm

The Oilpan team is currently responsible for keeping the Oilpan bots
happy&green, so nothing to worry about here :)

(If you're unsure about Oilpan details, just Cc: or add
oilpan-reviews@chromium.org as reviewers.)

[sof, 2014-03-31 19:32:39]

https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:18: class PushManager FINAL : public
RefCountedWillBeGarbageCollectedFinalized<PushManager>, public ScriptWrappable {
On 2014/03/31 19:08:07, Peter Beverloo wrote:
> Could you confirm my understanding of this change?
> 
> Very similar classes, e.g. Notification and BatteryManager, are
> RefCountedWillBeRefCountedGarbageCollected, but they have to be given that
> events can be fired on them, and they thus have to be kept alive.  This is not
> the case for PushManager, which is not an EventTarget, and can therefore be
GCed
> the moment a call to navigator.push.register() (or other future methods) is
> done. Therefore the reference counting is not needed, and we can rely on just
> GC.

Yes, an event target will have to be kept alive for as long as there are
listeners attached (which aren't heap allocated, so a ref count is separately
kept.)

As you say, there are no heap-external PushManager references, so
*RefCountedGarbageCollected is not needed for it, *GarbageCollectedFinalized
suffices. If it sprouts events later, we can add it then.

Do notice that the Navigator supplementable will trace&keep NavigatorPushManager
(a supplement of it) alive. Which again will keep PushManager alive. So, the
push manager&supplement will stay around for as long as the Navigator object
does.

[sof, 2014-03-31 20:33:44]

On 2014/03/31 19:32:39, sof wrote:
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> File Source/modules/push_messaging/PushManager.h (right):
> 
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> Source/modules/push_messaging/PushManager.h:18: class PushManager FINAL :
public
> RefCountedWillBeGarbageCollectedFinalized<PushManager>, public ScriptWrappable
{
> On 2014/03/31 19:08:07, Peter Beverloo wrote:
> > Could you confirm my understanding of this change?
> > 
> > Very similar classes, e.g. Notification and BatteryManager, are
> > RefCountedWillBeRefCountedGarbageCollected, but they have to be given that
> > events can be fired on them, and they thus have to be kept alive.  This is
not
> > the case for PushManager, which is not an EventTarget, and can therefore be
> GCed
> > the moment a call to navigator.push.register() (or other future methods) is
> > done. Therefore the reference counting is not needed, and we can rely on
just
> > GC.
> 
> Yes, an event target will have to be kept alive for as long as there are
> listeners attached (which aren't heap allocated, so a ref count is separately
> kept.)
> 
> As you say, there are no heap-external PushManager references, so
> *RefCountedGarbageCollected is not needed for it, *GarbageCollectedFinalized
> suffices. If it sprouts events later, we can add it then.
> 
> Do notice that the Navigator supplementable will trace&keep
NavigatorPushManager
> (a supplement of it) alive. Which again will keep PushManager alive. So, the
> push manager&supplement will stay around for as long as the Navigator object
> does.

It doesn't do harm per se, but it might be worth thinking about GC clang plugin
support for detecting unnecessary uses of RefCountedGarbageCollected & reporting
those as warnings.

[haraken, 2014-03-31 23:04:33]

LGTM, thanks for the quick fix.

[zerny-chromium, 2014-04-01 08:55:49]

On 2014/03/31 20:33:44, sof wrote:
> On 2014/03/31 19:32:39, sof wrote:
> >
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> > File Source/modules/push_messaging/PushManager.h (right):
> > 
> >
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> > Source/modules/push_messaging/PushManager.h:18: class PushManager FINAL :
> public
> > RefCountedWillBeGarbageCollectedFinalized<PushManager>, public
ScriptWrappable
> {
> > On 2014/03/31 19:08:07, Peter Beverloo wrote:
> > > Could you confirm my understanding of this change?
> > > 
> > > Very similar classes, e.g. Notification and BatteryManager, are
> > > RefCountedWillBeRefCountedGarbageCollected, but they have to be given that
> > > events can be fired on them, and they thus have to be kept alive.  This is
> not
> > > the case for PushManager, which is not an EventTarget, and can therefore
be
> > GCed
> > > the moment a call to navigator.push.register() (or other future methods)
is
> > > done. Therefore the reference counting is not needed, and we can rely on
> just
> > > GC.
> > 
> > Yes, an event target will have to be kept alive for as long as there are
> > listeners attached (which aren't heap allocated, so a ref count is
separately
> > kept.)
> > 
> > As you say, there are no heap-external PushManager references, so
> > *RefCountedGarbageCollected is not needed for it, *GarbageCollectedFinalized
> > suffices. If it sprouts events later, we can add it then.
> > 
> > Do notice that the Navigator supplementable will trace&keep
> NavigatorPushManager
> > (a supplement of it) alive. Which again will keep PushManager alive. So, the
> > push manager&supplement will stay around for as long as the Navigator object
> > does.
> 
> It doesn't do harm per se, but it might be worth thinking about GC clang
plugin
> support for detecting unnecessary uses of RefCountedGarbageCollected &
reporting
> those as warnings.

I don't think we can add compile-time checks for this as the need for ref
counting is not always locally determined. It is possible to have eliminated all
uses of RefPtr to a type but some part of the hierarchy might have a mixin (eg,
EventTarget or ActiveDOMObject) which still requires ref/deref.

I've added some stats collecting to the process-graph.py plugin script which can
makes post-compilation checks of the graph. Running it will allow us to identify
cases where the ref counting can be removed. (Current ToT has no such cases)

[sof, 2014-04-01 11:46:26]

On 2014/04/01 08:55:49, zerny-chromium wrote:
> On 2014/03/31 20:33:44, sof wrote:
> > On 2014/03/31 19:32:39, sof wrote:
> > >
> >
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> > > File Source/modules/push_messaging/PushManager.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/219753004/diff/1/Source/modules/push_messagin...
> > > Source/modules/push_messaging/PushManager.h:18: class PushManager FINAL :
> > public
> > > RefCountedWillBeGarbageCollectedFinalized<PushManager>, public
> ScriptWrappable
> > {
> > > On 2014/03/31 19:08:07, Peter Beverloo wrote:
> > > > Could you confirm my understanding of this change?
> > > > 
> > > > Very similar classes, e.g. Notification and BatteryManager, are
> > > > RefCountedWillBeRefCountedGarbageCollected, but they have to be given
that
> > > > events can be fired on them, and they thus have to be kept alive.  This
is
> > not
> > > > the case for PushManager, which is not an EventTarget, and can therefore
> be
> > > GCed
> > > > the moment a call to navigator.push.register() (or other future methods)
> is
> > > > done. Therefore the reference counting is not needed, and we can rely on
> > just
> > > > GC.
> > > 
> > > Yes, an event target will have to be kept alive for as long as there are
> > > listeners attached (which aren't heap allocated, so a ref count is
> separately
> > > kept.)
> > > 
> > > As you say, there are no heap-external PushManager references, so
> > > *RefCountedGarbageCollected is not needed for it,
*GarbageCollectedFinalized
> > > suffices. If it sprouts events later, we can add it then.
> > > 
> > > Do notice that the Navigator supplementable will trace&keep
> > NavigatorPushManager
> > > (a supplement of it) alive. Which again will keep PushManager alive. So,
the
> > > push manager&supplement will stay around for as long as the Navigator
object
> > > does.
> > 
> > It doesn't do harm per se, but it might be worth thinking about GC clang
> plugin
> > support for detecting unnecessary uses of RefCountedGarbageCollected &
> reporting
> > those as warnings.
> 
> I don't think we can add compile-time checks for this as the need for ref
> counting is not always locally determined. It is possible to have eliminated
all
> uses of RefPtr to a type but some part of the hierarchy might have a mixin
(eg,
> EventTarget or ActiveDOMObject) which still requires ref/deref.
> 

Yes, it's not a local property. But I wonder how many false positives there
would be for a simple check (!= RefCountedGarbageCollected) for classes that
derive from ScriptWrappable only. i.e., catching the case here of adapting
a class declaration that also derived from EventTarget.

Just a thought.

> I've added some stats collecting to the process-graph.py plugin script which
can
> makes post-compilation checks of the graph. Running it will allow us to
identify
> cases where the ref counting can be removed. (Current ToT has no such cases)

That's quite awesome.