Bug 45110 – Implement JSC features required for IndexedDB

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

In order for IndexedDB to work on JSC, we need to implement several features.  So far:
* SerializedScriptValue needs to be serializable to a "wire" format
* We need to support the Static extended attribute (being added in https://bugs.webkit.org/show_bug.cgi?id=45044)

[Jeremy Orlow]

*** Bug 43863 has been marked as a duplicate of this bug. ***

[Jeremy Orlow]

Note also that currently all work happens on the main thread.  This is OK for Chromium because of our multi-process architecture (which runs IndexedDB on a "main thread" in a different process), but we'll need to actually do the work on a background process for any other implementation (including those using V8).

[Robin Qiu]

Is anybody fixing this bug? If nobody, I would like to have a try.

[Jeremy Orlow]

I'm pretty sure no one has started on this yet.  If you'd like to, that'd be great!

Note that in addition to what's already listed:
* the build files probably need to be updates
and
* JSC should have custom bindings for everything we have v8 custom bindings (and you should double check the JSC ones have similar logic)

For the serializedScriptValue wire format, I'd suggest trying to re-use the v8 SSV code.  My hunch is that you could split it into 2 fairly small JSC/V8 specific classes + a big template class with most of the logic.  Not only would this be more maintainable, but it'd also allow implementations to switch between v8/jsc without needing to convert all their IndexedDB data.

Note that we're only about 80% done with the overall API.  We're hoping to have that done before too long, but lately we've been spending a lot of time tracking down some ugly bugs.

[Robin Qiu]

(In reply to comment #4)
> I'm pretty sure no one has started on this yet.  If you'd like to, that'd be great!
> 

Thanks for your advices.

[Robin Qiu]

Some questions:

1. In many IndexedDB classes, object parameters are passed in ref, but JSC binding code, which are automatically generated, is in point, except changing code like this, is there another way to fix this? :

Source/WebCore/storage/IDBObjectStore.h: (see OptionsObject)
-    PassRefPtr<IDBIndex> createIndex(const String& name, const String& keyPath, const OptionsObject&, ExceptionCode&);
+    PassRefPtr<IDBIndex> createIndex(const String& name, const String& keyPath, OptionsObject*, ExceptionCode&);

2. OptionsObject is not implemented in JSC, I plan to add OptionObject.idl, OptionObject.cpp/h, and use JSC::JSValue to take place of v8::Local<v8::Value>.
Is this all right?

3. In JSC, WebCore::SerializedScriptValue is not derived from ThreadSafeShared<> but from RefCounted<>. In IDB classes, it is assumed to be ThreadSafeShared<>, but in other classes, it is assumed to be RefCounted<>. How to deal with this? multi-inheritance?

[Jeremy Orlow]

(In reply to comment #6)
> Some questions:
> 
> 1. In many IndexedDB classes, object parameters are passed in ref, but JSC binding code, which are automatically generated, is in point, except changing code like this, is there another way to fix this? :
> 
> Source/WebCore/storage/IDBObjectStore.h: (see OptionsObject)
> -    PassRefPtr<IDBIndex> createIndex(const String& name, const String& keyPath, const OptionsObject&, ExceptionCode&);
> +    PassRefPtr<IDBIndex> createIndex(const String& name, const String& keyPath, OptionsObject*, ExceptionCode&);

The options object is not ref-counted, so I'm pretty sure the const & version is the right one.  There's probably some list in the generator that you need to add it to.  I believe I had to do something similar for v8, but the code bases are pretty different these days.

> 2. OptionsObject is not implemented in JSC, I plan to add OptionObject.idl, OptionObject.cpp/h, and use JSC::JSValue to take place of v8::Local<v8::Value>.
> Is this all right?

I don't think you need an IDL.  I implemented it much like how SerializedScriptValue is implemented, IIRC: i.e. it's a first class citizen in the bindings generator.  Maybe look at my patch that added OO?

Otherwise copying and changing the v8 specific parts probably makes sense (unless you can think of a clean way to share the code...but I can't off the top of my head).
 
> 3. In JSC, WebCore::SerializedScriptValue is not derived from ThreadSafeShared<> but from RefCounted<>. In IDB classes, it is assumed to be ThreadSafeShared<>, but in other classes, it is assumed to be RefCounted<>. How to deal with this? multi-inheritance?

I'm not sure what you mean by multi-inheritance.

I think V8's SerializedScriptValue should have a |threadSafeCopy()| method.  It would call |m_data.crossThreadString()| and then clone itself.

Not sure what we can do other than a copy for JSC's.  (Do you know why the latter uses a vector?  Would it be possible for it to use a String?)

[Donggwan Kim]

Is there any progress on this patch? I'm interested in this patch.

[Robin Qiu]

To be honest, I didn't really get start. So, please feel free to take it over.

[Donggwan Kim]

I have a question about implementing IDBBindingUtilities for JSC. IDBBindingUtilities.cpp have createIDBKeyFromSerializedValueAndKeyPath and injectIDBKeyIntoSerializedValue in V8
To implement it for JSC, i should pass ExecState* to that methods but i couldn't pass to them because there is no way to pass it. Do you have any idea for that?

[Donggwan Kim]

Created attachment 120546 [details]
first patch

Patch

[WebKit Review Bot]

Attachment 120546 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/storage/IDBFactoryBackendInterface.h:59:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendInterface.h:60:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendInterface.h:61:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendImpl.h:61:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendImpl.h:62:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendImpl.h:63:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/IDBFactoryBackendImpl.h:68:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Donggwan Kim]

Created attachment 120549 [details]
Patch

Fix style check problem

[WebKit Review Bot]

Comment on attachment 120549 [details]
Patch

Attachment 120549 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11035076

[Adam Barth]

Comment on attachment 120549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120549&action=review

> Source/WebCore/storage/IDBKeyPathBackendImpl.cpp:51
> +#if USE(JSC)

WebCore proper shouldn't have any JSC specific code.  All the JSC specific code should be in the bindings.

[Donggwan Kim]

I understand what you meant. 

However, without including USE(JSC), I couldn't find any way that enables KeyPathBackendImpl to use IDBBindingUtilities methods (e.g., createIDBKeyFromSerializedValueAndKeyPath and injectIDBKeyIntoSerializedValue).


In JSC, the above JSC binding mothods require ExecState as a parameter unlike to V8. Therefore, I have appended USE(JSC) into the WebCore codes. If you have any idea without needing to include USE(JSC) for this purpose, I would be happy to know about it. 

Thanks,

[Sam Weinig]

Comment on attachment 120549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120549&action=review

This of course needs a changelog.

> Source/WebCore/bindings/js/OptionsObject.h:58
> +class OptionsObject {
> +public:
> +    OptionsObject();
> +    OptionsObject(JSC::ExecState*, const JSC::JSValue& options);
> +
> +    OptionsObject& operator=(const OptionsObject&);
> +
> +    bool get(const String&, bool&) const;
> +    bool get(const String&, int32_t&) const;
> +    bool get(const String&, double&) const;
> +    bool get(const String&, String&) const;
> +    bool get(const String&, unsigned short&) const;
> +    bool get(const String&, unsigned&) const;
> +    bool get(const String&, unsigned long long&) const;
> +
> +    bool getWithUndefinedOrNullCheck(const String&, String&) const;
> +
> +    ~OptionsObject();
> +
> +private:
> +    bool getKey(const String& key, JSC::JSValue&) const;
> +
> +    JSC::ExecState* m_exec;
> +    JSC::JSValue m_options;
> +};

The JSC bindings have a class called JSDictionary which has much of the same functionality has this, I would rather we didn't duplicate this code.

> Source/WebCore/bindings/js/SerializedScriptValue.h:77
>      String toString();
> +    String toWireString();

It would probably make sense to add a comment explaining what the differences between toString and toWireString are.

>> Source/WebCore/storage/IDBKeyPathBackendImpl.cpp:51
>> +#if USE(JSC)
> 
> WebCore proper shouldn't have any JSC specific code.  All the JSC specific code should be in the bindings.

Indeed, why does JSC need special cased code here?

> Source/WebCore/storage/IDBTransactionBackendImpl.h:98
> +    ScriptExecutionContext* m_scriptExecutionContext;

What ensures the lifetime of the ScriptExecutionContext here?

[Donggwan Kim]

Created attachment 120854 [details]
Patch

[WebKit Review Bot]

Comment on attachment 120854 [details]
Patch

Attachment 120854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11024322

[Donggwan Kim]

Created attachment 120859 [details]
Patch

Fix chromium build error and append comment to toString and toWireString of SerializedScriptValue.h

[Donggwan Kim]

(In reply to comment #17)
> (From update of attachment 120549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120549&action=review
> 
> This of course needs a changelog.

I appended it.

> 
> > Source/WebCore/bindings/js/OptionsObject.h:58
> > +class OptionsObject {
> > +public:
> > +    OptionsObject();
> > +    OptionsObject(JSC::ExecState*, const JSC::JSValue& options);
> > +
> > +    OptionsObject& operator=(const OptionsObject&);
> > +
> > +    bool get(const String&, bool&) const;
> > +    bool get(const String&, int32_t&) const;
> > +    bool get(const String&, double&) const;
> > +    bool get(const String&, String&) const;
> > +    bool get(const String&, unsigned short&) const;
> > +    bool get(const String&, unsigned&) const;
> > +    bool get(const String&, unsigned long long&) const;
> > +
> > +    bool getWithUndefinedOrNullCheck(const String&, String&) const;
> > +
> > +    ~OptionsObject();
> > +
> > +private:
> > +    bool getKey(const String& key, JSC::JSValue&) const;
> > +
> > +    JSC::ExecState* m_exec;
> > +    JSC::JSValue m_options;
> > +};
> 
> The JSC bindings have a class called JSDictionary which has much of the same functionality has this, I would rather we didn't duplicate this code.

OptionsObject is exposed to WebCore. so I think we should keep this interface like V8.

> 
> > Source/WebCore/bindings/js/SerializedScriptValue.h:77
> >      String toString();
> > +    String toWireString();
> 
> It would probably make sense to add a comment explaining what the differences between toString and toWireString are.

I appended comment to it.

> 
> >> Source/WebCore/storage/IDBKeyPathBackendImpl.cpp:51
> >> +#if USE(JSC)
> > 
> > WebCore proper shouldn't have any JSC specific code.  All the JSC specific code should be in the bindings.
> 
> Indeed, why does JSC need special cased code here?

Two methods createIDBKeyFromSerializedValueAndKeyPath and injectIDBKeyIntoSerializedValue in IDBKeyPathBackendImpl have functionality that create and inject key from and into SerializedScriptValue. To do so, they should use bindings method of SerializedScriptValue. In JSC bindings, they require ExecState as parameter unlike V8.
> 
> > Source/WebCore/storage/IDBTransactionBackendImpl.h:98
> > +    ScriptExecutionContext* m_scriptExecutionContext;
> 
> What ensures the lifetime of the ScriptExecutionContext here?
The lifetime of the ScriptExecutionContext is same as document.

[WebKit Review Bot]

Comment on attachment 120859 [details]
Patch

Attachment 120859 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11028324

[Donggwan Kim]

Created attachment 120864 [details]
Patch

Fix chromium build error

[WebKit Review Bot]

Comment on attachment 120864 [details]
Patch

Attachment 120864 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11056763

[Jeff Rogers]

Just curios if there is still working going on for this patch?

[Donggwan Kim]

I have to fix chromium build error, but not yet fixed. I will update fixed patch as soon as possible. Thank you for your attention.

[Joshua Bell]

https://bugs.webkit.org/show_bug.cgi?id=80207 looks like a duplicated effort on this. Can these be consolidated?

[Joshua Bell]

Comment on attachment 120864 [details]
Patch

Given the changes in http://trac.webkit.org/changeset/110539 this patch would need to be significantly reworked.

[Charles Wei]

I would like to break this down to a serial of small bugs/patches with each one deals with a specific issue

[Charles Wei]

https://bugs.webkit.org/show_bug.cgi?id=88287  is the only outstanding bug that blocks indexeddb from working for JSC. anybody can help with pushing it forward by a review?

[Michael Pruett]

Created attachment 161701 [details]
Implement IndexedDB for JSC

[WebKit Review Bot]

Attachment 161701 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/Modules/indexeddb/IDBKeyPathBackendImpl.cpp:45:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/Modules/indexeddb/IDBKeyPathBackendImpl.cpp:45:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/Modules/indexeddb/IDBKeyPathBackendImpl.cpp:47:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:46:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:98:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:98:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:128:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:128:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:169:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:169:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:186:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:186:  Missing spaces around <  [whitespace/operators] [3]
Source/WebCore/bindings/js/IDBBindingUtilities.cpp:202:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.h:66:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 15 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Pruett]

Created attachment 161713 [details]
Implement IndexedDB for JSC

With this patch, WebKitGTK+ passes 149 of the 184 tests in storage/indexeddb.

[Joshua Bell]

Comment on attachment 161713 [details]
Implement IndexedDB for JSC

View in context: https://bugs.webkit.org/attachment.cgi?id=161713&action=review

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.h:66
> +    virtual void postSuccessHandlerCallback() { }

This is fine - in the Chromium port there's an extra layer in place which uses this callback as part of a cursor pre-fetching mechanism, so making the back end be a no-op here is good.

> Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:32
> +        readonly attribute [V8CustomGetter, JSCustomGetter, CachedAttribute] IDBAny value;

Drive-by comment: we had to use a custom getter for V8 because although we want the value to be re-used across script accesses it needed a mechanism to be invalidated by the cursor. Not sure if CachedAttribute is buying anything (but if it is simplifying code, great!).

> Source/WebCore/Modules/indexeddb/IDBKeyPathBackendImpl.cpp:43
> +void IDBKeyPathBackendImpl::createIDBKeysFromSerializedValuesAndKeyPath(const Vector<RefPtr<SerializedScriptValue>, 0>& values, const IDBKeyPath& keyPath, Vector<RefPtr<IDBKey>, 0>& keys)

This file is being removed in https://bugs.webkit.org/show_bug.cgi?id=95385 (it is no longer needed)

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:76
> +static PassRefPtr<IDBKey> createIDBKeyFromValue(ExecState* exec, JSValue value, Vector<JSArray*>& stack)

Some of this may be obsoleted by:
https://bugs.webkit.org/show_bug.cgi?id=94023
https://bugs.webkit.org/show_bug.cgi?id=95409
... which attempt to optimize the pattern of converting ScriptValue -> SerializedScriptValue during binding, then converting SSV back to SV for these key operations, then (sometimes) re-serializing as SSV.

[Michael Pruett]

(In reply to comment #34)
> Drive-by comment: we had to use a custom getter for V8 because although we want the value to be re-used across script accesses it needed a mechanism to be invalidated by the cursor. Not sure if CachedAttribute is buying anything (but if it is simplifying code, great!).

I have taken the same approach with the JSC bindings. Although it is necessary to have a custom getter to invalidate the cached value when the cursor value is dirty, CachedAttribute is required for the JSC binding generator to declare the cached m_value member in the JSIDBCursorWithValue class.

[Joshua Bell]

(In reply to comment #34)
> 
> Some of this may be obsoleted by:
> https://bugs.webkit.org/show_bug.cgi?id=94023
> https://bugs.webkit.org/show_bug.cgi?id=95409
> ... which attempt to optimize the pattern of converting ScriptValue -> SerializedScriptValue during binding, then converting SSV back to SV for these key operations, then (sometimes) re-serializing as SSV.

This is all in now, so this bug should be unblocked. I believe we're done tinkering with these fundamentals, so now is a great time to revisit this.

[Michael Pruett]

Created attachment 169575 [details]
Implement IndexedDB for JSC

[WebKit Review Bot]

Comment on attachment 169575 [details]
Implement IndexedDB for JSC

Attachment 169575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14459408

[Michael Pruett]

Created attachment 169577 [details]
Implement IndexedDB for JSC

[WebKit Review Bot]

Comment on attachment 169577 [details]
Implement IndexedDB for JSC

Attachment 169577 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14412945

[Michael Pruett]

Created attachment 169581 [details]
Implement IndexedDB for JSC

[Build Bot]

Comment on attachment 169581 [details]
Implement IndexedDB for JSC

Attachment 169581 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14424903

New failing tests:
fast/events/constructors/message-event-constructor.html
fast/files/blob-constructor.html
fast/events/constructors/custom-event-constructor.html
fast/events/constructors/event-constructors.html
fast/dom/Geolocation/argument-types.html
fast/events/constructors/pop-state-event-constructor.html
fast/events/constructors/storage-event-constructor.html

[Michael Pruett]

Created attachment 169813 [details]
Implement IndexedDB for JSC

With this patch, WebKitGTK+ passes 121 of the 202 tests in storage/indexeddb.

[Joshua Bell]

Comment on attachment 169813 [details]
Implement IndexedDB for JSC

View in context: https://bugs.webkit.org/attachment.cgi?id=169813&action=review

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:149
> +        RefPtr<IDBKey> keyPathKey = createIDBKeyFromScriptValueAndKeyPath(context, value, keyPath);

These changes look complimentary to the patch over in https://bugs.webkit.org/show_bug.cgi?id=99975 - watch for collisions.

> Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:30
> +    [V8CustomGetter, JSCustomGetter, CachedAttribute] readonly attribute IDBAny value;

Does CachedAttribute gain anything here? In the V8 case the V8CustomGetter is used as CachedAttribute doesn't let us "dirty" the values.

(As an alternative approach, I was pondering having IDBCursor's properties be "any" in the IDL and just a ScriptValue in the C++, but haven't played with that yet.)

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
> +    const IDBIndexMetadata& indexMetadata,

Indentation here is odd.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1845
> +    if (m_data.size() & 1) {

Looks like a similar fix to https://bugs.webkit.org/show_bug.cgi?id=99310

Include the test case?

[Joshua Bell]

> > Source/WebCore/Modules/indexeddb/IDBCursorWithValue.idl:30
> > +    [V8CustomGetter, JSCustomGetter, CachedAttribute] readonly attribute IDBAny value;

> (As an alternative approach, I was pondering having IDBCursor's properties be "any" in the IDL and just a ScriptValue in the C++, but haven't played with that yet.)

FYI: https://bugs.webkit.org/show_bug.cgi?id=100034

[Michael Pruett]

Created attachment 173148 [details]
Implement IndexedDB for JSC

I've updated this patch in accordance with other recent changes to IndexedDB. In particular I've removed the custom bindings for IDBCursorWithValue.

> > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
> > +    const IDBIndexMetadata& indexMetadata,
>
> Indentation here is odd.

Unfortunately the indentation in the file currently no longer passes check-webkit-style, so retaining the current indentation would seem not to be an option. I would be happy to change the indentation to any style you suggest.

> > Source/WebCore/bindings/js/SerializedScriptValue.cpp:1845
> > +    if (m_data.size() & 1) {
>
> Looks like a similar fix to https://bugs.webkit.org/show_bug.cgi?id=99310
>
> Include the test case?

The only place the JSC implementation of SerializedScriptValue::toWireString() is called is in IDBObjectStoreBackendImpl::putInternal(). This change is covered by numerous tests in storage/indexeddb, including the following which fail without this change:

storage/indexeddb/cursor-delete.html
storage/indexeddb/cursor-inconsistency.html
storage/indexeddb/cursor-key-order.html
storage/indexeddb/cursor-prev-no-duplicate.html
storage/indexeddb/cursor-value.html
storage/indexeddb/database-close.html
storage/indexeddb/factory-deletedatabase.html
storage/indexeddb/index-multientry.html
storage/indexeddb/index-population.html
storage/indexeddb/mutating-cursor.html
storage/indexeddb/mozilla/put-get-values.html
storage/indexeddb/odd-strings.html
storage/indexeddb/open-cursor.html
storage/indexeddb/transaction-rollback.html
storage/indexeddb/two-version-changes.html
storage/indexeddb/value-undefined.html
storage/indexeddb/values-odd-types.html

[Joshua Bell]

(In reply to comment #46)
> > > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:105
> > > +    const IDBIndexMetadata& indexMetadata,
> >
> > Indentation here is odd.
> 
> Unfortunately the indentation in the file currently no longer passes check-webkit-style, so retaining the current indentation would seem not to be an option. I would be happy to change the indentation to any style you suggest.

Yep, the file is currently incorrect. Just remove the line breaks.

[Charles Wei]

Comment on attachment 173148 [details]
Implement IndexedDB for JSC

View in context: https://bugs.webkit.org/attachment.cgi?id=173148&action=review

General comments:  it would make it easier to break up the big patches to series of small patches, with each one targetting to solve one specific problem.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:48
> +class IDBJSContext {

For this, please refer 88287 which has similar solution but goes no where there waiting for review for a long time. Wish you good luck with this :-)

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1849
> +    }

For this part, please refer: 99310.

[Charles Wei]

View in context: https://bugs.webkit.org/attachment.cgi?id=173148&action=review

General comments:  it would make it easier to break up the big patches to series of small patches, with each one targetting to solve one specific problem.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:48
> +class IDBJSContext {

For this, please refer 88287 which has similar solution but goes no where there waiting for review for a long time. Wish you good luck with this :-)

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1849
> +    }

For this part, please refer: 99310.

[Adam Barth]

Comment on attachment 173148 [details]
Implement IndexedDB for JSC

View in context: https://bugs.webkit.org/attachment.cgi?id=173148&action=review

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:73
> +    ScriptState* m_scriptState;

Given that we're in the JSC bindings here, we should probably use the type ExecState* for this member.

[Martin Robinson]

All dependent bugs are fixed now, so can this be closed?

[Alexey Proskuryakov]

Seems like that.