Use Dictionary in MutationObserver.observe to kill custom code
Created attachment 148707 [details] Patch
Comment on attachment 148707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review > Source/WebCore/dom/WebKitMutationObserver.cpp:95 > + if (optionsDictionary.isUndefinedOrNull()) { > + ec = TYPE_MISMATCH_ERR; > + return; > + } I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back.
(In reply to comment #2) > (From update of attachment 148707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review > > > Source/WebCore/dom/WebKitMutationObserver.cpp:95 > > + if (optionsDictionary.isUndefinedOrNull()) { > > + ec = TYPE_MISMATCH_ERR; > > + return; > > + } > > I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back. It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html). Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object: http://www.w3.org/TR/WebIDL/#es-dictionary The code as I've written it is maintains the pre-existing behavior.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 148707 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review > > > > > Source/WebCore/dom/WebKitMutationObserver.cpp:95 > > > + if (optionsDictionary.isUndefinedOrNull()) { > > > + ec = TYPE_MISMATCH_ERR; > > > + return; > > > + } > > > > I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back. > > It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html). Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object: > > http://www.w3.org/TR/WebIDL/#es-dictionary > > The code as I've written it is maintains the pre-existing behavior. Hmm, but you're quite right, the generated code does try to do this check. Investigating further...
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 148707 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=148707&action=review > > > > > > > Source/WebCore/dom/WebKitMutationObserver.cpp:95 > > > > + if (optionsDictionary.isUndefinedOrNull()) { > > > > + ec = TYPE_MISMATCH_ERR; > > > > + return; > > > > + } > > > > > > I guess that this check would be already done in the generated code before WebKitMutationObserver::observe() is called back. > > > > It's not. If I leave this out, I get the SYNTAX_ERR due to validateOptions failing (this is covered by fast/mutation/observe-exceptions.html). Per WebIDL, it seems we're actually supposed to throw a TypeError if the passed-in dictionary isn't an Object: > > > > http://www.w3.org/TR/WebIDL/#es-dictionary > > > > The code as I've written it is maintains the pre-existing behavior. > > Hmm, but you're quite right, the generated code does try to do this check. Investigating further... Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined: if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) { ec = TYPE_MISMATCH_ERR; V8Proxy::setDOMException(ec, args.GetIsolate()); return V8Proxy::throwTypeError("Not an object."); } Should we change CodeGenerator to do something different here?
(In reply to comment #5) > Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined: > > if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) { > ec = TYPE_MISMATCH_ERR; > V8Proxy::setDOMException(ec, args.GetIsolate()); > return V8Proxy::throwTypeError("Not an object."); > } > > Should we change CodeGenerator to do something different here? Yeah, fixing the condition sounds reasonable. Also, per the spec, we do not need to set TYPE_MISMATCH_ERR, we just need to throw TypeError. (Note: TYPE_MISMATCH_ERR and TypeError are different things.) If there is no concern about compatibility, we might want to remove TYPE_MISMATCH_ERR.
(In reply to comment #6) > (In reply to comment #5) > > Oh, I just misread the generated code. It passes on undefined or null options, apparently it only catches the case of non-Object, non-null, non-undefined: > > > > if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) { > > ec = TYPE_MISMATCH_ERR; > > V8Proxy::setDOMException(ec, args.GetIsolate()); > > return V8Proxy::throwTypeError("Not an object."); > > } > > > > Should we change CodeGenerator to do something different here? > > Yeah, fixing the condition sounds reasonable. Okay. > Also, per the spec, we do not need to set TYPE_MISMATCH_ERR, we just need to throw TypeError. (Note: TYPE_MISMATCH_ERR and TypeError are different things.) If there is no concern about compatibility, we might want to remove TYPE_MISMATCH_ERR. Yeah, I know they're different, but I don't think there are compat concerns here. I'd like to do both of the aforementioned things in separate changes, though, since they both could change behavior. This patch does not change behavior as-is.
(In reply to comment #6) > > if (args.Length() > 1 && !options.isUndefinedOrNull() && !options.isObject()) This condition looks really strange. Maybe it could be: if (options.isUndefinedOrNull() || options.isObject()) I think we do not need to check args.Length(), since it is already handled by MAYBE_MISSING_PARAMETER() macro.
Comment on attachment 148707 [details] Patch Clearing flags on attachment: 148707 Committed r120900: <http://trac.webkit.org/changeset/120900>
All reviewed patches have been landed. Closing bug.