Bug 141950

Created attachment 247198 [details] Patch

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

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

Created attachment 247299 [details] Patch

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

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 ?

Created attachment 247308 [details] Patch

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.

Created attachment 247399 [details] Patch

Created attachment 247403 [details] Patch

Created attachment 247405 [details] Patch

Created attachment 247407 [details] Patch

Created attachment 247412 [details] Patch

Created attachment 247413 [details] Patch

Created attachment 247414 [details] Patch

Created attachment 247415 [details] Patch

Created attachment 247416 [details] Patch

Created attachment 247418 [details] Patch

Created attachment 247419 [details] Patch

Created attachment 247420 [details] Patch

Created attachment 247594 [details] Patch

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

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

Created attachment 251424 [details] Patch for landing

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

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 on attachment 251424 [details] Patch for landing Clearing flags on attachment: 251424 Committed r183186: <http://trac.webkit.org/changeset/183186>

All reviewed patches have been landed. Closing bug.