[webkit-reviews] review granted: [Bug 28633] Submitting a form with target=_blank works only once : [Attachment 107519] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 15 12:58:01 PDT 2011

Andy Estes <aestes at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 28633: Submitting a form with target=_blank works only once

Attachment 107519: Patch

------- Additional Comments from Andy Estes <aestes at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107519&action=review

Nice, r=me with comments.

> LayoutTests/ChangeLog:9
> +	   New test that simulates mouse clicking submit button twice (which
didn't work), as well as using the keyboard twice (which did work)

This sentence should end with a period.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:8
> +	       This test will click the first submit button twice, then press
enter on the second submit button twice. Both should popup two blank windows.

Should read 'then press the space bar ...'

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:61
> +		   document.getElementById("nextOpKey").value = "space";
> +	       else
> +		   document.getElementById("nextOpKey").value = "notifyDone";

You set 'nextOpKey' here but you never check it in
submit-to-blank-multiple-times-form-action.html (you only check 'nextOp'). This
happens to not matter since 'nextOp' already has the value of 'space', so you
probably don't need 'nextOpKey'.

> LayoutTests/fast/forms/submit-to-blank-multiple-times.html:67
> +	   function notifyDone() {
> +	       layoutTestController.setCloseRemainingWindowsWhenComplete();
> +	       layoutTestController.notifyDone();

I see how notifyDone() is called in the passing case, but I don't in the
failing case. It looks like this test will time out if this bug is
re-introduced in the future. Instead of timing out after 35 seconds (ORWT's
default), we could time out after a more reasonable two seconds or so by adding
a setTimeout() that calls notifyDone().

> Source/WebCore/page/EventHandler.cpp:1344
> +    // FIXME (bug 28633): this call should be made at another abstraction

Bug 28633 is this bug, which will be closed after you land your patch. We
should file a new bug to track cleaning this up, and you can link to that bug
in this FIXME.

> Source/WebCore/page/EventHandler.cpp:2524
> +    // FIXME (bug 28633): this call should be made at another abstraction


More information about the webkit-reviews mailing list