Make IDL Callbacks non-refcounted by default

Make IDL Callbacks non-refcounted by default

Callbacks are different from other IDL-generated code, in that they invert
the flow of control of other interfaces (they let C++ code talk to JS, rather
than the other way around). For this reason, they don't interact with the GC,
and so can usually have a single owner rather than be managed with RefPtrs.

This patch converts most existing callbacks to be non-refcounted, and
instead manages them with OwnPtrs. Those callbacks where such conversion
was non-trivial are listed in IsLegacyRefCountedCallback() in
code_generator_v8.pm, and can be converted as follow-ons to this patch.

BUG=323681
R=haraken@chromium.org

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

[adamk, 2013-11-26 21:26:10]

Hi reviewers, I've put a variety of bindings OWNERS to make sure y'all are on
board with this approach.

[abarth-chromium, 2013-11-26 21:50:15]

Sounds like a good idea to me.

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:6359: sub
IsLegacyRefCountedCallback
Should this be an IDL attribute instead of hard-coded in the code generator?

[adamk, 2013-11-26 21:54:27]

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:6359: sub
IsLegacyRefCountedCallback
On 2013/11/26 21:50:16, abarth wrote:
> Should this be an IDL attribute instead of hard-coded in the code generator?

That would require having the attributes of an interface available whenever that
interface appears in a method signature. I don't believe we currently have that
capability (correct me if I'm wrong).

[abarth-chromium, 2013-11-26 21:57:10]

On 2013/11/26 21:54:27, adamk wrote:
>
https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
> File Source/bindings/scripts/code_generator_v8.pm (right):
> 
>
https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
> Source/bindings/scripts/code_generator_v8.pm:6359: sub
> IsLegacyRefCountedCallback
> On 2013/11/26 21:50:16, abarth wrote:
> > Should this be an IDL attribute instead of hard-coded in the code generator?
> 
> That would require having the attributes of an interface available whenever
that
> interface appears in a method signature. I don't believe we currently have
that
> capability (correct me if I'm wrong).

I see.  That makes sense.

[haraken, 2013-11-27 00:37:10]

LGTM

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/scripts/c...
Source/bindings/scripts/code_generator_v8.pm:6359: sub
IsLegacyRefCountedCallback
On 2013/11/26 21:54:28, adamk wrote:
> On 2013/11/26 21:50:16, abarth wrote:
> > Should this be an IDL attribute instead of hard-coded in the code generator?
> 
> That would require having the attributes of an interface available whenever
that
> interface appears in a method signature. I don't believe we currently have
that
> capability (correct me if I'm wrong).

Yeah.

Shall we add a FIXME that IsLegacyRefCountedCallback should be removed?

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/v8/V8Call...
File Source/bindings/v8/V8Callback.h (right):

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/v8/V8Call...
Source/bindings/v8/V8Callback.h:60: return nullptr;

Nit: Do we prefer nullptr to 0 for an OwnPtr? (I'm not sure of the current
coding rule.)

[adamk, 2013-11-27 00:45:20]

I'd be okay with a FIXME, but it's not clear to me that it's going to get fixed
anytime soon. That's why I went with "Legacy" in the name :)

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/v8/V8Call...
File Source/bindings/v8/V8Callback.h (right):

https://codereview.chromium.org/85263006/diff/20001/Source/bindings/v8/V8Call...
Source/bindings/v8/V8Callback.h:60: return nullptr;
On 2013/11/27 00:37:11, haraken wrote:
> 
> Nit: Do we prefer nullptr to 0 for an OwnPtr? (I'm not sure of the current
> coding rule.)

Only nullptr will work, in fact! OwnPtr's can't be constructed from 0.

[jochen (gone - plz use gerrit), 2013-11-27 08:59:49]

looks good

[commit-bot: I haz the power, 2013-11-27 16:42:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/85263006/20001

[commit-bot: I haz the power, 2013-11-27 16:42:18]

Failed to apply patch for Source/modules/mediastream/RTCStatsRequestImpl.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/mediastream/RTCStatsRequestImpl.h
  Hunk #2 FAILED at 53.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/mediastream/RTCStatsRequestImpl.h.rej

Patch:       Source/modules/mediastream/RTCStatsRequestImpl.h
Index: Source/modules/mediastream/RTCStatsRequestImpl.h
diff --git a/Source/modules/mediastream/RTCStatsRequestImpl.h
b/Source/modules/mediastream/RTCStatsRequestImpl.h
index
2cf6dc932b7178b341a36d1954cfdf1a3c6435a8..14afa161182a6ff815cd3f919e1c8523468d7bcf
100644
--- a/Source/modules/mediastream/RTCStatsRequestImpl.h
+++ b/Source/modules/mediastream/RTCStatsRequestImpl.h
@@ -39,7 +39,7 @@ class RTCStatsCallback;
 
 class RTCStatsRequestImpl : public RTCStatsRequest, public ActiveDOMObject {
 public:
-    static PassRefPtr<RTCStatsRequestImpl> create(ExecutionContext*,
PassRefPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);
+    static PassRefPtr<RTCStatsRequestImpl> create(ExecutionContext*,
PassOwnPtr<RTCStatsCallback>, PassRefPtr<MediaStreamTrack>);
     virtual ~RTCStatsRequestImpl();
 
     virtual PassRefPtr<RTCStatsResponseBase> createResponse() OVERRIDE;
@@ -53,11 +53,11 @@ public:
     virtual void stop() OVERRIDE;
 
 private:
-    RTCStatsRequestImpl(ExecutionContext*, PassRefPtr<RTCStatsCallback>,
PassRefPtr<MediaStreamTrack>);
+    RTCStatsRequestImpl(ExecutionContext*, PassOwnPtr<RTCStatsCallback>,
PassRefPtr<MediaStreamTrack>);
 
     void clear();
 
-    RefPtr<RTCStatsCallback> m_successCallback;
+    OwnPtr<RTCStatsCallback> m_successCallback;
     RefPtr<MediaStreamDescriptor> m_stream;
     RefPtr<MediaStreamComponent> m_component;
 };

[commit-bot: I haz the power, 2013-11-27 18:27:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/85263006/40001

[adamk, 2013-11-27 19:40:21]

Committed patchset #3 manually as r162776 (presubmit successful).