Bug 74666 – [chromium] FileSystem bridge needs threadsafe fixes and other clean-up.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Levin]

See summary.

[David Levin]

Created attachment 119580 [details]
WIP

[David Levin]

Comment on attachment 119580 [details]
WIP

Not completely ready for review but almost....

[Dmitry Lomov]

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?)

[Kinuko Yasuda]

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)

[David Levin]

Created attachment 121027 [details]
Patch

[David Levin]

Comment on attachment 121027 [details]
Patch

I need to address the feedback already given.

[David Levin]

Created attachment 121153 [details]
Patch

[David Levin]

Comment on attachment 121153 [details]
Patch

Wrong bug.

[David Levin]

Created attachment 121174 [details]
Patch

[Dmitry Titov]

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.

[David Levin]

Committed as http://trac.webkit.org/changeset/104113