Bug 127554 - [GLIB] Add GUniqueOutPtr and use it instead of GOwnPtr
Summary: [GLIB] Add GUniqueOutPtr and use it instead of GOwnPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2014-01-24 04:25 PST by Carlos Garcia Campos
Modified: 2014-02-10 10:17 PST (History)
1 user (show)

See Also:


Attachments
Patch (84.21 KB, patch)
2014-01-24 04:34 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Fixed coding style issues (84.31 KB, patch)
2014-01-24 05:44 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (84.00 KB, patch)
2014-01-28 06:15 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (83.98 KB, patch)
2014-01-29 08:37 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (83.96 KB, patch)
2014-02-09 05:15 PST, Carlos Garcia Campos
gns: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-01-24 04:25:49 PST
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.
Comment 1 Carlos Garcia Campos 2014-01-24 04:34:43 PST
Created attachment 222092 [details]
Patch
Comment 2 WebKit Commit Bot 2014-01-24 04:36:24 PST
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.
Comment 3 Carlos Garcia Campos 2014-01-24 05:44:15 PST
Created attachment 222093 [details]
Fixed coding style issues
Comment 4 Zan Dobersek 2014-01-27 07:01:01 PST
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.
Comment 5 Carlos Garcia Campos 2014-01-27 08:51:00 PST
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
Comment 6 Carlos Garcia Campos 2014-01-28 06:15:42 PST
Created attachment 222435 [details]
Updated patch

Made reset private as suggested by Zan
Comment 7 Carlos Garcia Campos 2014-01-29 08:37:10 PST
Created attachment 222574 [details]
Rebased patch
Comment 8 Carlos Garcia Campos 2014-02-09 05:15:21 PST
Created attachment 223627 [details]
Rebased patch
Comment 9 Gustavo Noronha (kov) 2014-02-10 05:53:08 PST
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/
Comment 10 Carlos Garcia Campos 2014-02-10 10:17:35 PST
Committed r163797: <http://trac.webkit.org/changeset/163797>