IndexedDB: Check WebCommonWorkerClient::allowIndexedDB before proceeding from a shared worker
Created attachment 129540 [details] Patch
Created attachment 129551 [details] Patch
Created attachment 129552 [details] Patch
Created attachment 129555 [details] Patch
This is the webkit side, there still needs to be an override of WebSharedWorkerClient::allowDatabase in WebSharedWorkerClientProxy::allowDatabase.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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?
(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.
Created attachment 129567 [details] Patch
Michael, could you take a look at this?
Created attachment 129572 [details] Patch Move layout test stuff to separate patch
Created attachment 129573 [details] Patch
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
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.
Created attachment 129776 [details] Patch
Tony, could you review this?
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.
Created attachment 129964 [details] Patch
(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.
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.
Created attachment 129981 [details] Patch
(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.
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
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)?
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.
Comment on attachment 129981 [details] Patch Clearing flags on attachment: 129981 Committed r109948: <http://trac.webkit.org/changeset/109948>
All reviewed patches have been landed. Closing bug.