Bug 139019

Summary:

Use std::unique_ptr instead of PassOwnPtr|OwnPtr for Pasteboard

Product:

WebKit

Reporter:

Joonghun Park <jh718.park>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, gyuyoung.kim

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

128007

Attachments:

Description	Flags
Patch	none
Patch	none
<WIP>	none
<WIP>	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Joonghun Park

Reported 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).

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Joonghun Park

Comment 1 2014-11-23 22:18:14 PST

Created attachment 242144 [details] Patch

Joonghun Park

Comment 2 2014-11-23 23:10:45 PST

Created attachment 242146 [details] Patch

Gyuyoung Kim

Comment 3 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<>

Joonghun Park

Comment 4 2014-11-24 02:03:20 PST

Created attachment 242151 [details] <WIP>

Joonghun Park

Comment 5 2014-11-24 02:36:10 PST

Created attachment 242153 [details] <WIP>

Joonghun Park

Comment 6 2014-11-24 06:42:12 PST

Created attachment 242157 [details] Patch

Joonghun Park

Comment 7 2014-11-24 07:05:54 PST

Created attachment 242159 [details] Patch

Gyuyoung Kim

Comment 8 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.

Joonghun Park

Comment 9 2014-11-24 08:11:14 PST

Created attachment 242163 [details] Patch

Joonghun Park

Comment 10 2014-11-24 08:28:58 PST

Created attachment 242164 [details] Patch

Gyuyoung Kim

Comment 11 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.

Darin Adler

Comment 12 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.

Darin Adler

Comment 13 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.

Joonghun Park

Comment 14 2014-12-02 10:31:18 PST

Created attachment 242425 [details] Patch for landing

WebKit Commit Bot

Comment 15 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>

WebKit Commit Bot

Comment 16 2014-12-03 19:02:35 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.