[webkit-reviews] review denied: [Bug 68412] REGRESSION (WK2): (Shift-)option-tabbing skips over elements when transitioning from chrome to webview : [Attachment 108414] Patch

Thu Sep 22 20:25:02 PDT 2011

Daniel Bates <dbates at webkit.org> has denied Jon Lee <jonlee at apple.com>'s
request for review:
Bug 68412: REGRESSION (WK2): (Shift-)option-tabbing skips over elements when
transitioning from chrome to webview
https://bugs.webkit.org/show_bug.cgi?id=68412

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=108414&action=review

I did not review this patch for correctness. I suggest someone more
knowledgable about this area of the code look through this patch. I have some
minor nits.

I am r-'ing this patch since you need to update the WebPage::setInitialFocus()
call site in Source/WebKit2/UIProcess/API/C/win/WKView.cpp as pointed out in
the output of the Windows EWS bot:

7>..\UIProcess\API\C\win\WKView.cpp(72) : error C2660:
'WebKit::WebView::setInitialFocus' : function does not take 1 arguments

> LayoutTests/fast/forms/focus-option-control-on-page-expected.txt:8
> +<https://bugs.webkit.org/show_bug.cgi?id=68412> This test checks to see if
option(alt)-tabbing properly focuses form elements that are normally not
focused. For testing, the assumption is that by default pressing tab will skip
over buttons, and option-tab will include buttons.
> +
> +  
> +Tab:
> +0:2
> +1:
> +2:2
> +3:

I'm unclear how to interpret these results to determine whether this test
passed or failed. The test description doesn't seem to elaborate on how to tell
whether this test passed or failed.

I suggest either writing some more english to explain how to interpret these
results or, even better, write make the output more straight forward to
understand by using a "PASS" and "FAIL" style output. One example of
"PASS"/"FAIL"-style output can be seen in the test
LayoutTests/fast/forms/menulist-submit-without-selection.html:
<http://trac.webkit.org/export/95777/trunk/LayoutTests/fast/forms/menulist-subm
it-without-selection.html>.

> LayoutTests/fast/forms/focus-option-control-on-page.html:1
> +<p><https://bugs.webkit.org/show_bug.cgi?id=68412> This test checks to
see if option(alt)-tabbing properly focuses form elements that are normally not
focused. For testing, the assumption is that by default pressing tab will skip
over buttons, and option-tab will include buttons.</p>

Nit: For new layout tests we tend to include a DOCTYPE, <html> and <body> tags.

> LayoutTests/fast/forms/focus-option-control-on-page.html:7
> +<p id="log"></p>

Can we write this test using the js-test constructs provided by
LayoutTests/fast/js/resources/js-test-{pre, post}.js? This will make this test
more consistent with other DRT tests in this directory. These scripts also
provide machinery to make PASS/FAIL-style tests. See my comment on
focus-option-control-on-page-expected.txt (above) for an example test case that
uses this machinery.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1319
> +    

Did you mean to add this tab-indented empty line? Having one empty line seems
sufficient to visually separate the early returns from the actual body of this
function. I suggest removing this line.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:1208
> +    NSTextField* textField = [[NSTextField alloc]
initWithFrame:NSMakeRect(0, 0, 100, 20)];

Nit: As far as I know, we put the '*' on the right in Objective-C/C++ code.