[webkit-reviews] review denied: [Bug 106243] ER: new accessibility layout test to verify levels of headings : [Attachment 182261] layout test patch and changelog (unreviewed)

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has denied James Craig
<james at cookiecrook.com>'s request for review:
Bug 106243: ER: new accessibility layout test to verify levels of headings
https://bugs.webkit.org/show_bug.cgi?id=106243

Attachment 182261: layout test patch and changelog (unreviewed)
https://bugs.webkit.org/attachment.cgi?id=182261&action=review

------- Additional Comments from chris fleizach <cfleizach at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=182261&action=review


test looks like its failing on chromium because they implement intValue() as 
void AccessibilityUIElement::intValueGetterCallback(CppVariant* result)
{
    result->set(accessibilityObject().valueForRange());
}

and we do something different.

I would skip this test in LayoutTests/platform/chromium/TestExpectations and
make a bug for chromium to resolve this intValue() difference and assign to
Dominic

> LayoutTests/ChangeLog:3
> +	   ER: new accessibility layout test to verify levels of headings

ER is probably not appropriate, since it's just a layout test. Also capitalize
beginning of bug

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

add some comment why this is needed

> LayoutTests/accessibility/heading-level-expected.txt:1
> +X

it'd be cool to hide all these X's once the test is done with something like

document.getElementById("test-div").style.display = none or something

> LayoutTests/accessibility/heading-level.html:14
> +<!-- explicit aria-level overrides on h1-h6 (withOUT explicit heading role
declaration)  -->

i would reference the webkit bug that causes this to be broken

> LayoutTests/accessibility/heading-level.html:47
> +	for (var i=0, c=examples.length; i<c; i++){

use a space between terms 
i=0 --> i = 0
i<c --> i  < c

space between ){

> LayoutTests/accessibility/heading-level.html:55
> +			result.innerText += "PASS: level is " +
axElement.intValue + ".\n";

it looks like tabs are being used. 
webkit prefers only spaces instead of tabs (4 spaces per indent)