[webkit-reviews] review denied: [Bug 45808] Add Worker support for FileSystem API : [Attachment 68338] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 45808: Add Worker support for FileSystem API
https://bugs.webkit.org/show_bug.cgi?id=45808

Attachment 68338: Patch
https://bugs.webkit.org/attachment.cgi?id=68338&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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(WebCommonWorker
Client* 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