Bug 141950 - Use std::unique_ptr instead of OwnPtr in ThreadGlobalData
Summary: Use std::unique_ptr instead of OwnPtr in ThreadGlobalData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks: 128007
  Show dependency treegraph
 
Reported: 2015-02-23 21:09 PST by Joonghun Park
Modified: 2015-04-23 08:53 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.25 KB, patch)
2015-02-23 21:12 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.05 KB, patch)
2015-02-24 20:04 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2015-02-24 21:01 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.00 KB, patch)
2015-02-25 20:18 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2015-02-25 21:30 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2015-02-25 22:01 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.18 KB, patch)
2015-02-25 22:45 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.17 KB, patch)
2015-02-26 00:27 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (6.80 KB, patch)
2015-02-26 01:05 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2015-02-26 01:53 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2015-02-26 01:59 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2015-02-26 02:05 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (6.29 KB, patch)
2015-02-26 02:52 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (6.18 KB, patch)
2015-02-26 02:56 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2015-02-26 03:14 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2015-02-27 23:57 PST, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch for landing (5.56 KB, patch)
2015-04-23 03:09 PDT, Gyuyoung Kim
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 2015-02-23 21:09:44 PST
In ThreadGlobalData, change OwnPtr to std::unique_ptr.
Comment 1 Joonghun Park 2015-02-23 21:12:37 PST
Created attachment 247198 [details]
Patch
Comment 2 Gyuyoung Kim 2015-02-24 00:52:00 PST
Comment on attachment 247198 [details]
Patch

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

LGTM except for my question.

> Source/WebCore/dom/EventNames.h:310
> +    // Do not call EventNames() instead of eventNames()

I want to get other reviewer comment about this moving.
Comment 3 Darin Adler 2015-02-24 09:06:20 PST
Comment on attachment 247198 [details]
Patch

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

> Source/WebCore/dom/EventNames.h:311
> +    EventNames();

Anders, what should we do about this? We went out of our way to make this constructor private so we would not accidentally call it rather than the global function. Making it public seems unfortunate. Is there some other technique to make make_unique work, or no?

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:35
> +    CachedResourceRequestInitiators();

This one seems like less of a problem. Doesn’t seem likely someone would instantiate it by accident.
Comment 4 Anders Carlsson 2015-02-24 09:09:54 PST
(In reply to comment #3)
> Comment on attachment 247198 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247198&action=review
> 
> > Source/WebCore/dom/EventNames.h:311
> > +    EventNames();
> 
> Anders, what should we do about this? We went out of our way to make this
> constructor private so we would not accidentally call it rather than the
> global function. Making it public seems unfortunate. Is there some other
> technique to make make_unique work, or no?
> 

I suppose we could make std::make_unique be a friend of the class? Something like:

template<typename T, typename... Args>
friend std::unique_ptr<T> make_unique(Args&&...);

> > Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:35
> > +    CachedResourceRequestInitiators();
> 
> This one seems like less of a problem. Doesn’t seem likely someone would
> instantiate it by accident.
Comment 5 Joonghun Park 2015-02-24 20:04:31 PST
Created attachment 247299 [details]
Patch
Comment 6 Joonghun Park 2015-02-24 20:07:39 PST
According to the comments Darin and Anders made, I uploaded a revised patch.
Please take a look into it. :)
Comment 7 Gyuyoung Kim 2015-02-24 20:15:56 PST
Comment on attachment 247299 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:35
> +    CachedResourceRequestInitiators();

> This one seems like less of a problem. Doesn’t seem likely someone would instantiate it by accident.

Darin said like that. So how about removing it ?
Comment 8 Joonghun Park 2015-02-24 21:01:25 PST
Created attachment 247308 [details]
Patch
Comment 9 Darin Adler 2015-02-25 15:29:34 PST
Comment on attachment 247308 [details]
Patch

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

> Source/WebCore/dom/EventNames.h:310
> +    template<class T, class... Args>
> +    friend typename std::_Unique_if<T>::_Single_object std::make_unique(Args&&...);

