Summary: | Pop-up blocking blocks window.open for already open windows | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Kaldari <kaldari> | ||||||
Component: | JavaScriptCore | Assignee: | Darin Adler <darin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | alice.barraclough, darin | ||||||
Priority: | P1 | Keywords: | InRadar | ||||||
Version: | 312.x | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.3 | ||||||||
URL: | http://www.angelblade.com/bugs/popup/index.html | ||||||||
Attachments: |
|
Description
Ryan Kaldari
2005-06-07 11:37:50 PDT
Increasing priority since this bug is breaking some dutch banking sites. <rdar://problem/3853181> is assigned to vicki with P2 Here's the line of code that needs to change. In the window.open implementation, there's this: if (!allowPopUp(exec, window) && !(frame->findFrame(frameName))) return jsUndefined(); It says "if there's not already a frame by this name, and we won't allow a pop-up, then do nothing". The bug is that findFrame only looks for frames inside the current frame tree, not in other windows. We just need to fix the code to use a frame-searching function that implements the whole frame naming algorithm, like -[WebCoreFrameBridge findFrameNamed:]. I have the Radar for this, so I'm going to grab the Bugzilla too! Created attachment 6898 [details]
no test case yet, but would like a first pass of review anyway
Created attachment 6976 [details]
patch with detailed change log and a layout test
Comment on attachment 6976 [details]
patch with detailed change log and a layout test
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 :).
(In reply to comment #7) > Is there a reason for the layout test not to dumpAsText()? I tried it, and it just dumped as the empty string. I don't think frame contents are included in 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. OK. > 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. I'd like to find a better name. I think uniqueNameForChild is probably good. > > + 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()); OK. I'll do something more like what you suggest. > DumpRenderTree needs a ChangeLog. Will do. |