Bug 50606 – WebKit2: Need a WebKit2 equivalent of the WebKit1 WebDatabaseManager

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

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>

[Jessie Berlin]

Created attachment 76147 [details]
Patch

[Adam Roben (:aroben)]

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?

[Jessie Berlin]

(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());

[Adam Roben (:aroben)]

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!

[Sam Weinig]

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

[Jessie Berlin]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release

[Jessie Berlin]

(In reply to comment #7)
> http://trac.webkit.org/changeset/73808 might have broken Qt Linux Release

Attempting to fix now.

[Jessie Berlin]

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

[Jessie Berlin]

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.