[webkit-reviews] review granted: [Bug 3308] Pop-up blocking blocks window.open for already open windows : [Attachment 6976] patch with detailed change log and a layout test

[bugzilla-request-daemon at opendarwin.org]

Geoffrey Garen <ggaren at apple.com> has granted Geoffrey Garen
<ggaren at apple.com>'s request for review:
Bug 3308: Pop-up blocking blocks window.open for already open windows
http://bugzilla.opendarwin.org/show_bug.cgi?id=3308

Attachment 6976: patch with detailed change log and a layout test
http://bugzilla.opendarwin.org/attachment.cgi?id=6976&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
Looks great. A few comments.

Is there a reason for the layout test not to dumpAsText()?

+    Frame* childFrame = parentFrame->tree()->child(m_name);
+    if (childFrame)
+	
childFrame->openURL(getDocument()->completeURL(relativeURL.qstring()));

I would move the assignment here inside the if statement, to match other code
in your patch.

I would call "nameForNewChild" "uniqueNameForNewChild" or just
"uniqueNameForChild". "New" is misleading since you call this function on old
frames inside FrameTree::setName. Maybe you left out "unique" in order to hide
within the Frame class the details of how new children are named, but doing so
made it difficult for me to understand why you would have to call a method like
this at all.

+    char suffix[40]; // 16 + maximum width of an unsigned integer
+    sprintf(suffix, "/<!--frame%u-->-->", childCount());

This looks like it works, but it's shady. I would *strongly* prefer it if we
handled sprintf with thick rubber gloves. Also, "maximum" on what system? 32
bit? 64? 128? (By my calculations, it's 20 characters on a 64 bit system:
18,446,744,074,000,000,000 sans commas.) And what about the null terminator? I
suggest something like:

ASSERT(sizeof(unsigned) <= 64);
char suffix[sizeof("/<!--frame-->-->") + 20]; // 20 is the number of characters
required to print the largest unsigned integer on a 64 bit system
snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount());

DumpRenderTree needs a ChangeLog.

r=me, but given the sprintf business, I'm kinda rounding up :).