Oilpan: Move [WillBeGarbageCollected] from Node to EventTarget.

Oilpan: Move [WillBeGarbageCollected] from Node to EventTarget.

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

[haraken, 2014-06-02 07:25:05]

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerContainer.h:56: public
RefCountedWillBeRefCountedGarbageCollected<ServiceWorkerContainer>,

I'm curious why ServiceWorkerContainer needs to become
RefCountedGarbageCollected. Does it derive EventTarget?

https://codereview.chromium.org/302263004/diff/1/Source/modules/webmidi/MIDIC...
File Source/modules/webmidi/MIDIConnectionEvent.h (right):

https://codereview.chromium.org/302263004/diff/1/Source/modules/webmidi/MIDIC...
Source/modules/webmidi/MIDIConnectionEvent.h:65: RefPtrWillBeRawPtr<MIDIPort>
port() { return m_port; }

Not related to your CL, this should be PassRefPtrWillBeRawPtr.

[zerny-chromium, 2014-06-02 07:47:32]

Thanks for the review.

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerContainer.h:56: public
RefCountedWillBeRefCountedGarbageCollected<ServiceWorkerContainer>,
On 2014/06/02 07:25:06, haraken wrote:
> 
> I'm curious why ServiceWorkerContainer needs to become
> RefCountedGarbageCollected. Does it derive EventTarget?

Good point. The IDL implied change is that controller must return a RawPtr in
the oilpan build. I'll do just that and leave the class itself (including the
RefPtr<EventTarget>) as is. That will change in a follow-up CL, but I guess we
could use a Persistent<EventTarget> in that case.

https://codereview.chromium.org/302263004/diff/1/Source/modules/webmidi/MIDIC...
File Source/modules/webmidi/MIDIConnectionEvent.h (right):

https://codereview.chromium.org/302263004/diff/1/Source/modules/webmidi/MIDIC...
Source/modules/webmidi/MIDIConnectionEvent.h:65: RefPtrWillBeRawPtr<MIDIPort>
port() { return m_port; }
On 2014/06/02 07:25:06, haraken wrote:
> 
> Not related to your CL, this should be PassRefPtrWillBeRawPtr.

Done.

[sof, 2014-06-02 07:50:53]

Have you run run-bindings-test and verified that the expected outputs did not
change?

[haraken, 2014-06-02 07:54:32]

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/302263004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerContainer.h:56: public
RefCountedWillBeRefCountedGarbageCollected<ServiceWorkerContainer>,
On 2014/06/02 07:47:33, zerny-chromium wrote:
> On 2014/06/02 07:25:06, haraken wrote:
> > 
> > I'm curious why ServiceWorkerContainer needs to become
> > RefCountedGarbageCollected. Does it derive EventTarget?
> 
> Good point. The IDL implied change is that controller must return a RawPtr in
> the oilpan build. I'll do just that and leave the class itself (including the
> RefPtr<EventTarget>) as is. That will change in a follow-up CL, but I guess we
> could use a Persistent<EventTarget> in that case.

Probably we want to just use a Persistent for m_controller (instead of making
ServiceWorkerContainer RefCountedWillBeRefCountedGarbageCollected), since
serviceworker/ is actively being developed and we don't want to mess it up at
the moment.

[zerny-chromium, 2014-06-02 12:32:32]

On 2014/06/02 07:50:53, sof wrote:
> Have you run run-bindings-test and verified that the expected outputs did not
> change?

No, I was not aware of the binding generation tests. I've run them and updated
the expected results to use transition types. PTAL.

