Bug 108851 – FileSystem mods: Changes to snapshot file creation to reduce dependencies on blobs.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Nordman]

New methods have been declared in the chromium WebKit API for snapfile file creation. Need to patch webcore/webkit to use those new methods.

This patch is the 3rd in a series of changes that straddle webkit vs chromium repositories.

1) WK: Declare new virtual createSnapshotFile/didCreateSnapshotFile public api methods.
    done in: https://bugs.webkit.org/show_bug.cgi?id=108736
2) CR: Implement the new create method such that the didCreate method is invoked in response.
    done in: https://codereview.chromium.org/12084077/
3) WK: Use the new create method and implement the new didCreate method.
3) CR: Cleanup the obsolete/deprecated blocks of code.

[Michael Nordman]

Created attachment 186463 [details]
impl of new api

will have to wait for https://codereview.chromium.org/12084077/ to land in chromium svn

[WebKit Review Bot]

Attachment 186463 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/filesystem/DOMFileSystem.cpp', u'Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp', u'Source/WebCore/platform/AsyncFileSystemCallbacks.h', u'Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp', u'Source/WebCore/platform/network/BlobData.cpp', u'Source/WebCore/platform/network/BlobData.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.h', u'Source/WebKit/chromium/src/LocalFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h', u'Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h']" exit_code: 1
Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h:61:  The parameter name "info" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h:69:  The parameter name "info" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/network/BlobData.h:217:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:310:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 186466 [details]
impl of new api

fix some style lint things

[WebKit Review Bot]

Attachment 186466 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/filesystem/DOMFileSystem.cpp', u'Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp', u'Source/WebCore/platform/AsyncFileSystemCallbacks.h', u'Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp', u'Source/WebCore/platform/network/BlobData.cpp', u'Source/WebCore/platform/network/BlobData.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.h', u'Source/WebKit/chromium/src/LocalFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h', u'Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h']" exit_code: 1
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:310:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

> Are you using a 4-space indent?  [whitespace/indent] [3]

If you were a person, instead of a mindless bot, you'd see that I'm making a slight modification to an existing line of code without altering its indentation :)

[Build Bot]

Comment on attachment 186466 [details]
impl of new api

Attachment 186466 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375463

[WebKit Review Bot]

Comment on attachment 186466 [details]
impl of new api

Attachment 186466 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16365612

New failing tests:
fast/filesystem/file-writer-abort-depth.html
fast/filesystem/file-writer-empty-blob.html
fast/filesystem/file-from-file-entry.html
fast/filesystem/workers/file-writer-gc-blob.html
fast/filesystem/workers/file-from-file-entry-sync.html
fast/filesystem/file-writer-abort-continue.html
fast/filesystem/file-writer-abort.html
fast/filesystem/workers/file-writer-sync-write-overlapped.html
fast/filesystem/file-writer-truncate-extend.html
http/tests/websocket/tests/hybi/send-file-blob-fail.html
http/tests/inspector/filesystem/request-file-content.html
fast/filesystem/file-writer-events.html
fast/filesystem/file-writer-write-overlapped.html
fast/filesystem/workers/file-writer-events.html
fast/filesystem/file-metadata-after-write.html
fast/filesystem/workers/file-writer-empty-blob.html
fast/filesystem/simple-readonly-file-object.html
fast/filesystem/workers/file-writer-truncate-extend.html
fast/filesystem/workers/file-writer-write-overlapped.html
fast/filesystem/workers/file-writer-sync-truncate-extend.html
fast/filesystem/workers/file-from-file-entry.html
http/tests/websocket/tests/hybi/send-file-blob.html
fast/filesystem/file-writer-gc-blob.html

[WebKit Review Bot]

Comment on attachment 186466 [details]
impl of new api

Attachment 186466 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16368502

