| Summary: | FileSystem mods: Changes to snapshot file creation to reduce dependencies on blobs. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Nordman <michaeln> | ||||||||||||||
| Component: | WebCore Misc. | Assignee: | Michael Nordman <michaeln> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, dglazkov, fishd, kinuko, levin+threading, webkit.review.bot | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 108736 | ||||||||||||||||
| Bug Blocks: | 108733 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 186463 [details] impl of new api will have to wait for https://codereview.chromium.org/12084077/ to land in chromium svn 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.
Created attachment 186466 [details]
impl of new api
fix some style lint things
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.
> 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 :)
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 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 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 fyi, the chromium test failures are expected until https://codereview.chromium.org/12084077/ lands 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. Created attachment 189344 [details]
patch
same patch, reupped to run ews bots again
(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. 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.
ap, not uploaded yet but here are the general blob changes i'm working up to. https://codereview.chromium.org/11192017/ now that https://codereview.chromium.org/12084077/ is committed, tests are passing... so this is ready for a review adam or darin, maybe one of you can take a look? 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? > 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.
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. > 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.
Created attachment 192600 [details]
impl of new api
patch for landing with a new change log comment about no tests
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 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. 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 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 Created attachment 193004 [details]
impl of new api
merged and modified per kinuko's comments
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 > /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.
Created attachment 193016 [details]
impl of new api
changelog massaging
Comment on attachment 193016 [details] impl of new api Clearing flags on attachment: 193016 Committed r145771: <http://trac.webkit.org/changeset/145771> All reviewed patches have been landed. Closing bug. |
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.