Bug 94996 – [chromium] Clean up dependencies of WebScrollbar and WebScrollbarLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Clean up dependencies of WebScrollbar and WebScrollbarLayer

[James Robinson]

Created attachment 160540 [details]
Patch

[James Robinson]

Enne - I wasn't sure if this was already on your plate or not, but it wasn't too hard so I went ahead and did it.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

Comment on attachment 160540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160540&action=review

R=me.  Thanks for taking care of this.

> Source/Platform/chromium/public/WebScrollbarLayer.h:42
> +    // This takes ownership of the provided WebScrollbar.
> +    WEBKIT_EXPORT static WebScrollbarLayer* create(WebScrollbar*, WebScrollbarThemePainter, PassOwnPtr<WebScrollbarThemeGeometry>);

No need for a comment--make the WebScrollbar a PassOwnPtr too like the geometry.  It's weird to be inconsistent about ownership transfer.

> Source/WebCore/platform/chromium/support/WebScrollbarImpl.h:38
> +    explicit WebScrollbarImpl(WebCore::Scrollbar*);

Can you add a static create function that returns a PassOwnPtr so you don't have to adoptPtr it all over the place?

[James Robinson]

(In reply to comment #4)
> (From update of attachment 160540 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160540&action=review
> 
> R=me.  Thanks for taking care of this.
> 
> > Source/Platform/chromium/public/WebScrollbarLayer.h:42
> > +    // This takes ownership of the provided WebScrollbar.
> > +    WEBKIT_EXPORT static WebScrollbarLayer* create(WebScrollbar*, WebScrollbarThemePainter, PassOwnPtr<WebScrollbarThemeGeometry>);
> 
> No need for a comment--make the WebScrollbar a PassOwnPtr too like the geometry.  It's weird to be inconsistent about ownership transfer.

Can't do that - you can't expect users of the WebKit API to have access to PassOwnPtr since it's in WTF and we have no equivalent concept in the public API.  I have to kill the other PassOwnPtr as well soon.


> 
> > Source/WebCore/platform/chromium/support/WebScrollbarImpl.h:38
> > +    explicit WebScrollbarImpl(WebCore::Scrollbar*);
> 
> Can you add a static create function that returns a PassOwnPtr so you don't have to adoptPtr it all over the place?

No, you can't PassOwnPtr<> through the WebKit API

[Adrienne Walker]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 160540 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160540&action=review
> > 
> > R=me.  Thanks for taking care of this.
> > 
> > > Source/Platform/chromium/public/WebScrollbarLayer.h:42
> > > +    // This takes ownership of the provided WebScrollbar.
> > > +    WEBKIT_EXPORT static WebScrollbarLayer* create(WebScrollbar*, WebScrollbarThemePainter, PassOwnPtr<WebScrollbarThemeGeometry>);
> > 
> > No need for a comment--make the WebScrollbar a PassOwnPtr too like the geometry.  It's weird to be inconsistent about ownership transfer.
> 
> Can't do that - you can't expect users of the WebKit API to have access to PassOwnPtr since it's in WTF and we have no equivalent concept in the public API.  I have to kill the other PassOwnPtr as well soon.

Ok, sure.  I still think you should be consistent here, one way or the other.

> > > Source/WebCore/platform/chromium/support/WebScrollbarImpl.h:38
> > > +    explicit WebScrollbarImpl(WebCore::Scrollbar*);
> > 
> > Can you add a static create function that returns a PassOwnPtr so you don't have to adoptPtr it all over the place?
> 
> No, you can't PassOwnPtr<> through the WebKit API

What's wrong with PassOwnPtr here?

[Adrienne Walker]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 160540 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=160540&action=review
> > > 
> > > R=me.  Thanks for taking care of this.
> > > 
> > > > Source/Platform/chromium/public/WebScrollbarLayer.h:42
> > > > +    // This takes ownership of the provided WebScrollbar.
> > > > +    WEBKIT_EXPORT static WebScrollbarLayer* create(WebScrollbar*, WebScrollbarThemePainter, PassOwnPtr<WebScrollbarThemeGeometry>);
> > > 
> > > No need for a comment--make the WebScrollbar a PassOwnPtr too like the geometry.  It's weird to be inconsistent about ownership transfer.
> > 
> > Can't do that - you can't expect users of the WebKit API to have access to PassOwnPtr since it's in WTF and we have no equivalent concept in the public API.  I have to kill the other PassOwnPtr as well soon.
> 
> Ok, sure.  I still think you should be consistent here, one way or the other.

Actually, this is in WebScrollbarLayerImpl, so why not use PassOwnPtr?

[James Robinson]

Comment on attachment 160540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160540&action=review

>>>>> Source/Platform/chromium/public/WebScrollbarLayer.h:42
>>>>> +    WEBKIT_EXPORT static WebScrollbarLayer* create(WebScrollbar*, WebScrollbarThemePainter, PassOwnPtr<WebScrollbarThemeGeometry>);
>>>> 
>>>> No need for a comment--make the WebScrollbar a PassOwnPtr too like the geometry.  It's weird to be inconsistent about ownership transfer.
>>> 
>>> Can't do that - you can't expect users of the WebKit API to have access to PassOwnPtr since it's in WTF and we have no equivalent concept in the public API.  I have to kill the other PassOwnPtr as well soon.
>> 
>> Ok, sure.  I still think you should be consistent here, one way or the other.
> 
> Actually, this is in WebScrollbarLayerImpl, so why not use PassOwnPtr?

Not sure what you mean.  This header is public/WebScrollbarLayer.h, so I can't use PassOwnPtr<> here.

>>>> Source/WebCore/platform/chromium/support/WebScrollbarImpl.h:38
>>>> +    explicit WebScrollbarImpl(WebCore::Scrollbar*);
>>> 
>>> Can you add a static create function that returns a PassOwnPtr so you don't have to adoptPtr it all over the place?
>> 
>> No, you can't PassOwnPtr<> through the WebKit API
> 
> What's wrong with PassOwnPtr here?

This constructor is only called by the implementation of WebScrollbarLayer::create(). I could make this PassOwnPtr<> and implement WebScrollbarLayer::create() thusly:

    return WebScrollbarLayerImpl::create(...).leakPtr();

but that doesn't seem worth it.

The adoptPtr()s are all around the return value of WebScrollbarLayer::create(), not around this constructor, so I couldn't remove any

[Adrienne Walker]

Ah, you're right on both counts.  Sorry, I was thinking about this backwards.    Having OwnPtr stuff (for now) on both sides of the Platform API but not crossing it is super confusing.

[James Robinson]

No kidding! I think I can write a PassOwnPtr<> equiv that crosses the API boundary, will mess around with that a bit.  It'd be great to get type safety for these functions that transfer ownership.  Probably not worth holding the rest of this up, however.

[James Robinson]

WebPassOwnPtr<> in https://bugs.webkit.org/show_bug.cgi?id=95040 should help on both counts. I'll land this as-is and convert to smart API types once we get those sorted out.

[James Robinson]

Committed r126803: <http://trac.webkit.org/changeset/126803>

[James Robinson]

(In reply to comment #12)
> Committed r126803: <http://trac.webkit.org/changeset/126803>

Landed with raw pointers everywhere.  I'll convert over the WebPassOwnPtr<> where possible when bug 95040 lands - still figuring out some type details on that.