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

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #68338|review?                     |review-
               Flag|                            |




--- Comment #31 from David Levin <levin at chromium.org>  2010-09-21 22:36:06 PST ---
(From update of attachment 68338)
View in context: https://bugs.webkit.org/attachment.cgi?id=68338&action=review

I almost r+'ed but it feels like there are enough little things to address that it would be good to check it over one last time after the adjustment.

Thanks for your patience. It is almost all done!

> 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 {
public:
    explicit SystemBasePath(string path) : m_value(path) { }

    string value() const
    {
        return m_value.threadsafeCopy();
    }  

private:
    String m_value;
};
SystemBasePath m_basePath;

Then no need for a comment that people probably won't see when adding code or doing reviews :)

> 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.

> WebCore/workers/WorkerContext.cpp:385
> +    if (m_context) {

Consider "fail fast" here.

if (!m_context)
    return;

> WebCore/workers/WorkerContext.cpp:386
> +        ASSERT(m_context->isContextThread());

Why doesn't this do
  m_context->unregisterObserver(this);
?

> WebCore/workers/WorkerContext.cpp:397
> +    HashSet<Observer*>::iterator observersEnd = m_workerObservers.end();
> +    for (HashSet<Observer*>::iterator iter = m_workerObservers.begin(); iter != observersEnd; ++iter) {
> +        (*iter)->notifyStop();
> +        (*iter)->stopObserving();
> +    }

I would make this robust to unregisterObserver being called during notifyStop. For example, an object may delete itself :)

Here's a simple change:
HashSet<Observer*>::iterator iter = m_workerObservers.begin();
while (iter != m_workerObservers.end()) {
    (*iter)->notifyStop();
    (*iter)->stopObserving();
    iter = m_workerObservers.begin();
}

Of course, there could be a pathological case in which an object adds something else when it gets the notifyStop but that seems highly unlikely.

> WebCore/workers/WorkerContext.cpp:398
> +    m_workerObservers.clear();

If the above is done, this should be already done.

> WebCore/workers/WorkerContext.h:59
> +    class WorkerWeakReference;

This doesn't appear to be used (or defined).

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

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:120
> +    if (m_callbacksOnWorkerThread) {

How do you know that m_callbacksOnWorkerThread is still valid at this point?

> 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.

> 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());

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

(here and in didOpenFileSystemOnMainThread)

> WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:153
> +void WorkerFileSystemCallbacksBridge::didFailOnWorkerThread(ScriptExecutionContext*, WorkerFileSystemCallbacksBridge* bridge, WebFileError error)

s/WorkerFileSystemCallbacksBridge*/PassRefPtr<WorkerFileSystemCallbacksBridge>/
ditto for didOpenFileSystemOnWorkerThread

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