Bug 79954 – IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from a shared worker

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

[David Grogan]

IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from a shared worker

[David Grogan]

Created attachment 129540 [details]
Patch

[David Grogan]

Created attachment 129551 [details]
Patch

[David Grogan]

Created attachment 129552 [details]
Patch

[David Grogan]

Created attachment 129555 [details]
Patch

[David Grogan]

This is the webkit side, there still needs to be an override of WebSharedWorkerClient::allowDatabase in WebSharedWorkerClientProxy::allowDatabase.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Joshua Bell]

Comment on attachment 129555 [details]
Patch

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

lgtm (once I recovered from those pre-existing names.... WebCommonWorkerClient? ugh...)

> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> +    virtual bool allowIndexedDB(const WebString& name)

This can be made pure virtual once the Chromium override is in place, correct?

[David Grogan]

(In reply to comment #7)
> > Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> > +    virtual bool allowIndexedDB(const WebString& name)
> 
> This can be made pure virtual once the Chromium override is in place, correct?

Correct.

[David Grogan]

Created attachment 129567 [details]
Patch

[David Grogan]

Michael, could you take a look at this?

[David Grogan]

Created attachment 129572 [details]
Patch

Move layout test stuff to separate patch

[David Grogan]

Created attachment 129573 [details]
Patch

[Michael Nordman]

Comment on attachment 129573 [details]
Patch

looks reasonable to me

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:58
> +    virtual bool allowIndexedDB(const WebString& name) = 0;

Please add the comment that clarifies which thread the new method is called on (similar to the others above).

> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
> +    virtual bool allowIndexedDB(const WebString& name)

I think if the chrome-side is committed first, you don't need to provide any overriden stubs here. Looks like pure virtual allow methods declared in the base class should be enough. I think they're only replicated on this interface (sometimes with a stub) to appease the two-sided-patch-landing gods.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:-124
> -        // webFrame is not deleted as long as the process is alive, relying on

yikes... good to get rid of this webFrame code, this comment is flat out incorrect given in-process dedicated workers

[David Grogan]

Comment on attachment 129573 [details]
Patch

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

>> Source/WebKit/chromium/public/WebCommonWorkerClient.h:58
>> +    virtual bool allowIndexedDB(const WebString& name) = 0;
> 
> Please add the comment that clarifies which thread the new method is called on (similar to the others above).

Done.

>> Source/WebKit/chromium/public/WebSharedWorkerClient.h:91
>> +    virtual bool allowIndexedDB(const WebString& name)
> 
> I think if the chrome-side is committed first, you don't need to provide any overriden stubs here. Looks like pure virtual allow methods declared in the base class should be enough. I think they're only replicated on this interface (sometimes with a stub) to appease the two-sided-patch-landing gods.

Good point.  I hadn't considered landing chrome-side first.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:-124
>> -        // webFrame is not deleted as long as the process is alive, relying on
> 
> yikes... good to get rid of this webFrame code, this comment is flat out incorrect given in-process dedicated workers

Agreed.  I wrote that comment but now I'm confused as to how I ever thought that.

[David Grogan]

Created attachment 129776 [details]
Patch

[David Grogan]

Tony, could you review this?

[Tony Chang]

Comment on attachment 129776 [details]
Patch

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

Seems fine, but please let fishd review the API change.

> Source/WebKit/chromium/ChangeLog:8
> +
> +        * public/WebCommonWorkerClient.h:

Where are the tests?  If this is covered on the chromium side, please mention which test covers it.

[David Grogan]

Created attachment 129964 [details]
Patch

[David Grogan]

(In reply to comment #17)
> View in context: https://bugs.webkit.org/attachment.cgi?id=129776&action=review
> 
> Seems fine, but please let fishd review the API change.
> 
> > Source/WebKit/chromium/ChangeLog:8
> > +
> > +        * public/WebCommonWorkerClient.h:
> 
> Where are the tests?  If this is covered on the chromium side, please mention which test covers it.

Sorry, I should have anticipated this.  Comment added in the ChangeLog file:

Tests: The 'allow' case will be tested once the patch at http://webkit.org/b/80189 and https://chromiumcodereview.appspot.com/9585031/ both land. Bug for testing the 'deny' case is http://crbug.com/113738.

[Michael Nordman]

Comment on attachment 129964 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:59
> +    virtual bool allowIndexedDB(const WebString& name) = 0;

About the webkit api.

I think we're moving away from pure virtuals in these interfaces in favor of empty stubs (so the two-sided patch landing dance has fewer steps), take a look at WebFrameClient.h for an example. With that in mind, i think it'd be good to provide inline stubs of the existing 3 methods and the new method being added.

Also, there's no reason to re-declare these same four methods in the derived WebSharedWorkerClient class. It'd be better to remove those re-declarations to eliminate the possibility of them getting out of sync.

[David Grogan]

Created attachment 129981 [details]
Patch

[David Grogan]

(In reply to comment #20)
> (From update of attachment 129964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129964&action=review
> 
> > Source/WebKit/chromium/public/WebCommonWorkerClient.h:59
> > +    virtual bool allowIndexedDB(const WebString& name) = 0;
> 
> About the webkit api.
> 
> I think we're moving away from pure virtuals in these interfaces in favor of empty stubs (so the two-sided patch landing dance has fewer steps), take a look at WebFrameClient.h for an example. With that in mind, i think it'd be good to provide inline stubs of the existing 3 methods and the new method being added.
> 
> Also, there's no reason to re-declare these same four methods in the derived WebSharedWorkerClient class. It'd be better to remove those re-declarations to eliminate the possibility of them getting out of sync.

Done and done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 129981 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:-50
> -    virtual bool allowDatabase(WebFrame*, const WebString& name, const WebString& displayName, unsigned long estimatedSize) = 0;

webkit API changes LGTM

[Tony Chang]

Comment on attachment 129981 [details]
Patch

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

> Source/WebKit/chromium/src/WebWorkerClientImpl.h:107
> +    virtual bool allowIndexedDB(const WebString& name);

Should we add OVERRIDE to these methods (separate patch)?

[David Grogan]

Comment on attachment 129981 [details]
Patch

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

>> Source/WebKit/chromium/src/WebWorkerClientImpl.h:107
>> +    virtual bool allowIndexedDB(const WebString& name);
> 
> Should we add OVERRIDE to these methods (separate patch)?

Looks like it.  Forthcoming.

[WebKit Review Bot]

Comment on attachment 129981 [details]
Patch

Clearing flags on attachment: 129981

Committed r109948: <http://trac.webkit.org/changeset/109948>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.