Bug 129672

Summary: Move GTK WebKit2 code to std::unique_ptr
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, gustavo, mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128007    
Attachments:
Description Flags
Patch
none
Patch none

Description Zan Dobersek 2014-03-04 01:43:13 PST
Move GTK WK2 code to std::unique_ptr
Comment 1 Zan Dobersek 2014-03-04 01:44:01 PST
Created attachment 225760 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-04 01:46:12 PST
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
Comment 3 Carlos Garcia Campos 2014-03-04 02:20:17 PST
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?
Comment 4 Zan Dobersek 2014-03-15 06:09:24 PDT
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.
Comment 5 Zan Dobersek 2014-03-15 07:02:08 PDT
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.
Comment 6 Zan Dobersek 2014-03-15 07:53:47 PDT
Created attachment 226820 [details]
Patch
Comment 7 Zan Dobersek 2014-03-16 12:47:30 PDT
Comment on attachment 226820 [details]
Patch

Clearing flags on attachment: 226820

Committed r165707: <http://trac.webkit.org/changeset/165707>
Comment 8 Zan Dobersek 2014-03-16 12:47:43 PDT
All reviewed patches have been landed.  Closing bug.