[webkit-reviews] review denied: [Bug 134721] Move dir attribute from HTMLDocument to Document to match the spec : [Attachment 234615] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Prashant Hiremath
<hiremathprashants at gmail.com>'s request for review:
Bug 134721: Move dir attribute from HTMLDocument to Document to match the spec
https://bugs.webkit.org/show_bug.cgi?id=134721

Attachment 234615: Patch
https://bugs.webkit.org/attachment.cgi?id=234615&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=234615&action=review


> Source/WebCore/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

You need a blank line after this.

> Source/WebCore/ChangeLog:8
> +	   http://www.w3.org/html/wg/drafts/html/master/dom.html#dom-dir

Please refer to a specific version of the spec since WG could be updated in the
future.

> Source/WebCore/ChangeLog:9
> +	   This is a blinkMerge from
https://src.chromium.org/viewvc/blink?revision=171529&view=revision

should read "a blink merge".

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:112
>  - (DOMElement *)createElement:(NSString *)tagName;

This attribute is certainly not available in OS X 10.5. r- because of this.

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:-456
> - at property (copy) NSString *dir WEBKIT_AVAILABLE_MAC(10_5);

I don't think we can make this change safely.

> LayoutTests/fast/dom/dir-non-html-document.html:12
> +var xmlDocument = document.implementation.createDocument("", "", null);
> +shouldBe("xmlDocument.__proto__", "Document.prototype");

Please test a SVG document as well as a MathML document to ensure
assigning a value doesn't modify SVG and/or MathML document.

You also need to fix
fast/dom/assign-to-prototype-accessor-on-prototype-should-be-silent.html
as it assumes that Document.prototype.dir doesn't exist but the change set
http://trac.webkit.org/r168385
makes me think that this patch may not be Web compatible to begin with.