Bug 50606 - WebKit2: Need a WebKit2 equivalent of the WebKit1 WebDatabaseManager
Summary: WebKit2: Need a WebKit2 equivalent of the WebKit1 WebDatabaseManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jessie Berlin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-12-06 17:38 PST by Jessie Berlin
Modified: 2010-12-10 15:45 PST (History)
7 users (show)

See Also:


Attachments
Patch (47.16 KB, patch)
2010-12-09 17:54 PST, Jessie Berlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2010-12-06 17:38:52 PST
WebKit2 needs to expose methods to get a list of origins for which there are databases, delete the databases for a given origin, and delete all the databases.

In WebKit1, these methods are exposed on WebDatabaseManager.

<rdar://problem/8706731>
Comment 1 Jessie Berlin 2010-12-09 17:54:01 PST
Created attachment 76147 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-12-10 06:23:35 PST
Comment on attachment 76147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review

Is it possible to add some API tests for this?

You should probably have Sam give the new API a once-over.

> WebKit2/DerivedSources.make:28
> +    $(WebKit2)/WebProcess/WebCoreSupport \

Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we?

> WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:64
> +    m_databaseOriginsCallbacks.set(callbackID, callback.get());

You can use callback.release() here to save a ref/deref.

> WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81
> +        securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef());

Who is in charge of dereffing these objects?
Comment 3 Jessie Berlin 2010-12-10 10:52:04 PST
(In reply to comment #2)
> (From update of attachment 76147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review
> 
> Is it possible to add some API tests for this?

Probably, is it ok to do that in a subsequent patch?

> 
> You should probably have Sam give the new API a once-over.

We talked about it earlier, but I think he is giving it another look now.

> 
> > WebKit2/DerivedSources.make:28
> > +    $(WebKit2)/WebProcess/WebCoreSupport \
> 
> Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we?

The WebDatabaseManager.messages.in is in WebCoreSupport. Do you think I should make a separate directory / filter for Databases  and move all of the WebDatabaseManager stuff in there?

> 
> > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:64
> > +    m_databaseOriginsCallbacks.set(callbackID, callback.get());
> 
> You can use callback.release() here to save a ref/deref.

Done.

> 
> > WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81
> > +        securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef());
> 
> Who is in charge of dereffing these objects?

Changed to:

    size_t originIdentifiersCount = originIdentifiers.size();
    Vector<RefPtr<APIObject> > securityOrigins(originIdentifiersCount);

    for (size_t i = 0; i < originIdentifiersCount; ++i)
        securityOrigins[i] = WebSecurityOrigin::create(originIdentifiers[i]);

    callback->performCallbackWithReturnValue(ImmutableArray::adopt(securityOrigins).get());
Comment 4 Adam Roben (:aroben) 2010-12-10 10:54:28 PST
Comment on attachment 76147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review

>>> WebKit2/DerivedSources.make:28
>>> +    $(WebKit2)/WebProcess/WebCoreSupport \
>> 
>> Is this really needed? We don't have any .messages.in files in WebCoreSupport, do we?
> 
> The WebDatabaseManager.messages.in is in WebCoreSupport. Do you think I should make a separate directory / filter for Databases  and move all of the WebDatabaseManager stuff in there?

It probably doesn't matter very much. I don't feel bothered enough by it to have you change it.

>>> WebKit2/UIProcess/WebDatabaseManagerProxy.cpp:81
>>> +        securityOrigins->append(WebSecurityOrigin::create(originIdentifiers[i]).leakRef());
>> 
>> Who is in charge of dereffing these objects?
> 
> Changed to:
> 
>     size_t originIdentifiersCount = originIdentifiers.size();
>     Vector<RefPtr<APIObject> > securityOrigins(originIdentifiersCount);
> 
>     for (size_t i = 0; i < originIdentifiersCount; ++i)
>         securityOrigins[i] = WebSecurityOrigin::create(originIdentifiers[i]);
> 
>     callback->performCallbackWithReturnValue(ImmutableArray::adopt(securityOrigins).get());

Beautiful!
Comment 5 Sam Weinig 2010-12-10 13:57:31 PST
Comment on attachment 76147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review

> WebKit2/UIProcess/WebDatabaseManagerProxy.h:47
> +typedef GenericCallback<WKArrayRef, ImmutableArray*> DatabaseOriginsCallback;

I don't think the ImmutableArray* type is needed here. It should be inferred.

> WebKit2/UIProcess/WebDatabaseManagerProxy.h:76
> +    HashMap<uint64_t, RefPtr<DatabaseOriginsCallback> > m_databaseOriginsCallbacks;
> +
> +};
> +

Extra newline.

> WebKit2/UIProcess/WebPageProxy.h:467
> +template<typename T>
> +void invalidateCallbackMap(HashMap<uint64_t, T>& map)
> +{
> +    Vector<T> callbacksVector;
> +    copyValuesToVector(map, callbacksVector);
> +    for (size_t i = 0, size = callbacksVector.size(); i < size; ++i)
> +        callbacksVector[i]->invalidate();
> +    map.clear();
> +}
> +
>  } // namespace WebKit
>  

Let's move this to GenericCallback.h
Comment 6 Jessie Berlin 2010-12-10 14:39:04 PST
(In reply to comment #5)
> (From update of attachment 76147 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76147&action=review
> 
> > WebKit2/UIProcess/WebDatabaseManagerProxy.h:47
> > +typedef GenericCallback<WKArrayRef, ImmutableArray*> DatabaseOriginsCallback;
> 
> I don't think the ImmutableArray* type is needed here. It should be inferred.

Removed.

> 
> > WebKit2/UIProcess/WebDatabaseManagerProxy.h:76
> > +    HashMap<uint64_t, RefPtr<DatabaseOriginsCallback> > m_databaseOriginsCallbacks;
> > +
> > +};
> > +
> 
> Extra newline.

Removed.

> 
> > WebKit2/UIProcess/WebPageProxy.h:467
> > +template<typename T>
> > +void invalidateCallbackMap(HashMap<uint64_t, T>& map)
> > +{
> > +    Vector<T> callbacksVector;
> > +    copyValuesToVector(map, callbacksVector);
> > +    for (size_t i = 0, size = callbacksVector.size(); i < size; ++i)
> > +        callbacksVector[i]->invalidate();
> > +    map.clear();
> > +}
> > +
> >  } // namespace WebKit
> >  
> 
> Let's move this to GenericCallback.h

Done.
Comment 7 WebKit Review Bot 2010-12-10 15:23:54 PST
http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release
Comment 8 Jessie Berlin 2010-12-10 15:31:04 PST
(In reply to comment #7)
> http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release

Attempting to fix now.
Comment 9 Jessie Berlin 2010-12-10 15:44:19 PST
Comment on attachment 76147 [details]
Patch

Committed in r73808
http://trac.webkit.org/changeset/73808
Qt build fix landed in r73814
http://trac.webkit.org/changeset/73814
Comment 10 Jessie Berlin 2010-12-10 15:45:54 PST
I will add tests in a subsequent patch. There needs to be some work done so that deleteAllDatabases in the tests does not delete all the databases for the person running the tests.