Bug 127554 – [GLIB] Add GUniqueOutPtr and use it instead of GOwnPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Carlos Garcia Campos]

In r162516 we added GUniquePtr that was used to replace GOwnPtr in all case except when the output pointer is needed. For those cases we can't use std::unique_ptr, so we need a class to cover these cases.

[Carlos Garcia Campos]

Created attachment 222092 [details]
Patch

[WebKit Commit Bot]

Attachment 222092 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/gtk/webkit/webkitdownload.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/gobject/GUniquePtr.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 222093 [details]
Fixed coding style issues

[Zan Dobersek]

Comment on attachment 222093 [details]
Fixed coding style issues

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

> Tools/TestWebKitAPI/Tests/WTF/gobject/GUniquePtr.cpp:188
> +        a.reset();

No need for calling reset() here -- the GUniqueOutPtr will handle freeing the memory when it goes out of scope and is destroyed.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:146
> -    result.clear();
> +    result.reset();

No need for calling reset() here (since the unique pointer will be reset when outPtr() is called the next time) ...

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:168
> -    result.clear();
> +    result.reset();

... or here (since the pointer will be deleted when GUniqueOutPtr goes out of scope).

I guess GUniqueOutPtr is now smart enough that the user doesn't really have to know about the reset() method, so it can be made private.

[Carlos Garcia Campos]

Comment on attachment 222093 [details]
Fixed coding style issues

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

Thanks for the review.

>> Tools/TestWebKitAPI/Tests/WTF/gobject/GUniquePtr.cpp:188
>> +        a.reset();
> 
> No need for calling reset() here -- the GUniqueOutPtr will handle freeing the memory when it goes out of scope and is destroyed.

Well, this is actually a test to check that reset works, if we make it private we don't need this test.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:146
>> +    result.reset();
> 
> No need for calling reset() here (since the unique pointer will be reset when outPtr() is called the next time) ...

You are right.

>> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:168
>> +    result.reset();
> 
> ... or here (since the pointer will be deleted when GUniqueOutPtr goes out of scope).
> 
> I guess GUniqueOutPtr is now smart enough that the user doesn't really have to know about the reset() method, so it can be made private.

Yes, I agree, and the proof is that it's only used in this test and it's not actually needed

[Carlos Garcia Campos]

Created attachment 222435 [details]
Updated patch

Made reset private as suggested by Zan

[Carlos Garcia Campos]

Created attachment 222574 [details]
Rebased patch

[Carlos Garcia Campos]

Created attachment 223627 [details]
Rebased patch

[Gustavo Noronha (kov)]

Comment on attachment 223627 [details]
Rebased patch

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

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:-272
> -            // FIXME: should GOwnPtr be smarter about this and replace the existing ptr when outPtr() is used?
> -            key.clear();
> -            value.clear();

\o/

[Carlos Garcia Campos]

Committed r163797: <http://trac.webkit.org/changeset/163797>