Bug 140983 – Rename pageCache() to PageCache::shared() and return a reference

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

Rename pageCache() to PageCache::shared() as this is a singleton class and have it return a reference instead of a pointer.

[Chris Dumez]

Created attachment 245515 [details]
Patch

[Andreas Kling]

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

[Chris Dumez]

Created attachment 245522 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 245522 [details]
Patch

Clearing flags on attachment: 245522

Committed r179247: <http://trac.webkit.org/changeset/179247>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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

[Darin Adler]

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.

[Chris Dumez]

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

[Darin Adler]

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

[Darin Adler]

Brief discussion would be maybe on webkit-dev?

[Brent Fulgham]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

I meant this thread: https://lists.webkit.org/pipermail/webkit-dev/2014-January/026068.html and bug127079 .

[Brent Fulgham]

Windows build fix landed in r179255. <http://trac.webkit.org/changeset/179255>

[Chris Dumez]

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

[Chris Dumez]

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

[Brent Fulgham]

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

[Brent Fulgham]

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

[Chris Dumez]

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

[Brent Fulgham]

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

[Chris Dumez]

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