Bug 112713 - [chromium] Support Quota API in Worker in WebKit API
Summary: [chromium] Support Quota API in Worker in WebKit API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on:
Blocks: 112972
  Show dependency treegraph
 
Reported: 2013-03-19 10:24 PDT by Kinuko Yasuda
Modified: 2013-03-22 12:53 PDT (History)
14 users (show)

See Also:


Attachments
Patch (25.77 KB, patch)
2013-03-19 22:15 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (40.53 KB, patch)
2013-03-20 17:39 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (42.70 KB, patch)
2013-03-21 14:06 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 (959.84 KB, application/zip)
2013-03-21 14:54 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 (504.13 KB, application/zip)
2013-03-21 15:07 PDT, Build Bot
no flags Details
Patch (26.25 KB, patch)
2013-03-21 15:10 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (26.22 KB, patch)
2013-03-21 18:34 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (26.39 KB, patch)
2013-03-22 10:38 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2013-03-19 10:24:53 PDT
We need worker-specific code in the platform layer.
Comment 1 Alec Flett 2013-03-19 22:15:23 PDT
Created attachment 193983 [details]
Patch
Comment 2 Alec Flett 2013-03-19 22:17:30 PDT
Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko. Most of this is cribbed from the FileSystem stuff. 

Tests coming up tomorrow.

There's one "XXX" in here about moving stuff into Platform/chromium - not sure what the best course of action is, but I'm CC'ing pilgrim@ just in case.

See also http://crbug.com/88490 for the chromium side.
Comment 3 WebKit Review Bot 2013-03-19 22:24:40 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Adam Barth 2013-03-19 22:49:45 PDT
Comment on attachment 193983 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:37
> +// XXX need to move this to Platform?

