Can I add LocalFileSystem::deleteFileSystem like following patch? I'm implementing FileSystem support of Inspector, and I'd like to allow web developers to delete FileSystem for debugging their applications.
Created attachment 153503 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 153503 [details] Patch WebKit API change LGTM
Comment on attachment 153503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153503&action=review > Source/WebCore/ChangeLog:7 > + We need a comment about how we are testing it. (E.g. "We will add a test when we wire-up the feature to the inspector" etc) > Source/WebCore/platform/AsyncFileSystem.h:70 > + static void deleteFileSystem(const String& basePath, const String& storageIdentifier, PassOwnPtr<AsyncFileSystemCallbacks>); This likely needs FileSystemType parameter? Also while you're there can you add FIXME comment at openFileSystem for adding type parameter? (Looks like we should have it there too) > Source/WebKit/chromium/public/WebFrameClient.h:353 > + // All on-flight operation and following operation may fail after the nit: on-flight -> in-flight, operation -> operations ? > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237 > + if (!context->isDocument()) { Should we rather ASSERT(context->isDocument()) ?
Created attachment 153726 [details] Patch
Created attachment 153727 [details] Patch
Comment on attachment 153503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153503&action=review >> Source/WebCore/ChangeLog:7 >> + > > We need a comment about how we are testing it. (E.g. "We will add a test when we wire-up the feature to the inspector" etc) Done >> Source/WebCore/platform/AsyncFileSystem.h:70 >> + static void deleteFileSystem(const String& basePath, const String& storageIdentifier, PassOwnPtr<AsyncFileSystemCallbacks>); > > This likely needs FileSystemType parameter? > > Also while you're there can you add FIXME comment at openFileSystem for adding type parameter? (Looks like we should have it there too) Done >> Source/WebKit/chromium/public/WebFrameClient.h:353 >> + // All on-flight operation and following operation may fail after the > > nit: on-flight -> in-flight, operation -> operations ? Done >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237 >> + if (!context->isDocument()) { > > Should we rather ASSERT(context->isDocument()) ? Done
Comment on attachment 153727 [details] Patch Thanks, FileSystem related change look good to me.
Comment on attachment 153727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153727&action=review > Source/WebKit/chromium/public/WebFrameClient.h:350 > + // WebFileSystemCallbacks::didSuccess() must be called when the operation s/didSuccess/didSucceed
Created attachment 154143 [details] Patch
Comment on attachment 153727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153727&action=review >> Source/WebKit/chromium/public/WebFrameClient.h:350 >> + // WebFileSystemCallbacks::didSuccess() must be called when the operation > > s/didSuccess/didSucceed Done
Comment on attachment 154143 [details] Patch Attachment 154143 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13330527 New failing tests: fast/loader/loadInProgress.html fast/inline/positionedLifetime.html fast/loader/unload-form-post-about-blank.html
Created attachment 154151 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 154143 [details] Patch Changing commit-queu flag to "?" from "-". I believe this patch doesn't make any functional change until wired-up.
Adam, could you take a look for it? I believe this patch is ready.
Comment on attachment 154143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154143&action=review > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237 > + ASSERT(context->isDocument()); This doesn't exist in workers? > Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:240 > + WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame()); What if document->frame is null?
Comment on attachment 154143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154143&action=review >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:237 >> + ASSERT(context->isDocument()); > > This doesn't exist in workers? For expected usage from Inspector, we don't need to call this from worker. >> Source/WebKit/chromium/src/LocalFileSystemChromium.cpp:240 >> + WebFrameImpl* webFrame = WebFrameImpl::fromFrame(document->frame()); > > What if document->frame is null? document->frame is always non-null for our usage in Inspector.
Comment on attachment 154143 [details] Patch Ok.
Comment on attachment 154143 [details] Patch Clearing flags on attachment: 154143 Committed r123653: <http://trac.webkit.org/changeset/123653>
All reviewed patches have been landed. Closing bug.