Bug 88283 - JSC:need to implement Dictionary::getWithUndefinedOrNullCheck for IDB
Summary: JSC:need to implement Dictionary::getWithUndefinedOrNullCheck for IDB
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charles Wei
URL:
Keywords:
Depends on:
Blocks: 45110
  Show dependency treegraph
 
Reported: 2012-06-04 20:30 PDT by Charles Wei
Modified: 2012-06-05 05:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2012-06-04 20:36 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (2.57 KB, patch)
2012-06-05 01:10 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (3.84 KB, patch)
2012-06-05 04:17 PDT, Charles Wei
no flags Details | Formatted Diff | Diff
Patch (3.86 KB, patch)
2012-06-05 04:40 PDT, Charles Wei
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Wei 2012-06-04 20:30:53 PDT
We need to implement the JSC version of Dictionary::getWithUndefinedOrNullCheck for IndexedDB, which is referenced in : Modules/indexeddb/IDBDatabase.cpp, to get the KeyPath.
Comment 1 Charles Wei 2012-06-04 20:36:00 PDT
Created attachment 145684 [details]
Patch
Comment 2 Kentaro Hara 2012-06-04 20:43:53 PDT
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}.
Comment 3 Charles Wei 2012-06-05 01:10:30 PDT
Created attachment 145718 [details]
Patch
Comment 4 Kentaro Hara 2012-06-05 01:23:38 PDT
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.
Comment 5 Charles Wei 2012-06-05 04:17:05 PDT
Created attachment 145754 [details]
Patch
Comment 6 Charles Wei 2012-06-05 04:22:41 PDT
(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().
Comment 7 Kentaro Hara 2012-06-05 04:30:25 PDT
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.
Comment 8 Charles Wei 2012-06-05 04:36:17 PDT
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.
Comment 9 Charles Wei 2012-06-05 04:40:05 PDT
Created attachment 145755 [details]
Patch
Comment 10 Kentaro Hara 2012-06-05 04:40:28 PDT
Comment on attachment 145755 [details]
Patch

Looks OK.
Comment 11 Charles Wei 2012-06-05 04:42:56 PDT
Comment on attachment 145755 [details]
Patch

Thanks for the review, Kentaro.Now send it to commit queue.
Comment 12 WebKit Review Bot 2012-06-05 05:28:05 PDT
Comment on attachment 145755 [details]
Patch

Clearing flags on attachment: 145755

Committed r119484: <http://trac.webkit.org/changeset/119484>
Comment 13 WebKit Review Bot 2012-06-05 05:28:27 PDT
All reviewed patches have been landed.  Closing bug.