[webkit-reviews] review denied: [Bug 43400] Add priority attribute to XMLHttpRequest : [Attachment 63779] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied James Simonsen
<simonjam+webkit at google.com>'s request for review:
Bug 43400: Add priority attribute to XMLHttpRequest
https://bugs.webkit.org/show_bug.cgi?id=43400

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=63779&action=prettypatch

Nits below.  I don't have an opinion on whether WebKit wants this feature.

> WebCore/xml/XMLHttpRequest.cpp:176
> +#endif // ENABLE(XMLHTTPREQUEST_PRIORITY)
We don't need the comment here.

> WebCore/xml/XMLHttpRequest.cpp:406
> +void XMLHttpRequest::setPriority(const String& s, ExceptionCode& ec)
Please don't use one-letter variables names.  Perhaps s => priority ?

> WebCore/xml/XMLHttpRequest.cpp:413
> +    for (int i = 0; i < 5; ++i) {
The magic constant 5 seems quite decoupled from the size of the array.	Do we
have an arraysize macro in this code?  Failing that, a global constant used in
place of each instance of 5 would be preferable.

> WebCore/xml/XMLHttpRequest.cpp:418
> +    }
I'd assert the invariants of m_priority at the end of this function, just to be
clear about what's going on.