Bug 129672 – Move GTK WebKit2 code to std::unique_ptr

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

[Zan Dobersek]

Move GTK WK2 code to std::unique_ptr

[Zan Dobersek]

Created attachment 225760 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 225760 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:27
> +#include <wtf/OwnPtr.h>

Why do we need to keep OwnPtr here?

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:36
> -    static PassOwnPtr<WebKitTextChecker> create() { return adoptPtr(new WebKitTextChecker()); }
> +    WebKitTextChecker();

Why don't we keep the create method, but calling make_unique instead of adoptPtr? This way we can keep the constructor private to ensure this is not misused.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:52
>      OwnPtr<WebCore::TextCheckerEnchant> m_textChecker;

This is because WebCore still returns a PassOwnPtr for this?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:410
> -    priv->pageClient = PageClientImpl::create(viewWidget);
> +    priv->pageClient = std::make_unique<PageClientImpl>(viewWidget);

Same here, isn't it possible to keep the create method?

[Zan Dobersek]

Comment on attachment 225760 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:27
>> +#include <wtf/OwnPtr.h>
> 
> Why do we need to keep OwnPtr here?

It's included for the TextCheckerEnchant member.

>> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:36
>> +    WebKitTextChecker();
> 
> Why don't we keep the create method, but calling make_unique instead of adoptPtr? This way we can keep the constructor private to ensure this is not misused.

That's possible and makes sense, but generally we just make the constructor public and use std::make_unique.

There's a nice approach to making it possible to use std::make_unique with a class that has a private constructor by declaring std::make_unique a friend of that class, but I haven't yet gotten this working on MSVC.

>> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:52
>>      OwnPtr<WebCore::TextCheckerEnchant> m_textChecker;
> 
> This is because WebCore still returns a PassOwnPtr for this?

Yes.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:410
>> +    priv->pageClient = std::make_unique<PageClientImpl>(viewWidget);
> 
> Same here, isn't it possible to keep the create method?

Ditto, it's possible but not usually done.

[Zan Dobersek]

Comment on attachment 225760 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:36
>>> +    WebKitTextChecker();
>> 
>> Why don't we keep the create method, but calling make_unique instead of adoptPtr? This way we can keep the constructor private to ensure this is not misused.
> 
> That's possible and makes sense, but generally we just make the constructor public and use std::make_unique.
> 
> There's a nice approach to making it possible to use std::make_unique with a class that has a private constructor by declaring std::make_unique a friend of that class, but I haven't yet gotten this working on MSVC.

To correct myself, std::make_unique (when not declared as a friend of the class in question) still requires a public constructor. The proper alternative is returning std::unique_ptr<T>(new T) in the create() method.

[Zan Dobersek]

Created attachment 226820 [details]
Patch

[Zan Dobersek]

Comment on attachment 226820 [details]
Patch

Clearing flags on attachment: 226820

Committed r165707: <http://trac.webkit.org/changeset/165707>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.