New failing tests:
fast/filesystem/file-writer-abort-depth.html
fast/filesystem/file-writer-empty-blob.html
fast/filesystem/file-from-file-entry.html
fast/filesystem/workers/file-writer-gc-blob.html
fast/filesystem/workers/file-from-file-entry-sync.html
fast/filesystem/file-writer-abort-continue.html
fast/filesystem/file-writer-abort.html
fast/filesystem/workers/file-writer-sync-write-overlapped.html
fast/filesystem/file-writer-truncate-extend.html
http/tests/websocket/tests/hybi/send-file-blob-fail.html
http/tests/inspector/filesystem/request-file-content.html
fast/filesystem/file-writer-events.html
fast/filesystem/file-writer-write-overlapped.html
fast/filesystem/workers/file-writer-events.html
fast/filesystem/file-metadata-after-write.html
fast/filesystem/workers/file-writer-empty-blob.html
fast/filesystem/simple-readonly-file-object.html
fast/filesystem/workers/file-writer-truncate-extend.html
fast/filesystem/workers/file-writer-write-overlapped.html
fast/filesystem/workers/file-writer-sync-truncate-extend.html
fast/filesystem/workers/file-from-file-entry.html
http/tests/websocket/tests/hybi/send-file-blob.html
fast/filesystem/file-writer-gc-blob.html

[Michael Nordman]

fyi, the chromium test failures are expected until https://codereview.chromium.org/12084077/ lands

[Alexey Proskuryakov]

What is the status of this patch, does it need a reviewer? I'm willing to look at general blob changes, but not at FileSystem ones.

[Michael Nordman]

Created attachment 189344 [details]
patch

same patch, reupped to run ews bots again

[Michael Nordman]

(In reply to comment #10)
> What is the status of this patch, does it need a reviewer? I'm willing to look at general blob changes, but not at FileSystem ones.

It does need a reviewer, but it is not a general blob change, this is very specific to the FileSystem.

[WebKit Review Bot]

Attachment 189344 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/filesystem/DOMFileSystem.cpp', u'Source/WebCore/Modules/filesystem/DOMFileSystemSync.cpp', u'Source/WebCore/platform/AsyncFileSystemCallbacks.h', u'Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp', u'Source/WebCore/platform/network/BlobData.cpp', u'Source/WebCore/platform/network/BlobData.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/AsyncFileSystemChromium.h', u'Source/WebKit/chromium/src/LocalFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.cpp', u'Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h', u'Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp', u'Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h']" exit_code: 1
Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp:310:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

ap, not uploaded yet but here are the general blob changes i'm working up to.

https://codereview.chromium.org/11192017/

[Michael Nordman]

now that https://codereview.chromium.org/12084077/ is committed, tests are passing... so this is ready for a review

[Michael Nordman]

adam or darin, maybe one of you can take a look?

[Adam Barth]

Comment on attachment 189344 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189344&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

This line will stop your patch from being landed.  You should either add a test of an explanation of why this change doesn't need a test.

> Source/WebCore/platform/network/BlobData.cpp:117
> +BlobDataHandle::~BlobDataHandle()
> +{
> +    ThreadableBlobRegistry::unregisterBlobURL(m_internalURL);
> +}

I take is that ThreadableBlobRegistry is safe to call on any thread.

> Source/WebCore/platform/network/BlobData.h:218
> +    KURL m_internalURL;

This object is ThreadSafeRefCounted but presumably KURL itself isn't actually thread safe.  What's the thread-safety story here?

[Alexey Proskuryakov]

> I take is that ThreadableBlobRegistry is safe to call on any thread.

Not quite. Registering and unregistering should be done on the same thread, because originMap() is ThreadSpecific.

[Michael Nordman]

Comment on attachment 189344 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189344&action=review

>> Source/WebCore/ChangeLog:10
>> +        No new tests (OOPS!).
> 
> This line will stop your patch from being landed.  You should either add a test of an explanation of why this change doesn't need a test.

Thnx, i'll explain that there is no change in content observable behavior.

>> Source/WebCore/platform/network/BlobData.cpp:117
>> +}
> 
> I take is that ThreadableBlobRegistry is safe to call on any thread.

Yes

>> Source/WebCore/platform/network/BlobData.h:218
>> +    KURL m_internalURL;
> 
> This object is ThreadSafeRefCounted but presumably KURL itself isn't actually thread safe.  What's the thread-safety story here?

That's right, I could invoke the ThreadableBlobRegistry with an copy, but under the covers ThreadableBlobRegistry/BlobRegistry are taking a copy if needed so it really isn't necessary.

[Michael Nordman]

> Not quite. Registering and unregistering should be done on the same thread, because originMap() is ThreadSpecific.

