[V8] Merge getCachedWrapper(Node*) into DOMDataStore
Created attachment 174035 [details] Patch
Comment on attachment 174035 [details] Patch Seems reasonable. Normal caveat applies (talk to harken if you feel you need).
I mean haraken. :)
Comment on attachment 174035 [details] Patch LGTM too
Created attachment 175085 [details] Patch for landing
Created attachment 175086 [details] Patch for landing
Created attachment 175087 [details] Patch for landing
Comment on attachment 175087 [details] Patch for landing Rejecting attachment 175087 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: kendImpl.h:32, from Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:27: Source/WebCore/Modules/indexeddb/IDBBackingStore.h:76: error: ISO C++ forbids declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type CXX(target) out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBCursorWithValue.o make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/14894386
I don't understand how this compiles without my patch... DISALLOW_COPY_AND_ASSIGN isn't a WebKit macro...
Comment on attachment 175087 [details] Patch for landing Clearing flags on attachment: 175087 Committed r135230: <http://trac.webkit.org/changeset/135230>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > (From update of attachment 175087 [details]) > Clearing flags on attachment: 175087 > > Committed r135230: <http://trac.webkit.org/changeset/135230> This commit caused compilation failure on Chromium Debug builders: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/1943/steps/compile/logs/stdio I removed the ASSERT for now in http://trac.webkit.org/changeset/135244.
Reopening to attach new patch.
Created attachment 175140 [details] Patch
Comment on attachment 175140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175140&action=review > Source/WebCore/bindings/v8/DOMDataStore.h:66 > + if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) { Just a confirmation: !v8::Context::InContext() is not needed here, right?
Comment on attachment 175140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175140&action=review >> Source/WebCore/bindings/v8/DOMDataStore.h:66 >> + if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) { > > Just a confirmation: !v8::Context::InContext() is not needed here, right? Correct.
> This commit caused compilation failure on Chromium Debug builders: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/1943/steps/compile/logs/stdio > > I removed the ASSERT for now in http://trac.webkit.org/changeset/135244. Thanks. Yes, now that this function is static, we can't assert something about the object's member variables.
Comment on attachment 175140 [details] Patch Clearing flags on attachment: 175140 Committed r135257: <http://trac.webkit.org/changeset/135257>
This (In reply to comment #18) > (From update of attachment 175140 [details]) > Clearing flags on attachment: 175140 > > Committed r135257: <http://trac.webkit.org/changeset/135257> This change broke Debug compilation for the same reason as the original patch: In file included from ../../WebCore/bindings/v8/V8DOMWrapper.h:34: ../../WebCore/bindings/v8/DOMDataStore.h:67:20: error: call to non-static member function without an object argument ASSERT(wrapperIsStoredInObject(object)); ^~~~~~~~~~~~~~~~~~~~~~~ ../../WTF/wtf/Assertions.h:247:8: note: expanded from macro 'ASSERT' (!(assertion) ? \ ^ http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac%20Builder%20%28dbg%29/builds/34123/steps/compile/logs/stdio Please make sure your changes at least compile before committing them.
(In reply to comment #20) > This change broke Debug compilation for the same reason as the original patch: I'm sorry, I'll fix asap after confirming the debug build locally.
Created attachment 175168 [details] Patch
Comment on attachment 175168 [details] Patch I confirmed it works in my local Chromiu/Debug build.
Comment on attachment 175168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175168&action=review > Source/WebCore/bindings/v8/DOMDataStore.h:67 > - ASSERT(wrapperIsStoredInObject(object)); > + ASSERT(current(isolate)->wrapperIsStoredInObject(object)); This isn't right. Let's just remove the assertion.
(In reply to comment #24) > > Source/WebCore/bindings/v8/DOMDataStore.h:67 > > - ASSERT(wrapperIsStoredInObject(object)); > > + ASSERT(current(isolate)->wrapperIsStoredInObject(object)); > > This isn't right. Let's just remove the assertion. It's OK to remove the assertion, but why isn't it right?
(In reply to comment #25) > (In reply to comment #24) > > > Source/WebCore/bindings/v8/DOMDataStore.h:67 > > > - ASSERT(wrapperIsStoredInObject(object)); > > > + ASSERT(current(isolate)->wrapperIsStoredInObject(object)); > > > > This isn't right. Let's just remove the assertion. > > It's OK to remove the assertion, but why isn't it right? You're right, this assertion is correct. Node implies that we must be on the main thread, and the non-existence of isolated worlds implies that this must be the main world.
Comment on attachment 175168 [details] Patch I need to rebase with ToT.
Created attachment 175303 [details] patch for landing
Comment on attachment 175303 [details] patch for landing One moment. This patch is based on something we might need to roll out because of bug 102852
ok, feel free to insert this assertion when you reland your patch.
Comment on attachment 175303 [details] patch for landing One moment.
If you don't mind, we can skip this patch because I'm changing how this stuff works a bit in bug 102854.