Bug 49940 - [Chromium] Implement FileWriterSync
Summary: [Chromium] Implement FileWriterSync
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric U.
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-22 14:28 PST by Eric U.
Modified: 2010-11-24 17:55 PST (History)
2 users (show)

See Also:


Attachments
Chromium bits to implement AsyncFileWriter support for synchronous operations (6.64 KB, patch)
2010-11-22 17:26 PST, Eric U.
levin: review-
Details | Formatted Diff | Diff
Added a mode for the runloop. (7.38 KB, patch)
2010-11-24 11:40 PST, Eric U.
levin: review-
Details | Formatted Diff | Diff
Fixed runloop to have a unique ID. (7.57 KB, patch)
2010-11-24 14:41 PST, Eric U.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric U. 2010-11-22 14:28:22 PST
Add the Chromium-specific implementation for FileWriterSync.
Comment 1 Eric U. 2010-11-22 17:26:48 PST
Created attachment 74620 [details]
Chromium bits to implement AsyncFileWriter support for synchronous operations

See https://bugs.webkit.org/show_bug.cgi?id=49939 for context, but the patches can go in in whichever order.
Comment 2 David Levin 2010-11-23 20:44:49 PST
Comment on attachment 74620 [details]
Chromium bits to implement AsyncFileWriter support for synchronous operations

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

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:208
> +        if (context->thread()->runLoop().runInMode(context, WorkerRunLoop::defaultMode()) == MessageQueueTerminated)

Don't run a nested loop in defaultMode. It should have a name (and ideally have a number appended).

See other places where this is done.

Other than that this change looks great!
Comment 3 Eric U. 2010-11-24 11:40:53 PST
Created attachment 74784 [details]
Added a mode for the runloop.

I decided not to make a unique per-invocation [numbered] mode string.  It's extra string manipulation on every call for a feature that may never be needed, and it'll be easy to add later.  I did put in a warning comment about it, though.
Comment 4 David Levin 2010-11-24 13:59:44 PST
Comment on attachment 74784 [details]
Added a mode for the runloop.

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

> WebKit/chromium/src/WorkerFileWriterCallbacksBridge.cpp:205
> +    m_proxy->postTaskForModeToWorkerContext(createCallbackTask(&runTaskOnWorkerThread, this, task), fileWriterOperationsMode);

Ok so this posts to a mode for the sync calls and the async calls. In other words, it is using a mode for the async calls when it really isn't trying to post to a nested message loop.

That will work and results in less code...  (it feels mildly odd for some reason but) ok.
Comment 5 David Levin 2010-11-24 14:03:28 PST
Comment on attachment 74784 [details]
Added a mode for the runloop.

Actually I think the single mode is broken.
Comment 6 David Levin 2010-11-24 14:04:14 PST
(In reply to comment #5)
> (From update of attachment 74784 [details])
> Actually I think the single mode is broken.

If this function is used by async functions for callbacks, then those callbacks will happen during this sync call and that would be bad.
Comment 7 Eric U. 2010-11-24 14:41:49 PST
Created attachment 74796 [details]
Fixed runloop to have a unique ID.
Comment 8 David Levin 2010-11-24 15:18:54 PST
Comment on attachment 74796 [details]
Fixed runloop to have a unique ID.

Thanks!
Comment 9 WebKit Commit Bot 2010-11-24 17:25:09 PST
The commit-queue encountered the following flaky tests while processing attachment 74796 [details]:

fast/css/font-face-download-error.html
animations/suspend-resume-animation-events.html

Please file bugs against the tests.  These tests were authored by cmarrin@apple.com and yuzo@google.com.  The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2010-11-24 17:55:36 PST
Comment on attachment 74796 [details]
Fixed runloop to have a unique ID.

Clearing flags on attachment: 74796

Committed r72711: <http://trac.webkit.org/changeset/72711>
Comment 11 WebKit Commit Bot 2010-11-24 17:55:42 PST
All reviewed patches have been landed.  Closing bug.