Have LiveNodeListBase::ownerNode() return a reference

Have LiveNodeListBase::ownerNode() return a reference

Have LiveNodeListBase::ownerNode() return a reference instead of a pointer as
it can never be null. It makes the code look safer, no longer requires
assertions and avoids unnecessary null checks in the generated bindings.

R=adamk@chromium.org, haraken@chromium.org, adamk, haraken
NOTRY=true

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

[adamk, 2014-02-25 22:31:14]

lgtm!

[Inactive, 2014-02-25 22:40:24]

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

[commit-bot: I haz the power, 2014-02-25 22:42:46]

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

[commit-bot: I haz the power, 2014-02-25 23:10:43]

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

[commit-bot: I haz the power, 2014-02-25 23:28:15]

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

[haraken, 2014-02-26 00:53:50]

LGTM

https://codereview.chromium.org/180143003/diff/1/Source/core/html/HTMLAllColl...
File Source/core/html/HTMLAllCollection.idl (right):

https://codereview.chromium.org/180143003/diff/1/Source/core/html/HTMLAllColl...
Source/core/html/HTMLAllCollection.idl:31:
SetWrapperReferenceFromReference=ownerNode,

Is it likely that you can completely replace [SetWrapperReferenceFrom] with
[SetWrapperReferenceFromReference]?

(In the end, [SetWrapperReferenceFromReference] should be renamed to
[SetWrapperReferenceFrom]. [SetWrapperReferenceFromReference] is confusing since
the first "Reference" and the second "Reference" mean different things:-)

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

On 2014/02/26 00:53:50, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/180143003/diff/1/Source/core/html/HTMLAllColl...
> File Source/core/html/HTMLAllCollection.idl (right):
> 
>
https://codereview.chromium.org/180143003/diff/1/Source/core/html/HTMLAllColl...
> Source/core/html/HTMLAllCollection.idl:31:
> SetWrapperReferenceFromReference=ownerNode,
> 
> Is it likely that you can completely replace [SetWrapperReferenceFrom] with
> [SetWrapperReferenceFromReference]?
> 
> (In the end, [SetWrapperReferenceFromReference] should be renamed to
> [SetWrapperReferenceFrom]. [SetWrapperReferenceFromReference] is confusing
since
> the first "Reference" and the second "Reference" mean different things:-)

I agree the naming of "SetWrapperReferenceFromReference" is a bit confusing. I
don't have a better proposal at the moment but I'll give it some thought.

Also, I don't think we can completely replace [SetWrapperReferenceFrom] with
[SetWrapperReferenceFromReference]. There are classes whose method can return
null (e.g. NodeList::virtualOwnerNode() or StyleSheetList::document()). I
haven't spent much time on this problem though.

[haraken, 2014-02-26 01:20:35]

> I agree the naming of "SetWrapperReferenceFromReference" is a bit confusing. I
> don't have a better proposal at the moment but I'll give it some thought.
> 
> Also, I don't think we can completely replace [SetWrapperReferenceFrom] with
> [SetWrapperReferenceFromReference]. There are classes whose method can return
> null (e.g. NodeList::virtualOwnerNode() or StyleSheetList::document()). I
> haven't spent much time on this problem though.

(Although I agreed with [SetWrapperReferenceFromReference] before,) another
solution might be to add a helper method that returns a raw poniter so that
bindings can always use [SetWrapperReferenceFrom]. Specifically, we can define
'Node* ownerNodeForBinding()'. We're adding these kinds of helpers to simplify
bindings.

[Nils Barth (inactive), 2014-02-26 01:21:26]

On 2014/02/26 01:14:55, Chris Dumez wrote:
> I agree the naming of "SetWrapperReferenceFromReference" is a bit confusing. I
> don't have a better proposal at the moment but I'll give it some thought.
> 
> Also, I don't think we can completely replace [SetWrapperReferenceFrom] with
> [SetWrapperReferenceFromReference]. There are classes whose method can return
> null (e.g. NodeList::virtualOwnerNode() or StyleSheetList::document()). I
> haven't spent much time on this problem though.

(Brainstorming): how about:
* [SetWrapperReferenceFrom]
* [SetWrapperReferenceOrNullFrom] / [SetWrapperReferenceFromOrNull]
?

[Inactive, 2014-02-26 01:58:59]

