Bug 141950 – Use std::unique_ptr instead of OwnPtr in ThreadGlobalData

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joonghun Park]

In ThreadGlobalData, change OwnPtr to std::unique_ptr.

[Joonghun Park]

Created attachment 247198 [details]
Patch

[Gyuyoung Kim]

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.

[Darin Adler]

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.

[Anders Carlsson]

(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.

[Joonghun Park]

Created attachment 247299 [details]
Patch

[Joonghun Park]

According to the comments Darin and Anders made, I uploaded a revised patch.
Please take a look into it. :)

[Gyuyoung Kim]

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 ?

[Joonghun Park]

Created attachment 247308 [details]
Patch

[Darin Adler]

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.

[Joonghun Park]

Created attachment 247399 [details]
Patch

[Joonghun Park]

Created attachment 247403 [details]
Patch

[Joonghun Park]

Created attachment 247405 [details]
Patch

[Joonghun Park]

Created attachment 247407 [details]
Patch

[Joonghun Park]

Created attachment 247412 [details]
Patch

[Joonghun Park]

Created attachment 247413 [details]
Patch

[Joonghun Park]

Created attachment 247414 [details]
Patch

[Joonghun Park]

Created attachment 247415 [details]
Patch

[Joonghun Park]

Created attachment 247416 [details]
Patch

[Joonghun Park]

Created attachment 247418 [details]
Patch

[Joonghun Park]

Created attachment 247419 [details]
Patch

[Joonghun Park]

Created attachment 247420 [details]
Patch

[Joonghun Park]

Created attachment 247594 [details]
Patch

[Joonghun Park]

(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?

[Darin Adler]

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.

[Darin Adler]

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!

[WebKit Commit Bot]

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

[Darin Adler]

We should find a way to land this soon. These are among the last remaining uses of OwnPtr now!

[Gyuyoung Kim]

Created attachment 251424 [details]
Patch for landing

[Gyuyoung Kim]

(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.

[Joonghun Park]

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 :)

[WebKit Commit Bot]

Comment on attachment 251424 [details]
Patch for landing

Clearing flags on attachment: 251424

Committed r183186: <http://trac.webkit.org/changeset/183186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.