The originMap only applies to public blob urls with 'null' security contexts, these are internal urls used as identifiers, they're security context is not 'null'. No entries is made in the originMap for them.

[Michael Nordman]

Created attachment 192600 [details]
impl of new api

patch for landing with  a new change log comment about no tests

[WebKit Review Bot]

Comment on attachment 192600 [details]
impl of new api

Rejecting attachment 192600 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 192600, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

patching file Source/WebKit/chromium/src/WorkerAsyncFileSystemChromium.cpp
patching file Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp
Hunk #1 FAILED at 33.
1 out of 7 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.cpp.rej
patching file Source/WebKit/chromium/src/WorkerFileSystemCallbacksBridge.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17169369

[Michael Nordman]

oh well... moving targets...
https://bugs.webkit.org/show_bug.cgi?id=112049
... files shuffling from from this public directory to some other one in preparation of some other moving around in the future. silly me trying to change the code, hopefully i'll be able to land something before it moves again.

[Kinuko Yasuda]

Comment on attachment 192600 [details]
impl of new api

View in context: https://bugs.webkit.org/attachment.cgi?id=192600&action=review

> Source/WebCore/ChangeLog:5
> +        patch, it's just enough to refactor the FileSystem code to not function in terms of blobURLs.

It's nicer to note why this refactoring is good in a simple sentence.

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Nit: It looks like the common ChangeLog format is:

> [Issue subject]
> https://bugs.webkit.org/show_bug.cgi?id=108851
> 
> Reviewed by ...
> 
> [Detail/more info about the change.]
>
> [Test line]

> Source/WebCore/platform/network/BlobData.h:206
> +// https://codereview.chromium.org/11192017/.

Hmm, referring chromium review site from the common code may not make a lot sense. Can we make the 'FIXME' point clearer (like: Do X when Y, X needs to be Y etc) and attach bugs.webkit.org URL instead so that this patch itself becomes slightly more complete?

> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h:67
> +    // created on the main thread to an  AsyncFileSystemCallback on a background worker thread. The other

nit: extra space before AsyncFileSystemCallback

[Michael Nordman]

Comment on attachment 192600 [details]
impl of new api

thnx for looking

View in context: https://bugs.webkit.org/attachment.cgi?id=192600&action=review

>> Source/WebCore/ChangeLog:5
>> +        patch, it's just enough to refactor the FileSystem code to not function in terms of blobURLs.
> 
> It's nicer to note why this refactoring is good in a simple sentence.

i thought i had, 'to reduce dependencies on blob URLs', is there other wording you'd like to see?

>> Source/WebCore/ChangeLog:8
>> +        Reviewed by NOBODY (OOPS!).
> 
> Nit: It looks like the common ChangeLog format is:

i don't understand this comment? webkit-patch land will fill this in automagically

>> Source/WebCore/platform/network/BlobData.h:206
>> +// https://codereview.chromium.org/11192017/.
> 
> Hmm, referring chromium review site from the common code may not make a lot sense. Can we make the 'FIXME' point clearer (like: Do X when Y, X needs to be Y etc) and attach bugs.webkit.org URL instead so that this patch itself becomes slightly more complete?

done. i've put a pointer here to the umbrella bug in webkit, https://bugs.webkit.org/show_bug.cgi?id=108733, but also left the reference to the chromium CL. The chromium CL shows the actual changes to this class in light of the larger refactoring.

>> Source/WebKit/chromium/src/WebFileSystemCallbacksImpl.h:67
>> +    // created on the main thread to an  AsyncFileSystemCallback on a background worker thread. The other
> 
> nit: extra space before AsyncFileSystemCallback

done

[Michael Nordman]

Created attachment 193004 [details]
impl of new api

merged and modified per kinuko's comments

[WebKit Review Bot]

Comment on attachment 193004 [details]
impl of new api

Rejecting attachment 193004 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 193004, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17174204

[Michael Nordman]

> /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Ok, so now i better understand kinuko's comment, apparently the commit bot will not fill this in automagically (seems like it used to?)... prepping another patch.

[Michael Nordman]

Created attachment 193016 [details]
impl of new api

changelog massaging

[WebKit Review Bot]

Comment on attachment 193016 [details]
impl of new api

Clearing flags on attachment: 193016

Committed r145771: <http://trac.webkit.org/changeset/145771>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.