Bug 88283

Created attachment 145684 [details] Patch

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}.

Created attachment 145718 [details] Patch

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.

Created attachment 145754 [details] Patch

(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 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 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.

Created attachment 145755 [details] Patch

Comment on attachment 145755 [details] Patch Looks OK.

Comment on attachment 145755 [details] Patch Thanks for the review, Kentaro.Now send it to commit queue.

Comment on attachment 145755 [details] Patch Clearing flags on attachment: 145755 Committed r119484: <http://trac.webkit.org/changeset/119484>

All reviewed patches have been landed. Closing bug.