[webkit-reviews] review denied: [Bug 11346] _blank is assigned to Window name : [Attachment 11202] Patch for the issue

[bugzilla-request-daemon at macosforge.org]

Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 11346: _blank is assigned to Window name
http://bugs.webkit.org/show_bug.cgi?id=11346

Attachment 11202: Patch for the issue
http://bugs.webkit.org/attachment.cgi?id=11202&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
My comments:

1) This patch appears to be in RTF format, it needs to be in plain text.

2) There is no ChangeLog entry.

3) There is no test case in the patch - this probably can't be tested
automatically but it should include a test for the manual-tests directory.

4) This fix is at the wrong layer. There are ther constructs besides
window.open that can open a new window, such as <a href="http://somesite.com/"
target="_blank">, these need to be covered as well. The right place to fix this
is in the Frame code itself - a request to open a new window for _blank should
make sure that the name ends up empt instead of "_blank".

5) Comments in the code should not talk about making a specific fix, nor should
they describe what the code is doing if that is obvious just from reading.

6) Coding style issue, there should be a space between the "if" and the open
paren: if(frameName == "_blank")

Please addresse these issues and resubmit.