Bug 74666 - [chromium] FileSystem bridge needs threadsafe fixes and other clean-up.
Summary: [chromium] FileSystem bridge needs threadsafe fixes and other clean-up.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 74969 74975 75494 75573 75579 75614
Blocks: 67942 74746
  Show dependency treegraph
 
Reported: 2011-12-15 16:45 PST by David Levin
Modified: 2012-01-05 04:04 PST (History)
5 users (show)

See Also:


Attachments
WIP (42.06 KB, patch)
2011-12-16 00:32 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (48.52 KB, patch)
2012-01-03 17:42 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (8.88 KB, text/plain)
2012-01-04 14:21 PST, David Levin
no flags Details
Patch (14.84 KB, patch)
2012-01-04 15:27 PST, David Levin
dimich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-12-15 16:45:26 PST
See summary.
Comment 1 David Levin 2011-12-16 00:32:56 PST
Created attachment 119580 [details]
WIP
Comment 2 David Levin 2011-12-16 00:35:14 PST
Comment on attachment 119580 [details]
WIP

Not completely ready for review but almost....
Comment 3 Dmitry Lomov 2011-12-16 16:22:05 PST
Comment on attachment 119580 [details]
WIP

The change looks good so far, the only issue I have is that it is hard to understand :) Maybe you could put more comments in the change log, regarding:
1) why m_mode fields are removed
2) the lifetime of WorkerFileSystemObserver and why the bridge is no longer an Observer
3) Some sort of "proof" that m_workerContextObserver only gets deleted on worker thread (ASSERTs, even?)
Comment 4 Kinuko Yasuda 2011-12-19 21:42:55 PST
Comment on attachment 119580 [details]
WIP

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

Awesome, it's really great to see the tests for this complex worker termination sequence.  The code looks good to me (as far as I understand).

> LayoutTests/http/tests/workers/terminate-during-sync-operation.html:39
> +function waitForAllWorkersToStart()

This name may sound a bit confusing as its main purpose is to start sync ops after making sure everyone is started?
How about startAfterAllWorkersStarted or something?  (I'm not good at naming though)
Comment 5 David Levin 2012-01-03 17:42:52 PST
Created attachment 121027 [details]
Patch
Comment 6 David Levin 2012-01-03 17:53:39 PST
Comment on attachment 121027 [details]
Patch

I need to address the feedback already given.
Comment 7 David Levin 2012-01-04 14:21:27 PST
Created attachment 121153 [details]
Patch
Comment 8 David Levin 2012-01-04 14:22:18 PST
Comment on attachment 121153 [details]
Patch

Wrong bug.
Comment 9 David Levin 2012-01-04 15:27:43 PST
Created attachment 121174 [details]
Patch
Comment 10 Dmitry Titov 2012-01-04 16:33:29 PST
Comment on attachment 121174 [details]
Patch

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

r+ with couple nits.

> LayoutTests/http/tests/workers/resources/sync-operations.js:22
> +	for (var i = 0; i < 10; ++i) {

Indenting.

> LayoutTests/http/tests/workers/resources/sync-operations.js:69
> +	return;

This needs comment on why it is needed and why it is ok to just return.

> LayoutTests/http/tests/workers/terminate-during-sync-operation.html:78
> +<div id='result'>

It'd be nice to add a sentence here describing what this test is testing and what is expected PASS or FAIL results. So the people can figure the purpose of the test w/o looking at the several related bugs.
Comment 11 David Levin 2012-01-04 19:10:28 PST
Committed as http://trac.webkit.org/changeset/104113