Seems that this does not compile on Windows.
Comment 10 Joonghun Park 2015-02-25 20:18:00 PST
Created attachment 247399 [details]
Patch
Comment 11 Joonghun Park 2015-02-25 21:30:39 PST
Created attachment 247403 [details]
Patch
Comment 12 Joonghun Park 2015-02-25 22:01:17 PST
Created attachment 247405 [details]
Patch
Comment 13 Joonghun Park 2015-02-25 22:45:44 PST
Created attachment 247407 [details]
Patch
Comment 14 Joonghun Park 2015-02-26 00:27:20 PST
Created attachment 247412 [details]
Patch
Comment 15 Joonghun Park 2015-02-26 01:05:52 PST
Created attachment 247413 [details]
Patch
Comment 16 Joonghun Park 2015-02-26 01:53:13 PST
Created attachment 247414 [details]
Patch
Comment 17 Joonghun Park 2015-02-26 01:59:45 PST
Created attachment 247415 [details]
Patch
Comment 18 Joonghun Park 2015-02-26 02:05:26 PST
Created attachment 247416 [details]
Patch
Comment 19 Joonghun Park 2015-02-26 02:52:33 PST
Created attachment 247418 [details]
Patch
Comment 20 Joonghun Park 2015-02-26 02:56:34 PST
Created attachment 247419 [details]
Patch
Comment 21 Joonghun Park 2015-02-26 03:14:15 PST
Created attachment 247420 [details]
Patch
Comment 22 Joonghun Park 2015-02-27 23:57:13 PST
Created attachment 247594 [details]
Patch
Comment 23 Joonghun Park 2015-02-27 23:59:17 PST
(In reply to comment #9)
> Comment on attachment 247308 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247308&action=review
> 
> > Source/WebCore/dom/EventNames.h:310
> > +    template<class T, class... Args>
> > +    friend typename std::_Unique_if<T>::_Single_object std::make_unique(Args&&...);
> 
> Seems that this does not compile on Windows.

Because of that problem, I revised the patch. Would you please review this change?
Comment 24 Darin Adler 2015-04-19 22:20:07 PDT
Comment on attachment 247594 [details]
Patch

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

> Source/WebCore/dom/EventNames.h:316
> +    // FIXME: The friend declaration to std::make_unique below does not work in windows port.

Windows should have a capital "W".

> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:35
> +    CachedResourceRequestInitiators();

We want this to stay private for the same reason as EventNames(); it’s too easy to use it wrong by accidentally capitalizing the function name, so we need it to be private to prevent that kind of accident.
Comment 25 Darin Adler 2015-04-20 09:58:39 PDT
Comment on attachment 247594 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequestInitiators.h:35
>> +    CachedResourceRequestInitiators();
> 
> We want this to stay private for the same reason as EventNames(); it’s too easy to use it wrong by accidentally capitalizing the function name, so we need it to be private to prevent that kind of accident.

But really what we want it to get rid of this over-engineered collection of three strings entirely!
Comment 26 WebKit Commit Bot 2015-04-20 10:00:21 PDT
Comment on attachment 247594 [details]
Patch

Rejecting attachment 247594 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 247594, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
atching file Source/WebCore/dom/EventNames.h
Hunk #1 FAILED at 313.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/dom/EventNames.h.rej
patching file Source/WebCore/loader/cache/CachedResourceRequestInitiators.h
patching file Source/WebCore/platform/ThreadGlobalData.cpp
patching file Source/WebCore/platform/ThreadGlobalData.h

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6556678946291712
Comment 27 Darin Adler 2015-04-23 00:47:20 PDT
We should find a way to land this soon. These are among the last remaining uses of OwnPtr now!
Comment 28 Gyuyoung Kim 2015-04-23 03:09:36 PDT
Created attachment 251424 [details]
Patch for landing
Comment 29 Gyuyoung Kim 2015-04-23 03:10:16 PDT
(In reply to comment #27)
> We should find a way to land this soon. These are among the last remaining
> uses of OwnPtr now!

I rebased this patch instead of Joonghun.
Comment 30 Joonghun Park 2015-04-23 07:35:56 PDT
Oh, thank you for your help, Gyuyoung! And thank you for your review, Darin :) Me and My macbook was in genius bar of Shinsaibashi, Osaka. Sorry for my late answer :)
Comment 31 WebKit Commit Bot 2015-04-23 08:53:30 PDT
Comment on attachment 251424 [details]
Patch for landing

Clearing flags on attachment: 251424

Committed r183186: <http://trac.webkit.org/changeset/183186>
Comment 32 WebKit Commit Bot 2015-04-23 08:53:37 PDT
All reviewed patches have been landed.  Closing bug.