Summary: | Modern IDB: Dump blobs to disk before storing them in an object store | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||
Component: | WebCore Misc. | Assignee: | Brady Eidson <beidson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, ossy | ||||||||
Priority: | P2 | ||||||||||
Version: | Safari 9 | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 143193 | ||||||||||
Attachments: |
|
Description
Brady Eidson
2016-03-31 09:46:27 PDT
This is a bit complicated due to https://bugs.webkit.org/show_bug.cgi?id=129979, but that change didn't really buy us anything so I'm basically going to be undoing it. Created attachment 275585 [details]
Patch v1
This patch dumps Blobs/Files to a temporary file on disk. That's it - nothing more. It's required for the actual IDB work going forward, but is a nice reviewable chunk by itself. Attachment 275585 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:244: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 10 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275585 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=275585&action=review > Source/WebCore/Modules/indexeddb/IDBValue.h:34 > +class IDBValue { > +public: > + IDBValue(); > +}; This is the ideal class. Please make more like this. > Source/WebCore/platform/FileSystem.cpp:137 > + if (!isHandleValid(source)) Probably return false if target is invalid, too. > Source/WebCore/platform/FileSystem.cpp:141 > + static int bufferSize = 1 << 20; > + Vector<char> buffer(bufferSize); 1MB might be a bit big for a buffer. > Source/WebCore/platform/FileSystem.cpp:143 > + ScopeGuard fileCloser([source, buffer]() { No need to capture buffer. > Source/WebCore/platform/FileSystem.cpp:145 > + PlatformFileHandle handle = source; > + closeFile(handle); Is this first line really needed? We can't just close source? > Source/WebCore/platform/network/BlobRegistryImpl.cpp:235 > + static auto& queue = WorkQueue::create("org.webkit.BlobUtility", WorkQueue::Type::Serial, WorkQueue::QOS::Background).leakRef(); NeverDestroyed. > Source/WebCore/platform/network/BlobRegistryImpl.cpp:286 > + callOnMainThread([completionHandler]() { > + Vector<String> filePaths; > + completionHandler(filePaths); > + }); This is redundant. Lots of calling the same thing on main thread. > Source/WebCore/platform/network/BlobRegistryImpl.cpp:290 > + for (auto& part : blob.filePathsOrDataBuffers) { ASSERT(part.first.isNull() || part.second.isEmpty()) (In reply to comment #5) > Comment on attachment 275585 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275585&action=review > > > Source/WebCore/Modules/indexeddb/IDBValue.h:34 > > +class IDBValue { > > +public: > > + IDBValue(); > > +}; > > This is the ideal class. Please make more like this. Will do. > > > Source/WebCore/platform/FileSystem.cpp:137 > > + if (!isHandleValid(source)) > > Probably return false if target is invalid, too. > > > Source/WebCore/platform/FileSystem.cpp:141 > > + static int bufferSize = 1 << 20; > > + Vector<char> buffer(bufferSize); > > 1MB might be a bit big for a buffer. s/1 << 20/1 << 19/ > > Source/WebCore/platform/FileSystem.cpp:145 > > + PlatformFileHandle handle = source; > > + closeFile(handle); > > Is this first line really needed? We can't just close source? Bizarrely yes, because captured arguments lose their const'ness, but closeFile expects an actual const int as its argument. > > Source/WebCore/platform/network/BlobRegistryImpl.cpp:235 > > + static auto& queue = WorkQueue::create("org.webkit.BlobUtility", WorkQueue::Type::Serial, WorkQueue::QOS::Background).leakRef(); > > NeverDestroyed. NeverCaughtByTheCompilerAndLinker > > Source/WebCore/platform/network/BlobRegistryImpl.cpp:286 > > + callOnMainThread([completionHandler]() { > > + Vector<String> filePaths; > > + completionHandler(filePaths); > > + }); > > This is redundant. Lots of calling the same thing on main thread. Will rework it into the scope guard. Created attachment 275590 [details]
Patch for landing
Attachment 275590 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:245: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 10 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 275636 [details]
Patch for landing v2
Patch for landing again... does style bot really prevent commit queue? That's BS, if so.
Going to let style bot complain, *then* mark cq+... just to try it. Attachment 275636 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.cpp:78: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.cpp:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.cpp:245: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:61: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Network/NetworkProcessConnection.h:84: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.h:87: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/FileAPI/BlobRegistryProxy.h:41: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistry.h:67: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/bindings/js/SerializedScriptValue.cpp:2750: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/BlobRegistryImpl.h:70: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 10 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275636 [details] Patch for landing v2 Clearing flags on attachment: 275636 Committed r199043: <http://trac.webkit.org/changeset/199043> (In reply to comment #12) > Comment on attachment 275636 [details] > Patch for landing v2 > > Clearing flags on attachment: 275636 > > Committed r199043: <http://trac.webkit.org/changeset/199043> FYI: It broke the Apple Mac cmake and the WinCairo build. See build.webkit.org for details. and the Apple Windows build too |