Bug 88052

Created attachment 145229 [details] Patch

Comment on attachment 145229 [details] Patch Attachment 145229 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12869424

Comment on attachment 145229 [details] Patch Attachment 145229 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12872320

Comment on attachment 145229 [details] Patch Attachment 145229 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12866454

Comment on attachment 145229 [details] Patch Attachment 145229 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12870393

Comment on attachment 145229 [details] Patch Attachment 145229 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12871410

Created attachment 145244 [details] Patch

Comment on attachment 145244 [details] Patch From an IDB perspective, these changes look correct but I can't speak to whether they are hitting all the correct cases in JSC. The V8 equivalent of this resides in V8RecursionScope which handles both IDB and mutation observers which need processing when returning from script. That approach would be cleaner than sprinkling multiple IDB-specific ENABLED checks/calls throughout the JSC code. The V8RecursionScope class also handles (1) recursion (duh) which I believe is tested in the storage/indexeddb/transaction-abort-with-js-recursion.html layout test (so may not be necessary with the JSC changes here) and (2) RAII-style calling of the post-script function in the scope destructor, which would simplify the code in evaluateInWorld.

Created attachment 146252 [details] Patch

Comment on attachment 146252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146252&action=review > Source/WebCore/ChangeLog:11 > + No test because JSC binding for IndexedDB not working yet. I've seen a couple of patches for trying to make IndexedDB work in JSC without testing. Actually I do not want to land a lot of patches without testing just because it is not yet enabled. How much work would be needed by the time we can enable IndexedDB in JSC and make it testable? > Source/WebCore/bindings/js/JSMainThreadExecState.cpp:31 > +#include <IDBPendingTransactionMonitor.h> You should use #include "IDBPendingTransactionMonitor.h".

cc-ing JSC folks.

Comment on attachment 146252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146252&action=review >> Source/WebCore/ChangeLog:11 >> + No test because JSC binding for IndexedDB not working yet. > > I've seen a couple of patches for trying to make IndexedDB work in JSC without testing. Actually I do not want to land a lot of patches without testing just because it is not yet enabled. How much work would be needed by the time we can enable IndexedDB in JSC and make it testable? Kentaro: Doesn't have a new test case doesn't mean it's not tested. Actually the JSC binding for IndexedDB is working in my local environment and working. I've tested them using the existing indexed test cases in LayoutTests/storage/indexeddb. I am trying to land all the indexed patches gradually, with each patch deals with one specific issue. All the problems have been identified and filed as bugs, please look at all blockers of 45110. We need all the blocking issues fixed to make IndexedDB full functional for JSC. >> Source/WebCore/bindings/js/JSMainThreadExecState.cpp:31 >> +#include <IDBPendingTransactionMonitor.h> > > You should use #include "IDBPendingTransactionMonitor.h". Sure.

(In reply to comment #12) > Kentaro: Doesn't have a new test case doesn't mean it's not tested. Actually the JSC binding for IndexedDB is working in my local environment and working. I've tested them using the existing indexed test cases in LayoutTests/storage/indexeddb. I am trying to land all the indexed patches gradually, with each patch deals with one specific issue. All the problems have been identified and filed as bugs, please look at all blockers of 45110. We need all the blocking issues fixed to make IndexedDB full functional for JSC. Sounds nice. Then you might want to describe that in ChangeLog, i.e. what tests you've tested manually in your local environment.

Created attachment 146511 [details] Patch

Comment on attachment 146511 [details] Patch Looks OK to me. JSC folks: would you take a look?

Created attachment 146521 [details] Patch

LGTM.

Comment on attachment 146521 [details] Patch Let me mark r+ based on LGTM from ggaren and jsbell.

Comment on attachment 146521 [details] Patch Clearing flags on attachment: 146521 Committed r119876: <http://trac.webkit.org/changeset/119876>

All reviewed patches have been landed. Closing bug.

FYI, while WebKit's IDB implementation works this way (empty transactions abort) it's actually not in the spec and is not how Firefox behaves (empty transactions complete). I'll be fixing this as part of https://bugs.webkit.org/show_bug.cgi?id=89379 Basically, the same logic is needed though - at end of an excursion into script, IDB needs a callback so it can poke transaction states. It's just the response to that poke that needs to change.

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.