Bug 61536

Created attachment 94992 [details] Patch

Comment on attachment 94992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review Great! Couple of comments, lets do another iteration: > Source/WebKit/chromium/src/DatabaseObserver.cpp:71 > + return false; Lets add ASSERT_NOT_REACHED() here, to document that we don't expect any other case but Document in this case. > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:101 > + allowed = false; Same as above, ASSERT_NOT_REACHED() could be useful here. > Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96 > + m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that.

Comment on attachment 94992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review >> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96 >> + m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); > > This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that. Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose.

Comment on attachment 94992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review >>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96 >>> + m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); >> >> This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that. > > Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose. This could do OwnPtr<WebFileSystemCallbacksImpl> callbacks(this); #if if (m_context && m_context->isWorkerContext()) { m_callbacks->didOpenFileSystem(name, WorkerAsyncFileSystemChromium::create(m_context, m_type, path, m_synchronous)); return; } #endif m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); which addresses both issues.

Created attachment 95015 [details] Patch

Comment on attachment 94992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94992&action=review >> Source/WebKit/chromium/src/DatabaseObserver.cpp:71 >> + return false; > > Lets add ASSERT_NOT_REACHED() here, to document that we don't expect any other case but Document in this case. Fixed. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:101 >> + allowed = false; > > Same as above, ASSERT_NOT_REACHED() could be useful here. Fixed. >>>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:96 >>>> + m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); >>> >>> This duplicates a line of code... By placing #if/#endif around if/else, it's possible to avoid that. >> >> Yes, but it may introduce style errors in the bots that don't enable workers. If you think this is ok I'll be happy to fix it like you propose. > > This could do > > OwnPtr<WebFileSystemCallbacksImpl> callbacks(this); > > #if > if (m_context && m_context->isWorkerContext()) { > m_callbacks->didOpenFileSystem(name, WorkerAsyncFileSystemChromium::create(m_context, m_type, path, m_synchronous)); > return; > } > #endif > m_callbacks->didOpenFileSystem(name, AsyncFileSystemChromium::create(m_type, path)); > > which addresses both issues. Thanks for the suggestion! Fixed. However I just realized a compile error here when uploading the patch. Give me a minute and I'll fix it.

Created attachment 95018 [details] Patch Fixing build error caused for not using adoptPtr.

Comment on attachment 95018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95018&action=review One small thing: > Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:99 > delete this; I think this 'delete' is not needed now since there is OwnPtr for 'this'... While at this, lets add a comment to the OwnPtr about this object self-deleting on exit, to make it more explicit.

Created attachment 95020 [details] Patch Performing review fixes.

Comment on attachment 95018 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95018&action=review >> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp:99 >> delete this; > > I think this 'delete' is not needed now since there is OwnPtr for 'this'... While at this, lets add a comment to the OwnPtr about this object self-deleting on exit, to make it more explicit. Done.

Comment on attachment 95020 [details] Patch You could just always do cq? with r? if you intend to use commit queue. Reviewer can flip both at one step if appropriate then.

(In reply to comment #11) > (From update of attachment 95020 [details]) > You could just always do cq? with r? if you intend to use commit queue. Reviewer can flip both at one step if appropriate then. I will, at least once the code seems ready to pass. Thanks for the advice.

The commit-queue encountered the following flaky tests while processing attachment 95020 [details]: inspector/debugger/debugger-scripts.html bug 59921 (authors: pfeldman@chromium.org and podivilov@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 95020 [details] Patch Clearing flags on attachment: 95020 Committed r87463: <http://trac.webkit.org/changeset/87463>

All reviewed patches have been landed. Closing bug.