Bug 80061

Created attachment 129761 [details] Patch

Comment on attachment 129761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129761&action=review This modules thing really has worked out well, it seems. So much less crap in these core classes. > Source/WebCore/page/PageGroup.h:46 > + class PageGroup : public Supplementable<PageGroup> { Remind me what (if any) over head Suplementable adds to this object? No storage, correct?

Comment on attachment 129761 [details] Patch Missing new files...

Comment on attachment 129761 [details] Patch Sorry, my fault.

Created attachment 129763 [details] Patch

Comment on attachment 129763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129763&action=review > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND Is this a license block of Google?

Comment on attachment 129763 [details] Patch My drive-by review thirst is now quenched.

Comment on attachment 129763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129763&action=review > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:44 > +PageGroupIndexedDatabase* PageGroupIndexedDatabase::from(PageGroup& group) PassOwnPtr? > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:47 > + DEFINE_STATIC_LOCAL(AtomicString, name, ("PageGroupIndexedDatabase")); > + PageGroupIndexedDatabase* supplement = static_cast<PageGroupIndexedDatabase*>(Supplement<PageGroup>::from(&group, name)); One wonders why we don't just make these hashes static on the Suplement implementation, instead of keyed off of an AtomicString. > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.h:41 > + static PageGroupIndexedDatabase* from(PageGroup&); I'm surprised I didn't rag on you about using smart pointers for these from() functions earlier. Seems leaky w/o them...

> > Source/WebCore/page/PageGroup.h:46 > > + class PageGroup : public Supplementable<PageGroup> { > > Remind me what (if any) over head Suplementable adds to this object? No storage, correct? It adds one pointer, which is what we're removing in this patch. (Of course, there's only one PageGroup per process, so the amount of memory we're talking about is quite small.) > Is this a license block of Google? This code was written by Google, yes. (In reply to comment #8) > (From update of attachment 129763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129763&action=review > > > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:44 > > +PageGroupIndexedDatabase* PageGroupIndexedDatabase::from(PageGroup& group) > > PassOwnPtr? The type here is to patch the type returned by Page::group(), which is what all the callers of this function use. Normally we would use a pointer, but that made the call sites slightly less pretty. > > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:47 > > + DEFINE_STATIC_LOCAL(AtomicString, name, ("PageGroupIndexedDatabase")); > > + PageGroupIndexedDatabase* supplement = static_cast<PageGroupIndexedDatabase*>(Supplement<PageGroup>::from(&group, name)); > > One wonders why we don't just make these hashes static on the Suplement implementation, instead of keyed off of an AtomicString. I'm not sure I follow. > > Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.h:41 > > + static PageGroupIndexedDatabase* from(PageGroup&); > > I'm surprised I didn't rag on you about using smart pointers for these from() functions earlier. Seems leaky w/o them... The object is owned by PageGroup. In the implementation of from(..), you'll notice that we call adoptPtr and then given the owning reference to the PageGroup.

(In reply to comment #9) > > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND > > > Is this a license block of Google? > > This code was written by Google, yes. I meant, the licence block should be Google, but it appears you are using a licence template of Apple. ("THIS SOFTWARE IS PROVIDED BY APPLE INC.")

> I meant, the licence block should be Google, but it appears you are using a licence template of Apple. ("THIS SOFTWARE IS PROVIDED BY APPLE INC.") I see. Yes. Will fix.

Comment on attachment 129763 [details] Patch I did not mean to clear dglazkovs r+, sorry.

Comment on attachment 129763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129763&action=review >>> Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:44 >>> +PageGroupIndexedDatabase* PageGroupIndexedDatabase::from(PageGroup& group) >> >> PassOwnPtr? > > The type here is to patch the type returned by Page::group(), which is what all the callers of this function use. Normally we would use a pointer, but that made the call sites slightly less pretty. I see, the fact that you're adopting the pointer makes this OK. >>> Source/WebCore/Modules/indexeddb/PageGroupIndexedDatabase.cpp:47 >>> + PageGroupIndexedDatabase* supplement = static_cast<PageGroupIndexedDatabase*>(Supplement<PageGroup>::from(&group, name)); >> >> One wonders why we don't just make these hashes static on the Suplement implementation, instead of keyed off of an AtomicString. > > I'm not sure I follow. Remind me and we can discuss in person. Not particularly important.

Committed r109463: <http://trac.webkit.org/changeset/109463>