Bug 156068

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

Fixed in http://trac.webkit.org/changeset/199058 and http://trac.webkit.org/changeset/199059