Bug 88283 – JSC:need to implement Dictionary::getWithUndefinedOrNullCheck for IDB

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Charles Wei]

We need to implement the JSC version of Dictionary::getWithUndefinedOrNullCheck for IndexedDB, which is referenced in : Modules/indexeddb/IDBDatabase.cpp, to get the KeyPath.

[Charles Wei]

Created attachment 145684 [details]
Patch

[Kentaro Hara]

Comment on attachment 145684 [details]
Patch

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

> Source/WebCore/bindings/js/Dictionary.cpp:82
> +bool Dictionary::getWithUndefinedOrNullCheck(const String& propertyName, String& value) const

This method should behave the same as get() except that getWithUndefinedOrNullCheck() returns false if value is undefined or null. Please refer to bindings/v8/Dictionary.{h,cpp}.

[Charles Wei]

Created attachment 145718 [details]
Patch

[Kentaro Hara]

Comment on attachment 145718 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests, this patch is to make idb work for JSC. All the existing
> +        test cases for idb should apply.

What are the existing test cases? IDB is already enabled on JSC?

> Source/WebCore/bindings/js/Dictionary.cpp:91
> +    JSObject* object = m_dictionary.initializerObject();
> +    ExecState* exec = m_dictionary.execState();
> +    Identifier identifier(exec, propertyName.impl());
> +    JSValue jsValue = object->get(exec, identifier);
>  

More reasonable approach would be

(1) Dictionary::getWithUndefinedOrNullCheck() just calls JSDictionary::getWithUndefinedOrNullCheck(), which you need to newly implement as follows.
(2) JSDictionary::getWithUndefinedOrNullCheck() calls JSDictionary::tryGetPropertyAndResult(..., true), where the last argument indicates to check undefined or null.
(3) Modify JSDictionary::tryGetPropertyAndResult(..., bool) to support the last bool argument. If it is true, check undefined or null.

[Charles Wei]

Created attachment 145754 [details]
Patch

[Charles Wei]

(In reply to comment #4)
> (From update of attachment 145718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145718&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        No new tests, this patch is to make idb work for JSC. All the existing
> > +        test cases for idb should apply.
> 
> What are the existing test cases? IDB is already enabled on JSC?
> 
> > Source/WebCore/bindings/js/Dictionary.cpp:91
> > +    JSObject* object = m_dictionary.initializerObject();
> > +    ExecState* exec = m_dictionary.execState();
> > +    Identifier identifier(exec, propertyName.impl());
> > +    JSValue jsValue = object->get(exec, identifier);
> >  
> 
> More reasonable approach would be
> 
> (1) Dictionary::getWithUndefinedOrNullCheck() just calls JSDictionary::getWithUndefinedOrNullCheck(), which you need to newly implement as follows.
> (2) JSDictionary::getWithUndefinedOrNullCheck() calls JSDictionary::tryGetPropertyAndResult(..., true), where the last argument indicates to check undefined or null.
> (3) Modify JSDictionary::tryGetPropertyAndResult(..., bool) to support the last bool argument. If it is true, check undefined or null.

Thanks for the review, Kentaro.  I just submitted a patch which is simpler and not changing any existing code of JSDictionary except the new added getWithUndefinedOrNullCheck().

[Kentaro Hara]

Comment on attachment 145754 [details]
Patch

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

The patch looks OK.

> Source/WebCore/ChangeLog:8
> +        No new tests, idb not working for JSC yet.

Did you manually confirm that getWithUndefinedOrNullCheck() works as expected? (You do not need to add a test, but might want to somehow confirm that it works.)

> Source/WebCore/bindings/js/Dictionary.h:64
> +    bool getWithUndefinedOrNullCheck(const String&, String&) const;

Nit: getWithUndefinedOrNullCheck(const String& propertyName, String& value) would be better to distinguish variables. WebKit omits an argument name only if it is obvious.

[Charles Wei]

Comment on attachment 145754 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests, idb not working for JSC yet.
> 
> Did you manually confirm that getWithUndefinedOrNullCheck() works as expected? (You do not need to add a test, but might want to somehow confirm that it works.)

Yes, of course.  I manually tested with correct value, null, undefined, no property, etc. All work as expected.

>> Source/WebCore/bindings/js/Dictionary.h:64
>> +    bool getWithUndefinedOrNullCheck(const String&, String&) const;
> 
> Nit: getWithUndefinedOrNullCheck(const String& propertyName, String& value) would be better to distinguish variables. WebKit omits an argument name only if it is obvious.

Ok, I can add that to make it clear.

[Charles Wei]

Created attachment 145755 [details]
Patch

[Kentaro Hara]

Comment on attachment 145755 [details]
Patch

Looks OK.

[Charles Wei]

Comment on attachment 145755 [details]
Patch

Thanks for the review, Kentaro.Now send it to commit queue.

[WebKit Review Bot]

Comment on attachment 145755 [details]
Patch

Clearing flags on attachment: 145755

Committed r119484: <http://trac.webkit.org/changeset/119484>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.