Bug 80061 – Remove last ENABLED(INDEXED_DATABASE) ifdef from WebCore proper

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove last ENABLED(INDEXED_DATABASE) ifdef from WebCore proper

[Adam Barth]

Created attachment 129761 [details]
Patch

[Eric Seidel (no email)]

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?

[Dimitri Glazkov (Google)]

Comment on attachment 129761 [details]
Patch

Missing new files...

[Eric Seidel (no email)]

Comment on attachment 129761 [details]
Patch

Sorry, my fault.

[Adam Barth]

Created attachment 129763 [details]
Patch

[Kentaro Hara]

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?

[Dimitri Glazkov (Google)]

Comment on attachment 129763 [details]
Patch

My drive-by review thirst is now quenched.

[Eric Seidel (no email)]

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

[Adam Barth]

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

[Kentaro Hara]

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

[Adam Barth]

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

[Eric Seidel (no email)]

Comment on attachment 129763 [details]
Patch

I did not mean to clear dglazkovs r+, sorry.

[Eric Seidel (no email)]

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.

[Adam Barth]

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