As opposed to JSDictionary used for generated bindings, Dictionary exposes JS dictionaries to WebCore objects. The interface differs between the two, so we can't have JSDictionary work for both without having a confusing interface, and also exposing unneeded information to the WebCore layer.
<rdar://problem/11209766>
Created attachment 136625 [details] Patch
Comment on attachment 136625 [details] Patch Attachment 136625 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12384652
Comment on attachment 136625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136625&action=review > Source/WebCore/bindings/js/Dictionary.h:42 > + Dictionary() Is this constructor needed? > Source/WebCore/bindings/js/Dictionary.h:45 > + { } Nit: '{' and '}' for each line, please > Source/WebCore/bindings/js/Dictionary.h:50 > + { } Nit: Ditto. > Source/WebCore/bindings/js/Dictionary.h:68 > + if (!m_isValid) > + return false; Maybe we want to move this check to JSDictionary.h, like this: bool JSDictionary::tryGetProperty(...) { if (!m_initializerObject) return false; ...; } Then you can remove m_isValid from Dictionary.h. > Source/WebCore/bindings/js/Dictionary.h:71 > + bool noExceptions = m_dictionary.tryGetProperty(propertyName.ascii().data(), result, getPropertyResult); Why do we need both 'getPropertyResult' and 'noExceptions'? Isn't it possible to have tryGetProperty return 'getPropertyResult' as a return value? (If the return value is 'ExceptionThrown', it means that there is an exception.)
Comment on attachment 136625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136625&action=review >> Source/WebCore/bindings/js/Dictionary.h:42 >> + Dictionary() > > Is this constructor needed? The existence of the default constructor in the v8 implementation led me to believe this was required. Obviously nobody is using Dictionary right now except IDB, so this could be removed until needed. >> Source/WebCore/bindings/js/Dictionary.h:50 >> + { } > > Nit: Ditto. I didn't think inline empty functions or constructors required this. The indenting for initializing the variables is wrong though. >> Source/WebCore/bindings/js/Dictionary.h:68 >> + return false; > > Maybe we want to move this check to JSDictionary.h, like this: > > bool JSDictionary::tryGetProperty(...) { > if (!m_initializerObject) > return false; > ...; > } > > Then you can remove m_isValid from Dictionary.h. I think I might have to expose the exec state now that i'm trying to use it with notifications, so it's possible i can do the check that way and avoid adding this variable. >> Source/WebCore/bindings/js/Dictionary.h:71 >> + bool noExceptions = m_dictionary.tryGetProperty(propertyName.ascii().data(), result, getPropertyResult); > > Why do we need both 'getPropertyResult' and 'noExceptions'? Isn't it possible to have tryGetProperty return 'getPropertyResult' as a return value? (If the return value is 'ExceptionThrown', it means that there is an exception.) I didn't want to change the existing signatures in JSDictionary because the bindings generation code relies on it, and I didn't want this bug to include having to completely overhaul that. I was afraid about making the API for JSDictionary inconsistent and confusing, but I think I found a way to make it work. This patch is also missing new expected results for the bindings tests, which I will include in a forthcoming patch.
(In reply to comment #5) > >> Source/WebCore/bindings/js/Dictionary.h:50 > >> + { } > > > > Nit: Ditto. > > I didn't think inline empty functions or constructors required this. The indenting for initializing the variables is wrong though. Nit: Please look at the coding style guide ("Other punctuations" 1.): http://www.webkit.org/coding/coding-style.html I think whether inline or not does not matter.
Created attachment 136852 [details] Patch
Comment on attachment 136852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136852&action=review > Source/WebCore/bindings/js/JSDictionary.h:120 > +bool JSDictionary::valueForProperty(const char* propertyName, Result& finalResult) const Nit: Maybe this can be renamed to JSDictionary::get(), for naming consistency with Dictionary::get(). It's up to you.
Committed r114001: <http://trac.webkit.org/changeset/114001>