On 2014/02/26 01:20:35, haraken wrote:
> (Although I agreed with [SetWrapperReferenceFromReference] before,) another
> solution might be to add a helper method that returns a raw poniter so that
> bindings can always use [SetWrapperReferenceFrom]. Specifically, we can define
> 'Node* ownerNodeForBinding()'. We're adding these kinds of helpers to simplify
> bindings.

Hmm, I am personally not a big fan of having methods specific to the bindings if
we can avoid it. Doing this will clutter our code a little bit as we'll
duplicate the methods. It also means generating unnecessarily complex bindings
code for cases where we know it cannot be null.
I honestly have no idea what this bindings code does so I admittedly have no
idea if it can be performance sensitive. Likely, the binary size impact is not
very significant either way as this IDL extended attribute is used by less than
15 interfaces and not all of them can be updated to return references.

I understand we want to reduce the complexity of the bindings generator to make
it easier to maintain. However, in this particular instance, I am not convinced
the attribute adds *much* complexity.
However, it does improve the readability of our implementation quite a bit (I
have always wished we could return more references instead of pointers to the
bindings).

- IDEA:
Note that we don't really care about the simplicity / performance of
visitDOMWrapper(), I think we could make it work no matter if the implementation
returns a pointer or a reference. This would avoid having duplicate IDL extended
attributes or duplicate methods on implementation side. The trick would be to
construct a RefPtr as it can be constructed from the return value (as RefPtr can
be created from either a pointer or a ref). The generated bindings code would
look like:
    if (RefPtr<Node> owner = impl->ownerNode()) {
        Node* root = V8GCController::opaqueRootForGC(owner.get(), isolate);
       
isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(root)),
wrapper);
        return;
    }
    setObjectGroup(object, wrapper, isolate);

Otherwise (if this is perf sensitive), I think we should just find a better name
for the extended IDL attribute.

[haraken, 2014-02-26 02:23:56]

> - IDEA:
> Note that we don't really care about the simplicity / performance of
> visitDOMWrapper(), I think we could make it work no matter if the
implementation
> returns a pointer or a reference. This would avoid having duplicate IDL
extended
> attributes or duplicate methods on implementation side. The trick would be to
> construct a RefPtr as it can be constructed from the return value (as RefPtr
can
> be created from either a pointer or a ref). The generated bindings code would
> look like:
>     if (RefPtr<Node> owner = impl->ownerNode()) {
>         Node* root = V8GCController::opaqueRootForGC(owner.get(), isolate);
>        
> isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(root)),
> wrapper);
>         return;
>     }
>     setObjectGroup(object, wrapper, isolate);
> 
> Otherwise (if this is perf sensitive), I think we should just find a better
name
> for the extended IDL attribute.

Thanks for the great write-up!

I like the idea of RefPtr. It seems like a good balancing point of our current
situations. I agree that the code is not performance-sensitive.

[Inactive, 2014-02-26 02:27:33]

On 2014/02/26 02:23:56, haraken wrote:
> > - IDEA:
> > Note that we don't really care about the simplicity / performance of
> > visitDOMWrapper(), I think we could make it work no matter if the
> implementation
> > returns a pointer or a reference. This would avoid having duplicate IDL
> extended
> > attributes or duplicate methods on implementation side. The trick would be
to
> > construct a RefPtr as it can be constructed from the return value (as RefPtr
> can
> > be created from either a pointer or a ref). The generated bindings code
would
> > look like:
> >     if (RefPtr<Node> owner = impl->ownerNode()) {
> >         Node* root = V8GCController::opaqueRootForGC(owner.get(), isolate);
> >        
> >
isolate->SetReferenceFromGroup(v8::UniqueId(reinterpret_cast<intptr_t>(root)),
> > wrapper);
> >         return;
> >     }
> >     setObjectGroup(object, wrapper, isolate);
> > 
> > Otherwise (if this is perf sensitive), I think we should just find a better
> name
> > for the extended IDL attribute.
> 
> Thanks for the great write-up!
> 
> I like the idea of RefPtr. It seems like a good balancing point of our current
> situations. I agree that the code is not performance-sensitive.

Ok, I'll likely prepare a CL for this tomorrow then. I'm glad we could find a
suitable compromise.

[Inactive, 2014-02-26 02:31:43]

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

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

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

[Inactive, 2014-02-26 03:46:29]

Committed patchset #2 manually as r167848 (presubmit successful).