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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=28633

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

------- 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
layer

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
layer

Ditto.