[webkit-reviews] review denied: [Bug 55110] fast/forms/input-file-not-open-without-gesture.html passes when a file picker dialog is opened without a gesture. : [Attachment 94948] patch v5

Thu May 26 05:59:55 PDT 2011

Adam Roben (:aroben) <aroben at apple.com> has denied Johnny(Jianning) Ding
<jnd at chromium.org>'s request for review:
Bug 55110: fast/forms/input-file-not-open-without-gesture.html passes when a
file picker dialog is opened without a gesture.
https://bugs.webkit.org/show_bug.cgi?id=55110

Attachment 94948: patch v5
https://bugs.webkit.org/attachment.cgi?id=94948&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94948&action=review

This patch as currently written will cause this test to fail in WebKit2. You'll
need to implement the new "OPEN PANEL" dumping in WebKitTestRunner.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:607
> +	   fprintf(stdout, "OPEN PANEL\n");

Something more descriptive might be nice. Even just "RUN OPEN PANEL" would be
better.

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.cpp:34
> +bool DumpRenderTreeSupportWx::s_drtRun = false;

This can just be a file-level static. It doesn't need to be a member variable.E

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.h:36
> +class DumpRenderTreeSupportWx {
> +
> +public:

Extra newline here.

> Source/WebKit/wx/WebKitSupport/DumpRenderTreeSupportWx.h:38
> +    DumpRenderTreeSupportWx();
> +    ~DumpRenderTreeSupportWx();

No need to declare/define these. The compiler will do it for you.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:20
> +function openFileDialog(withUserGesture) {
> +    window.console.log("Open the file dialog " + (withUserGesture ? "with" :
"without") + " a user gesture.");
> +    if (withUserGesture) {
> +	   eventSender.mouseMoveTo(10, 10);
>	   eventSender.mouseDown();
>	   eventSender.mouseUp();
> -	   clickFired = true;
> -	   testStage = OPEN_FILE_WITHOUT_GESTURE;
> -	   // Waits for opening the file dialog and checks whether the click
event can still be received by document.
> -	   setTimeout("checkResult(function() {
layoutTestController.notifyDone(); })", 500);
> -    }, 1);
> +    } else {
> +	   document.getElementById('f').click();
> +    }
>  }

Having two separate functions might be clearer.

> LayoutTests/fast/forms/input-file-not-open-without-gesture.html:31
> +<body style="margin:0px; padding:0px" onload="setTimeout('startTest();',
0)">

Why is this delay needed?

> LayoutTests/platform/win/Skipped:1245
> +# Since WebKit displays the file chooser directly on Apple's Windows port
instead of using UIDelegate, we need to skip the following test on Apple's
Windows port.
> +# See  https://bugs.webkit.org/show_bug.cgi?id=56383 for more details.
> +fast/forms/input-file-not-open-without-gesture.html

It would be a little better to check in expected failure results for Windows
rather than skipping the test. That way we'll notice if the test's behavior
changes in any other way unexpectedly.