Move GTK WK2 code to std::unique_ptr
Created attachment 225760 [details] Patch
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 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 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 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.
Created attachment 226820 [details] Patch
Comment on attachment 226820 [details] Patch Clearing flags on attachment: 226820 Committed r165707: <http://trac.webkit.org/changeset/165707>
All reviewed patches have been landed. Closing bug.