Kill [SetWrapperReferenceFromReference] IDL extended attribute

Kill [SetWrapperReferenceFromReference] IDL extended attribute

Kill [SetWrapperReferenceFromReference] IDL extended attribute to reduce the
complexity of our IDL and of our bindings generators. We now use only
[SetWrapperReferenceFrom] IDL extended attribute and the generated bindings
code will accept that the implementation returns either a reference or a
pointer (by constructing using WTF::getPtr()).

R=haraken, adamk

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

[Inactive, 2014-02-26 14:16:55]

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:572: if (RefPtr<Node> owner =
PassRefPtr<Node>(impl->${isReachableMethod}())) {
The RefPtr / PassRefPtr constructor that accepts a reference is unfortunately
explicit thus the explicit PassRefPtr<Node>().

[Inactive, 2014-02-26 14:17:31]

+deepak in cc as he added the attribute.

[haraken, 2014-02-26 16:06:39]

LGTM

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:572: if (RefPtr<Node> owner =
PassRefPtr<Node>(impl->${isReachableMethod}())) {

$isReachableMethod => $setWrapperReferenceMethod (or something better)

[Inactive, 2014-02-26 16:14:15]

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/181513002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:572: if (RefPtr<Node> owner =
PassRefPtr<Node>(impl->${isReachableMethod}())) {
On 2014/02/26 16:06:40, haraken wrote:
> 
> $isReachableMethod => $setWrapperReferenceMethod (or something better)

Done.

[adamk, 2014-02-26 17:55:37]

Seems fine, but I wonder if this could be a little simpler...

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

https://codereview.chromium.org/181513002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:572: if (RefPtr<Node> owner =
PassRefPtr<Node>(impl->${setWrapperReferenceMethod}())) {
Why make a RefPTr here when we didn't make one before? Is this just to take
advantage of the reference constructor for PassRefPtr? If so, I wonder if you
could instead augment WTF::getPtr
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
to handle references...

[Inactive, 2014-02-26 18:01:56]

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

https://codereview.chromium.org/181513002/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:572: if (RefPtr<Node> owner =
PassRefPtr<Node>(impl->${setWrapperReferenceMethod}())) {
On 2014/02/26 17:55:38, adamk wrote:
> Why make a RefPTr here when we didn't make one before? Is this just to take
> advantage of the reference constructor for PassRefPtr? If so, I wonder if you
> could instead augment WTF::getPtr
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> to handle references...

Yes, this is the trick, we construct a RefPtr because it can be constructed from
either a pointer or a reference and the implementation may return either (we
don't know).

Augmenting the WTF::getPtr() to take a reference in is a much better idea
though. I'll do this instead, thanks :)

[Inactive, 2014-02-26 18:18:04]

I am now using WTF::getPtr().

+tkent for the change to wtf/

[haraken, 2014-02-26 22:48:18]

tkent@ is OOO today.

jochen@: would you take a look at wtf?

[Inactive, 2014-02-26 22:50:22]

On 2014/02/26 22:48:18, haraken wrote:
> tkent@ is OOO today.
> 
> jochen@: would you take a look at wtf?

Hmm, looks like jochen is out of office as well?

[Inactive, 2014-02-26 22:50:47]

+eseidel for wtf/

[jochen (gone - plz use gerrit), 2014-02-27 14:24:11]

lgtm

[Inactive, 2014-02-27 14:25:26]

On 2014/02/27 14:24:11, jochen (OOO from March 1) wrote:
> lgtm

Thanks!

[Inactive, 2014-02-27 14:25:30]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-27 14:26:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/181513002/40001

[commit-bot: I haz the power, 2014-02-27 16:15:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 16:15:45]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[Inactive, 2014-02-27 16:18:20]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-27 16:18:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/181513002/40001

[commit-bot: I haz the power, 2014-02-27 16:18:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 16:18:39]

Failed to apply patch for Source/bindings/scripts/code_generator_v8.pm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/bindings/scripts/code_generator_v8.pm
  |diff --git a/Source/bindings/scripts/code_generator_v8.pm
b/Source/bindings/scripts/code_generator_v8.pm
  |index
a554f2f090e7a318991dc654c0693d359a64a558..79ca0c2f2eebf32e72a227acd216f0362b119f72
100644
  |--- a/Source/bindings/scripts/code_generator_v8.pm
  |+++ b/Source/bindings/scripts/code_generator_v8.pm
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       Source/bindings/scripts/code_generator_v8.pm
Index: Source/bindings/scripts/code_generator_v8.pm
diff --git a/Source/bindings/scripts/code_generator_v8.pm
b/Source/bindings/scripts/code_generator_v8.pm
index
a554f2f090e7a318991dc654c0693d359a64a558..79ca0c2f2eebf32e72a227acd216f0362b119f72
100644
--- a/Source/bindings/scripts/code_generator_v8.pm
+++ b/Source/bindings/scripts/code_generator_v8.pm
@@ -513,7 +513,7 @@ sub HeaderFilesForInterface
 sub NeedsVisitDOMWrapper
 {
     my $interface = shift;
-    return
ExtendedAttributeContains($interface->extendedAttributes->{"Custom"},
"VisitDOMWrapper") ||
$interface->extendedAttributes->{"SetWrapperReferenceFrom"} ||
$interface->extendedAttributes->{"SetWrapperReferenceFromReference"} ||
$interface->extendedAttributes->{"SetWrapperReferenceTo"} ||
SVGTypeNeedsToHoldContextElement($interface->name);
+    return
ExtendedAttributeContains($interface->extendedAttributes->{"Custom"},
"VisitDOMWrapper") ||
$interface->extendedAttributes->{"SetWrapperReferenceFrom"} ||
$interface->extendedAttributes->{"SetWrapperReferenceTo"} ||
SVGTypeNeedsToHoldContextElement($interface->name);
 }
 
 sub GenerateVisitDOMWrapper
@@ -562,37 +562,25 @@ END
 END
     }
 
-    my $isReachableMethod =
$interface->extendedAttributes->{"SetWrapperReferenceFrom"};
-    my $isReachableReferenceMethod =
$interface->extendedAttributes->{"SetWrapperReferenceFromReference"};
-
-    if ($isReachableMethod || $isReachableReferenceMethod) {
+    my $setWrapperReferenceMethod =
$interface->extendedAttributes->{"SetWrapperReferenceFrom"};
+    if ($setWrapperReferenceMethod) {
         AddToImplIncludes("bindings/v8/V8GCController.h");
         AddToImplIncludes("core/dom/Element.h");
-        if ($isReachableMethod) {
-            $code .= <<END;
-    if (Node* owner = impl->${isReachableMethod}()) {
+        $code .= <<END;
+    // The ${setWrapperReferenceMethod}() method may return a reference or a
pointer.
+    if (Node* owner = WTF::getPtr(impl->${setWrapperReferenceMethod}())) {
         Node* root = V8GCController::opaqueRootForGC(owner, isolate);
        
isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(root)),
wrapper);
         return;
     }
 END
-        } else {
-            $code .= <<END;
-    Node* root =
V8GCController::opaqueRootForGC(&impl->${isReachableReferenceMethod}(),
isolate);
-   
isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(root)),
wrapper);
-}
-
-END
-        }
     }
 
