[webkit-reviews] review denied: [Bug 11388] For iframe if the name is not specified, id is assigned to name. : [Attachment 11207] Patch for review for the iframe name issue

bugzilla-request-daemon at macosforge.org bugzilla-request-daemon at macosforge.org
Tue Oct 31 04:08:16 PST 2006


Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 11388: For iframe if the name is not specified, id is assigned to name.
http://bugs.webkit.org/show_bug.cgi?id=11388

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Thanks for the patch! It needs a couple of problems fixed to be landable.

1) This patch appears to be formatted as an RTF file or something (lots of
weird backslashes). We need a plaintext patch to be able to land it.

2) The patch should not leave in commented code - if some lines of code are
wrong, you should just remove them. There's also no need to put comments
explaining why certain things aren't done.

3) The patch should include test cases in the form of tests and expected
results for the LayoutTests directory, since it changes behavior for web
content.  The samples should be part of the patch, not just a separate zip
file.

4) I am pretty sure the unique frame names are for a reason, specifically for
the back/forward list to be able to find things properly when you do
back/forward in nested framesets. Did you test this case?

5) The patch needs to include a ChangeLog entry.

Thanks for the submission, please address these comments and submit a new
patch. r- for now since this needs revision.



More information about the webkit-reviews mailing list