[Webkit-unassigned] [Bug 45808] Add Worker support for FileSystem API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45808





--- Comment #34 from Kinuko Yasuda <kinuko at chromium.org>  2010-09-22 16:14:45 PST ---
Thanks, updated.

(In reply to comment #31)
> (From update of attachment 68338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68338&action=review
> > WebCore/fileapi/LocalFileSystem.h:74
> >      String m_basePath;
> 
> What about making a (private) nested class that has this string as a private member?  Then access would be forced to got through its accessor.
> Something like this:
> class SystemBasePath { ...

Sounds a good idea.  Fixed.

> > WebCore/workers/WorkerContext.cpp:116
> > +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> > +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd; ++iter)
> > +        (*iter)->stopObserving();
> Just call notifyObserversOfStop.

Fixed.

> > WebCore/workers/WorkerContext.cpp:385
> > +    if (m_context) {
> Consider "fail fast" here.

Fixed.

> > WebCore/workers/WorkerContext.cpp:386
> > +        ASSERT(m_context->isContextThread());
> 
> Why doesn't this do
>   m_context->unregisterObserver(this);
> ?

Fixed.  (I wondered the same thing when I was self-reviewing... :))

> > WebCore/workers/WorkerContext.cpp:397
> > +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> > +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd;
>  ...
 > I would make this robust to unregisterObserver being called during notifyStop. For example, an object may delete itself :)

Right.  Fixed.

> > WebCore/workers/WorkerContext.h:59
> > +    class WorkerWeakReference;
> This doesn't appear to be used (or defined).

Removed.

> > WebCore/workers/WorkerContext.h:130
> > +        // They are placed here and in all capital letters to enforce compile-time enum checking.
> Is this comment useful? (If anyone tries to change this, they'll get a compile type error.)

Removed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:120
> > +    if (m_callbacksOnWorkerThread) {
> How do you know that m_callbacksOnWorkerThread is still valid at this point?

'Not owned' comment was wrong... it is self-destructed too and the bridge has the responsibility to call any of the callbacks (and the pointer itself should be valid until the bridge fires it).

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:126
> > +void WorkerFileSystemCallbacksBridge::postOpenFileSystemOnMainThread(WebCommonWorkerClient* commonClient, WebFileSystem::Type type, long long size, const String& mode)
> Consider making the name postOpenFileSystemToMainThread.
> OnXThread is being used for methods that run on Thread X but this method is to push something to the main thread.

Fixed.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:134
> > +{
> Please consider adding asserts to verify that things are called on the correct thread:
>  ASSERT(isMainThread()); and ASSERT(m_worker->isContextThread());

Added.

> > WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:145
> > +    mayPostTaskToWorker(createCallbackTask(&didFailOnWorkerThread, this, error), mode);
> 
> I believe you can change s/this/this->m_selfRef.releaseRef()/ and the remove all of the clear calls.
> or you may only be able to do s/this/this->m_selfRef/ (and just have one clear call in mayPostTaskToWorker).  (I should make the former possible in the future if it isn't currently.)

Ah that's sweet.
Seems like the former doesn't work for now, so I changed the code to pass this->m_selfRef and left only one m_selfRef.clear in mayPostTaskToWorker.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.