Bug 139019 - Use std::unique_ptr instead of PassOwnPtr|OwnPtr for Pasteboard
Summary: Use std::unique_ptr instead of PassOwnPtr|OwnPtr for Pasteboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 128007
  Show dependency treegraph
 
Reported: 2014-11-23 20:19 PST by Joonghun Park
Modified: 2014-12-03 19:02 PST (History)
3 users (show)

See Also:


Attachments
Patch (14.16 KB, patch)
2014-11-23 22:18 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (15.01 KB, patch)
2014-11-23 23:10 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
<WIP> (15.15 KB, patch)
2014-11-24 02:03 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
<WIP> (15.73 KB, patch)
2014-11-24 02:36 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (15.96 KB, patch)
2014-11-24 06:42 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (15.98 KB, patch)
2014-11-24 07:05 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (15.97 KB, patch)
2014-11-24 08:11 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (15.91 KB, patch)
2014-11-24 08:28 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (15.91 KB, patch)
2014-12-02 10:31 PST, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2014-11-23 20:19:48 PST
Change from PassOwnPtr|OwnPtr to std::unique_ptr for Pasteboard in All port(Mac, IOS, Efl, Gtk, Win).
Comment 1 Joonghun Park 2014-11-23 22:18:14 PST
Created attachment 242144 [details]
Patch
Comment 2 Joonghun Park 2014-11-23 23:10:45 PST
Created attachment 242146 [details]
Patch
Comment 3 Gyuyoung Kim 2014-11-24 00:47:16 PST
Comment on attachment 242146 [details]
Patch

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

r- because of create() factory function and wrong std::unique_ptr<> use.

> Source/WebCore/platform/mac/PasteboardMac.mm:128
> +    return std::unique_ptr<Pasteboard>(new Pasteboard(pasteboardName));

We decided not to use create() factory function. Instead we have to use std::make_unique<> except for specific cases.

> Source/WebCore/platform/win/PasteboardWin.cpp:114
> +    return std::unique_ptr<Pasteboard>(new Pasteboard(dataObject.get()));

Use std::make_unique<> instead of std::unique_ptr<>
Comment 4 Joonghun Park 2014-11-24 02:03:20 PST
Created attachment 242151 [details]
<WIP>
Comment 5 Joonghun Park 2014-11-24 02:36:10 PST
Created attachment 242153 [details]
<WIP>
Comment 6 Joonghun Park 2014-11-24 06:42:12 PST
Created attachment 242157 [details]
Patch
Comment 7 Joonghun Park 2014-11-24 07:05:54 PST
Created attachment 242159 [details]
Patch
Comment 8 Gyuyoung Kim 2014-11-24 07:38:23 PST
Comment on attachment 242159 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:860
> +        std::unique_ptr<Pasteboard> pasteboard = Pasteboard::createForCopyAndPaste();

Why not using auto ?

> Source/WebCore/page/mac/EventHandlerMac.mm:684
> +    std::unique_ptr<Pasteboard> pasteboard = return std::make_unique<Pasteboard>(NSDragPboard);

return ??????????

> Source/WebCore/platform/win/PasteboardWin.cpp:95
> +    std::unique_ptr<Pasteboard> pasteboard = std::make_unique<Pasteboard>();

ditto.
Comment 9 Joonghun Park 2014-11-24 08:11:14 PST
Created attachment 242163 [details]
Patch
Comment 10 Joonghun Park 2014-11-24 08:28:58 PST
Created attachment 242164 [details]
Patch
Comment 11 Gyuyoung Kim 2014-11-24 17:47:12 PST
Comment on attachment 242164 [details]
Patch

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

> Source/WebCore/platform/Pasteboard.h:183
> +    static std::unique_ptr<Pasteboard> createForDragAndDrop();

There is a concern we support both public constructor and createFoo() factory function. Some reviewers think this is wrong behavior. So, we need to try to remove createFoo() factory function.
Comment 12 Darin Adler 2014-12-01 19:19:25 PST
Comment on attachment 242164 [details]
Patch

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

>> Source/WebCore/platform/Pasteboard.h:183
>> +    static std::unique_ptr<Pasteboard> createForDragAndDrop();
> 
> There is a concern we support both public constructor and createFoo() factory function. Some reviewers think this is wrong behavior. So, we need to try to remove createFoo() factory function.

That’s not quite right. We don’t to have create functions that simply wrap std::make_unique and add nothing further. But if the create function adds some additional value, then it’s fine to have it.
Comment 13 Darin Adler 2014-12-02 09:00:01 PST
Comment on attachment 242164 [details]
Patch

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

> Source/WebCore/platform/Pasteboard.h:147
> +    Pasteboard(PassRefPtr<DataObjectGtk>);
> +    Pasteboard(GtkClipboard*);

These should be marked explicit. Not sure why they weren’t already.
Comment 14 Joonghun Park 2014-12-02 10:31:18 PST
Created attachment 242425 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2014-12-03 19:02:30 PST
Comment on attachment 242425 [details]
Patch for landing

Clearing flags on attachment: 242425

Committed r176780: <http://trac.webkit.org/changeset/176780>
Comment 16 WebKit Commit Bot 2014-12-03 19:02:35 PST
All reviewed patches have been landed.  Closing bug.