| Summary: | [chromium] FileSystem bridge needs threadsafe fixes and other clean-up. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Levin <levin> | ||||||||||
| Component: | WebKit Misc. | Assignee: | David Levin <levin> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | dimich, dslomov, kinuko, levin+threading, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Bug Depends on: | 74969, 74975, 75494, 75573, 75579, 75614 | ||||||||||||
| Bug Blocks: | 67942, 74746 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
David Levin
2011-12-15 16:45:26 PST
Created attachment 119580 [details]
WIP
Comment on attachment 119580 [details]
WIP
Not completely ready for review but almost....
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 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) Created attachment 121027 [details]
Patch
Comment on attachment 121027 [details]
Patch
I need to address the feedback already given.
Created attachment 121153 [details]
Patch
Comment on attachment 121153 [details]
Patch
Wrong bug.
Created attachment 121174 [details]
Patch
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. Committed as http://trac.webkit.org/changeset/104113 |