We usually use FIXME rather than XXX

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:54
>  namespace WebCore {

Yeah, it looks like some of this code needs to move into WebCore.  That's not something we need to fix in this patch, however.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:58
> +namespace {

We usually use the "static" keyword rather than anonymous namespaces.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:74
> +        // m_bridge->didGrandStorageQuotaOnMainThread(grantedQuotaInBytes);

??

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:139
> +        delete observer;

:(

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.h:71
> +class WorkerStorageQuotaCallbacksBridge : public ThreadSafeRefCounted<WorkerStorageQuotaCallbacksBridge> {

Why is this all so complicated?
Comment 5 Adam Barth 2013-03-19 22:51:19 PDT
> Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko.

Oh, sorry.  I thought this patch was further along.

The patch seems like it has way too much machinery for what it's trying to accomplish.  Maybe we should use some of our more modern threading primitives?
Comment 6 Kinuko Yasuda 2013-03-20 00:02:50 PDT
Comment on attachment 193983 [details]
Patch

I'll take another look tomorrow but I think the code looks good. Thanks for doing this quickly!

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:39
> +#include "WebStorageQuotaType.h"

I think we're expected to move them, but I hope it can come later in a separate patch.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:65
> +    WebCore::WorkerLoaderProxy* workerLoaderProxy =  &workerThread->workerLoaderProxy();

nit: extra space before &workerThread

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:47
> +// FileSystemCallbacks that are to be dispatched on the main thread.

nit; FileSystemCallbacks -> WebStorageQuotaCallbacks

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:74
> +        // m_bridge->didGrandStorageQuotaOnMainThread(grantedQuotaInBytes);

Let's remove this commented-out line after we make sure it's not mandatory to support this in Workers.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:143
> +// FIXME: implement this. I think we can just follow the way how WorkerFileSystemCallbacksBridge.cpp is doing, though it's kinda complicated..

Oops, can you remove this line... thanks!
Comment 7 Kinuko Yasuda 2013-03-20 00:06:44 PDT
(In reply to comment #5)
> > Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko.
> 
> Oh, sorry.  I thought this patch was further along.
> 
> The patch seems like it has way too much machinery for what it's trying to accomplish.  Maybe we should use some of our more modern threading primitives?

I agree this is too complex compared to what it's trying to achieve, while basically this one tries to follow the existing method so that we can be sure it works.  Because of the urgency we may not be able to re-implement this in the first patch but can you let us know about the modern threading primitives?
Comment 8 Kinuko Yasuda 2013-03-20 08:13:27 PDT
Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.

Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).

Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)

abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!
Comment 9 Adam Barth 2013-03-20 11:09:03 PDT
(In reply to comment #8)
> Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> 
> Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).

That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.

> Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)

I don't think we need to block this patch on finding a simpler design, but I suspect we can use WeakPtr and WTF::Functional to handle the shutdown case more gracefully.  It's likely we should apply those primitives to many of the threading operations in WebCore.

> abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!

I think it's fine to move forward with the existing tools and then think about how to improve this pattern across the codebase in follow-on work.
Comment 10 Kinuko Yasuda 2013-03-20 11:20:52 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > 
> > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> 
> That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.
> 
> > Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)
> 
> I don't think we need to block this patch on finding a simpler design, but I suspect we can use WeakPtr and WTF::Functional to handle the shutdown case more gracefully.  It's likely we should apply those primitives to many of the threading operations in WebCore.
>
> > abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!
> 
> I think it's fine to move forward with the existing tools and then think about how to improve this pattern across the codebase in follow-on work.

Thanks!  Looks like today we have a lot more useful primitives in WTF/WebCore (man, I didn't know we have WeakPtr now!!), we'd love to cleanup the code in follow-up patches (unless Alec wants take a shot now).
Comment 11 Mark Pilgrim (Google) 2013-03-20 11:26:00 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > 
> > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> 
> That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.

Implementation in render_view_impl.cc suggests that the WebFrame parameter is only used to get the security origin to check if it is unique, since unique origins can not store persistent state. This is a prevalent antipattern that I'm currently in the process of cleaning up in another (unrelated) bug. The fix is to change the WebFrame* parameter to a WebString that is the string serialization of the security origin of the document of the web frame, and change the implementation in Chromium to match.
Comment 12 Kinuko Yasuda 2013-03-20 11:35:38 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > > 
> > > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> > 
> > That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.
> 
> Implementation in render_view_impl.cc suggests that the WebFrame parameter is only used to get the security origin to check if it is unique, since unique origins can not store persistent state. This is a prevalent antipattern that I'm currently in the process of cleaning up in another (unrelated) bug. The fix is to change the WebFrame* parameter to a WebString that is the string serialization of the security origin of the document of the web frame, and change the implementation in Chromium to match.

Oh oh... yes now I remember I've seen your email about the cleanup.  So once we do that we can just move them to Platform. Let us see if we want to do the change now or just continue finishing this patch.  If we go the latter way we'd love to make the cleanup change later.
Comment 13 Alec Flett 2013-03-20 17:39:26 PDT
Created attachment 194150 [details]
Patch
Comment 14 Alec Flett 2013-03-20 17:40:43 PDT
And here's the patch with a working webcore, and a layout test.

The only issue left here is that shared workers are not working (the request is going in and the callback is firing, but not being dispatched to the JS callback) - dedicated workers work just fine. This may be something on the chromium side that I need to investigate.
Comment 15 Build Bot 2013-03-20 18:32:23 PDT
Comment on attachment 194150 [details]
Patch

Attachment 194150 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17240217

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html
Comment 16 WebKit Review Bot 2013-03-20 18:47:26 PDT
Comment on attachment 194150 [details]
Patch

Attachment 194150 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17183731

New failing tests:
fast/workers/worker-storagequota-query-usage.html
svg/as-image/image-preserveAspectRatio-all.svg
svg/filters/feImage-preserveAspectRatio-all.svg
fast/workers/shared-worker-storagequota-query-usage.html
Comment 17 Kinuko Yasuda 2013-03-20 20:17:40 PDT
Comment on attachment 194150 [details]
Patch

When you upload the final patch(es) I think we'd better file another bug for WebCore (e.g. "Expose StorageQuota interface on Workers") and split this into two: WebCore one and chromium-specific one.

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

> Source/WebCore/WebCore.gypi:178
> +            'Modules/quota/NavigatorStorageQuota.idl',

not needed?

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:154
> +

nit: extra empty line

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks

Can we update this FIXME to:
// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota with WebString and move the method to Platform so that we can remove all these complexity for Worker.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:89
> +

nit: Extra empty lines
Comment 18 Alec Flett 2013-03-21 14:06:46 PDT
Created attachment 194332 [details]
Patch
Comment 19 Alec Flett 2013-03-21 14:07:29 PDT
here's the final unified patch, with working tests. I'm now splitting it up into WebKit vs WebCore.
Comment 20 WebKit Review Bot 2013-03-21 14:54:18 PDT
Comment on attachment 194332 [details]
Patch

Attachment 194332 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17247281

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html
Comment 21 WebKit Review Bot 2013-03-21 14:54:21 PDT
Created attachment 194353 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 22 Alec Flett 2013-03-21 15:04:09 PDT
(those failure are mostly because the chrome side has to land first)
Comment 23 Build Bot 2013-03-21 15:07:43 PDT
Comment on attachment 194332 [details]
Patch

Attachment 194332 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17122854

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html
Comment 24 Build Bot 2013-03-21 15:07:49 PDT
Created attachment 194356 [details]
Archive of layout-test-results from webkit-ews-09

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: <class 'webkitpy.common.config.ports.MacWK2Port'>  Platform: Mac OS X 10.8.2
Comment 25 Kinuko Yasuda 2013-03-21 15:09:20 PDT
Please make sure add new tests to the skip list in each platform's TestExpectation (I'm sure you're doing so now).

> (those failure are mostly because the chrome side has to land first)

I don't think we support shared worker (and Quota) in test_shell/DumpRenderTree, so I think you need to skip them even for chromium at least for now.
Comment 26 Alec Flett 2013-03-21 15:10:51 PDT
Created attachment 194357 [details]
Patch
Comment 27 Alec Flett 2013-03-21 15:16:28 PDT
abarth@ - r?

The webcore side of things now shows up in bug 112972
Comment 28 Adam Barth 2013-03-21 16:01:54 PDT
Comment on attachment 194357 [details]
Patch

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

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:58
> +static const char queryUsageAndQuotaMode[] = "queryUsageAndQuotaMode";

You should just use the string literal on line 68.  There's no advantage to having a separate declarations.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:65
> +    WorkerScriptController* controller = WorkerScriptController::controllerForContext();
> +    WorkerContext* workerContext = controller->workerContext();
> +    WebCore::WorkerThread* workerThread = workerContext->thread();
> +    WorkerRunLoop& runLoop = workerThread->runLoop();

I would combine all of this into one line.  We shouldn't need to name all these temporaries.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:69
> +    mode.append(String::number(runLoop.createUniqueId()));

You should use operator+ rather than calling append() on String.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:72
> +    bridge->postQueryUsageAndQuotaToMainThread(commonClient, storageType, mode);

Is the bridge just dropped on the floor here?
Comment 29 Adam Barth 2013-03-21 16:02:16 PDT
> abarth@ - r?

I don't think I'm the best reviewer for this patch.
Comment 30 Adam Barth 2013-03-21 16:02:44 PDT
Comment on attachment 194357 [details]
Patch

API change LGTM
Comment 31 Kinuko Yasuda 2013-03-21 16:21:31 PDT
Comment on attachment 194357 [details]
Patch

The code looks good (in a sense that it looks mostly same to the existing working code).
Can you please update the FIXME comment so that it becomes clear there're possible cleanups we can do later?

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

>> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:72
>> +    bridge->postQueryUsageAndQuotaToMainThread(commonClient, storageType, mode);
> 
> Is the bridge just dropped on the floor here?

It's reference is held in a task that is posted to the main thread in the method.

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:154
> +

nit: extra empty space

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks

Can we update this FIXME to:

// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:89
> +

nit: extra empty lines
Comment 32 Kinuko Yasuda 2013-03-21 16:28:50 PDT
(In reply to comment #29)
> > abarth@ - r?
> 
> I don't think I'm the best reviewer for this patch.

(I pinged levin@ to review this. -- thanks Dave!)
Comment 33 David Levin 2013-03-21 17:44:14 PDT
Comment on attachment 194357 [details]
Patch

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

>> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
>> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks
> 
> Can we update this FIXME to:
> 
> // FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

+1

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:50
> +    // Callbacks are self-destructed and we always return leaked pointer here.

It might be better to explain that the class is destructed in the callbacks "did*. See the other comment below.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:63
> +        m_bridge->didQueryStorageUsageAndQuotaOnMainThread(usageInBytes, quotaInBytes, m_mode);

delete this;

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:68
> +        m_bridge->didFailOnMainThread(error, m_mode);

delete this;
Comment 34 David Levin 2013-03-21 17:45:09 PDT
Please address all the feedback given so far and then I think this will be ready for r+
Comment 35 Alec Flett 2013-03-21 18:34:01 PDT
Created attachment 194414 [details]
Patch

All review comments addressed... one more time, levin@ - r?
Comment 36 David Levin 2013-03-21 19:51:28 PDT
Comment on attachment 194414 [details]
Patch

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

Where are the tests?

Plus the other comment.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:91
> +// that it only gets deleted on the worker context thread which is verified by ~Observer.

Kinuko's previous comment about this:
"Can we update this FIXME to:

// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker."
Comment 37 Kinuko Yasuda 2013-03-21 19:55:44 PDT
(In reply to comment #36)
> (From update of attachment 194414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194414&action=review
> 
> Where are the tests?

We have tests in WebCore-side patch in bug 112972

(Alec, it'd be nicer to note that the test is coming in the other patch in ChangeLog)
Comment 38 David Levin 2013-03-21 21:12:32 PDT
Comment on attachment 194414 [details]
Patch

ok. Please fix the comment per Kinuko's suggestion before submitting.
Comment 39 Alec Flett 2013-03-22 10:38:54 PDT
Created attachment 194589 [details]
Patch for landing
Comment 40 Alec Flett 2013-03-22 10:40:06 PDT
(Sorry there were two FIXME's - so now I've replaced them both with the new comment)
Comment 41 WebKit Review Bot 2013-03-22 11:29:27 PDT
Comment on attachment 194589 [details]
Patch for landing

Clearing flags on attachment: 194589

Committed r146633: <http://trac.webkit.org/changeset/146633>
Comment 42 WebKit Review Bot 2013-03-22 11:29:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 43 Michael Nordman 2013-03-22 12:00:06 PDT
Comment on attachment 194589 [details]
Patch for landing

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

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

Would it be good to handle this more like we handle IDB api calls and do the thread hopping (back and forth) on the other side of the WebKit API? The amount of and complexity of this stuff in webkit/webcore feels really disproportionate to the task.
Comment 44 Kinuko Yasuda 2013-03-22 12:11:18 PDT
Comment on attachment 194589 [details]
Patch for landing

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

>> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
>> +// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.
> 
> Would it be good to handle this more like we handle IDB api calls and do the thread hopping (back and forth) on the other side of the WebKit API? The amount of and complexity of this stuff in webkit/webcore feels really disproportionate to the task.

I'm thinking about moving this to Platform and just send an IPC from there using thread-safe-sender with thread-safe callback map on the other side.
And (as I keep saying in this issue) I agree that this change is way too complex and should be gone asap.

For follow-up cleanup I filed a bug (on crbug for now)-- http://crbug.com/223203
Comment 45 Michael Nordman 2013-03-22 12:16:56 PDT
Comment on attachment 194589 [details]
Patch for landing

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:76
> +    virtual void queryUsageAndQuota(WebStorageQuotaType, WebStorageQuotaCallbacks*)

Yup, define this method to be called on the worker thread directly and to have the callback invoked directly on the worker thread in response.
Comment 46 Alec Flett 2013-03-22 12:53:46 PDT
and to follow up with michaeln@ - I asked dgrogan "how does IDB do this because I see none of this glue in any of our code" and he pointed out the change on the chrome side in the IndexedDBMessageFilter or something like that.