[sof, 2014-06-02 12:41:03]

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIConnectionEvent.h (right):

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:45: RefPtr<MIDIPort> port;
Should this be RefPtrWillBeMember? (Dictionary.h needs updating also, if so.)

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:48: class MIDIConnectionEvent FINAL
: public Event {
I think this event merits a .cpp file of its own by now.

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:69: virtual void trace(Visitor*
visitor) OVERRIDE
Move the definition the .cpp file?

[haraken, 2014-06-02 12:41:17]

https://codereview.chromium.org/302263004/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestInterfaceEventTarget.h (right):

https://codereview.chromium.org/302263004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestInterfaceEventTarget.h:40: static const int
internalFieldCount = v8DefaultWrapperInternalFieldCount + 2;

oh, this might be problematic, because this increases the size of each
EventTarget wrapper (including all Nodes) by 8 byte.

Given that persistentHandleIndex is not used in non-oilpan builds, we should
probably do something like:

#if ENABLE(OILPAN)
static const int internalFieldCount = v8DefaultWrapperInternalFieldCount + 2
#else
static const int internalFieldCount = v8DefaultWrapperInternalFieldCount + 1
#endif

(I can work on it tomorrow morning if you're not willing to touch the IDL
compiler.)

[zerny-chromium, 2014-06-02 13:06:48]

Thanks for the reviews!

https://codereview.chromium.org/302263004/diff/40001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestInterfaceEventTarget.h (right):

https://codereview.chromium.org/302263004/diff/40001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestInterfaceEventTarget.h:40: static const int
internalFieldCount = v8DefaultWrapperInternalFieldCount + 2;
On 2014/06/02 12:41:18, haraken wrote:
> 
> oh, this might be problematic, because this increases the size of each
> EventTarget wrapper (including all Nodes) by 8 byte.
> 
> Given that persistentHandleIndex is not used in non-oilpan builds, we should
> probably do something like:
> 
> #if ENABLE(OILPAN)
> static const int internalFieldCount = v8DefaultWrapperInternalFieldCount + 2
> #else
> static const int internalFieldCount = v8DefaultWrapperInternalFieldCount + 1
> #endif
> 
> (I can work on it tomorrow morning if you're not willing to touch the IDL
> compiler.)

Yes, please do. I agree that we should not increase the wrapper size for
non-oilpan builds, so I'll wait on your CL before this can land.

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIConnectionEvent.h (right):

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:45: RefPtr<MIDIPort> port;
On 2014/06/02 12:41:03, sof wrote:
> Should this be RefPtrWillBeMember? (Dictionary.h needs updating also, if so.)

I'd rather postpone that to a CL that completes the transition for MIDIPort.
This CL should minimally just support EventTarget and its IDL dependencies. If
you disagree I'm OK with pulling through the change here. (But in general,
moving RefPtrs for EventTarget-derived types is a very large task to complete).

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:48: class MIDIConnectionEvent FINAL
: public Event {
On 2014/06/02 12:41:03, sof wrote:
> I think this event merits a .cpp file of its own by now.

Ok. I'll add one.

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:69: virtual void trace(Visitor*
visitor) OVERRIDE
On 2014/06/02 12:41:03, sof wrote:
> Move the definition the .cpp file?

Done.

[sof, 2014-06-02 13:11:19]

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIConnectionEvent.h (right):

https://codereview.chromium.org/302263004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIConnectionEvent.h:45: RefPtr<MIDIPort> port;
On 2014/06/02 13:06:49, zerny-chromium wrote:
> On 2014/06/02 12:41:03, sof wrote:
> > Should this be RefPtrWillBeMember? (Dictionary.h needs updating also, if
so.)
> 
> I'd rather postpone that to a CL that completes the transition for MIDIPort.
> This CL should minimally just support EventTarget and its IDL dependencies. If
> you disagree I'm OK with pulling through the change here. (But in general,
> moving RefPtrs for EventTarget-derived types is a very large task to
complete).

I'm fine with scoping this CL to that only, and deal with the rest here later.

[zerny-chromium, 2014-06-03 12:57:59]

Rebased on https://codereview.chromium.org/314603003

PTAL.

[haraken, 2014-06-03 13:04:58]

LGTM, thanks!

[zerny-chromium, 2014-06-03 13:11:20]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-06-03 13:11:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/302263004/70001

[commit-bot: I haz the power, 2014-06-03 14:27:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6788)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10063)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10362)

[commit-bot: I haz the power, 2014-06-03 15:27:54]

Change committed as 175380