-    if (!$isReachableReferenceMethod) {
-        $code .= <<END;
+    $code .= <<END;
     setObjectGroup(object, wrapper, isolate);
 }
 
 END
-    }
 
     $implementation{nameSpaceWebCore}->add($code);
 }

[Inactive, 2014-02-27 16:25:31]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-27 16:25:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/181513002/60001

[commit-bot: I haz the power, 2014-02-27 19:42:49]

Change committed as 168048

[Nils Barth (inactive), 2014-02-28 02:16:34]

On 2014/02/27 19:42:49, I haz the power (commit-bot) wrote:
> Change committed as 168048

Thanks Chris; docs updated:
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-a...

[Nils Barth (inactive), 2014-02-28 02:18:16]

Just noticed some cruft; will fix.

https://codereview.chromium.org/181513002/diff/60001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/181513002/diff/60001/Source/bindings/template...
Source/bindings/templates/interface.cpp:706: {% if reachable_node_function or
set_wrapper_reference_to_list %}
This conditional is no longer necessary; I'll fix in a followup.

[Nils Barth (inactive), 2014-03-03 01:58:30]

https://codereview.chromium.org/181513002/diff/60001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/181513002/diff/60001/Source/bindings/template...
Source/bindings/templates/interface.cpp:706: {% if reachable_node_function or
set_wrapper_reference_to_list %}
On 2014/02/28 02:18:16, Nils Barth wrote:
> This conditional is no longer necessary; I'll fix in a followup.

Fixed in:
Cleanup [SetWrapperReferenceFromReference] cruft
https://codereview.chromium.org/176993007/