Rename pageCache() to PageCache::shared() as this is a singleton class and have it return a reference instead of a pointer.
Created attachment 245515 [details] Patch
Comment on attachment 245515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245515&action=review r=me > Source/WebCore/history/PageCache.h:81 > + PageCache(); // Use instance() instead. You mean use shared() instead :-)
Created attachment 245522 [details] Patch
Comment on attachment 245522 [details] Patch Clearing flags on attachment: 245522 Committed r179247: <http://trac.webkit.org/changeset/179247>
All reviewed patches have been landed. Closing bug.
(In reply to comment #4) > Comment on attachment 245522 [details] > Patch > > Clearing flags on attachment: 245522 > > Committed r179247: <http://trac.webkit.org/changeset/179247> It broke the Windows build, as the EWS noticed it in time. 1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\include\private\wtf/NeverDestroyed.h(53): error C2248: 'WebCore::PageCache::PageCache' : cannot access private member declared in class 'WebCore::PageCache' (..\history\PageCache.cpp) c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\history\PageCache.h(81) : see declaration of 'WebCore::PageCache::PageCache' c:\cygwin\home\buildbot\slave\win-release\build\source\webcore\history\PageCache.h(43) : see declaration of 'WebCore::PageCache' ..\history\PageCache.cpp(297) : see reference to function template instantiation 'WTF::NeverDestroyed<WebCore::PageCache>::NeverDestroyed<>(void)' being compiled ..\history\PageCache.cpp(297) : see reference to function template instantiation 'WTF::NeverDestroyed<WebCore::PageCache>::NeverDestroyed<>(void)' being compiled
Changing to a reference is fantastic. However, changing from a free function to a member function isn’t something I’m so happy about. I don’t think that PageCache::shared() is a better name than pageCache(), and generally I don’t think that singletons need to use member functions.
(In reply to comment #7) > Changing to a reference is fantastic. However, changing from a free function > to a member function isn’t something I’m so happy about. I don’t think that > PageCache::shared() is a better name than pageCache(), and generally I don’t > think that singletons need to use member functions. Ok, I have no strong feelings about this. I made this change because I think this is more consistent with our recent code (e.g. In WK2). I'll upload a partial revert today.
(In reply to comment #8) > (In reply to comment #7) > > Changing to a reference is fantastic. However, changing from a free function > > to a member function isn’t something I’m so happy about. I don’t think that > > PageCache::shared() is a better name than pageCache(), and generally I don’t > > think that singletons need to use member functions. > > Ok, I have no strong feelings about this. I made this change because I think > this is more consistent with our recent code (e.g. In WK2). I'll upload a > partial revert today. If we are starting to go a particular direction with this, my personal taste alone should not be the driver. Maybe we should have a brief discussion. More urgently, though, the Windows build is still broken!
Brief discussion would be maybe on webkit-dev?
This seems like a race between commit-queue and ews. The Windows bot was about 15 minutes slower than the mac ews. Once the reviewer marked it as cq+, it got applied and by the time the Windows ews noted the problem we were already sunk.
(In reply to comment #11) > This seems like a race between commit-queue and ews. The Windows bot was > about 15 minutes slower than the mac ews. Once the reviewer marked it as > cq+, it got applied and by the time the Windows ews noted the problem we > were already sunk. That's why it was good to let the EWS set cq- and comment bug on failure. But unfortunately it was stated SPAMs and disabled long time ago. :( Now the only way to be informed about failures is polling bugzilla manually ...
I meant this thread: https://lists.webkit.org/pipermail/webkit-dev/2014-January/026068.html and bug127079 .
Windows build fix landed in r179255. <http://trac.webkit.org/changeset/179255>
(In reply to comment #14) > Windows build fix landed in r179255. > <http://trac.webkit.org/changeset/179255> My patch already marked NeverDestroyed<PageCache> as a friend class of PageCache. Now you added it again so it is marked twice?
(In reply to comment #15) > (In reply to comment #14) > > Windows build fix landed in r179255. > > <http://trac.webkit.org/changeset/179255> > > My patch already marked NeverDestroyed<PageCache> as a friend class of > PageCache. Now you added it again so it is marked twice? - Mine is public, yours is private. AFAIK, this doesn't matter for friend class statements. - Your specifies WTF:: explicitly but this shouldn't matter either. StringImpl has the following statement for e.g. "friend class NeverDestroyed<StringImpl>;", and it builds fine on Windows. I am not quite sure what's going on.
(In reply to comment #15) > (In reply to comment #14) > > Windows build fix landed in r179255. > > <http://trac.webkit.org/changeset/179255> > > My patch already marked NeverDestroyed<PageCache> as a friend class of > PageCache. Now you added it again so it is marked twice? Oh! The problem must have been namespacing. You declared "WebCore::NeverDestroyed<PageCache>" as a friend, which is a nonexistent class. It needed to be declared as "WTF::NeverDestroyed<PageCache>". Can you delete the original incorrect declaration?
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > Windows build fix landed in r179255. > > > <http://trac.webkit.org/changeset/179255> > > > > My patch already marked NeverDestroyed<PageCache> as a friend class of > > PageCache. Now you added it again so it is marked twice? > > - Mine is public, yours is private. AFAIK, this doesn't matter for friend > class statements. > - Your specifies WTF:: explicitly but this shouldn't matter either. > StringImpl has the following statement for e.g. "friend class > NeverDestroyed<StringImpl>;", and it builds fine on Windows. StringImpl is in namespace WTF.
(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > Windows build fix landed in r179255. > > > > <http://trac.webkit.org/changeset/179255> > > > > > > My patch already marked NeverDestroyed<PageCache> as a friend class of > > > PageCache. Now you added it again so it is marked twice? > > > > - Mine is public, yours is private. AFAIK, this doesn't matter for friend > > class statements. > > - Your specifies WTF:: explicitly but this shouldn't matter either. > > StringImpl has the following statement for e.g. "friend class > > NeverDestroyed<StringImpl>;", and it builds fine on Windows. > > StringImpl is in namespace WTF. There are examples outside WTF namespace: Source/WebKit2/NetworkProcess/NetworkProcess.h: friend class NeverDestroyed<NetworkProcess>; Maybe this works if wtf/NeverDestroyed.h is included in the header.
(In reply to comment #19) > > > > StringImpl is in namespace WTF. > > There are examples outside WTF namespace: > Source/WebKit2/NetworkProcess/NetworkProcess.h: friend class > NeverDestroyed<NetworkProcess>; > > Maybe this works if wtf/NeverDestroyed.h is included in the header. Right. Because wtf/NeverDestroyed.h provides a using declaration: } // namespace WTF; using WTF::LazyNeverDestroyed; using WTF::NeverDestroyed; #endif // NeverDestroyed_h So anyone including this header can just say NeverDestroyed, just like you don't have to say "std::string" everywhere if you say "using std::string".
(In reply to comment #17) > (In reply to comment #15) > > (In reply to comment #14) > > > Windows build fix landed in r179255. > > > <http://trac.webkit.org/changeset/179255> > > > > My patch already marked NeverDestroyed<PageCache> as a friend class of > > PageCache. Now you added it again so it is marked twice? > > Oh! The problem must have been namespacing. You declared > "WebCore::NeverDestroyed<PageCache>" as a friend, which is a nonexistent > class. > > It needed to be declared as "WTF::NeverDestroyed<PageCache>". > > Can you delete the original incorrect declaration? Done, <http://trac.webkit.org/changeset/179262>.