Now the Pop-up blocker loads popups but hides them. This causes problems. So we want to switch Pop-up Blocker from "load but hide" to "don't load". Please refer to http://code.google.com/p/chromium/issues/detail?id=38458 for more details
Also in chromium, we allow users to define some URL patterns to allow some sites to pop up windows.
To implement all above requests, we need to let client APIs know which URL a new window will start with. The way is to add a target URL parameter to the createView and createWindow method.
To avoid affecting other clients who reply on original syntax of createView and createWindow, I overload the createView and createWindow to provide additional functions to support the new targetURL parameter.

Attachment 60188[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/ChromeClient.h:96: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 60230[details]
patch v1 with cleaning style warning and using KURL instead of String to pass target URL
> + KURL completedUrl =
> + url.isEmpty() ? KURL(ParsedURLString, "") : completeURL(url);
It should be completedURL, not completedUrl. See the coding style document and note what you yourself did below with targetURL.
I know you just moved the code, but is KURL(ParsedURLString, "") better than KURL()? Is it different at all?
> Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, bool& created);
> + // Another version of createWindow which passes the target URL where the created window will be navigated to.
> + Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, const KURL& targetURL, bool& created);
Do we really need to have both of these? Can we change the callers to all call the new one? I don't like having an extra function here.
> Page* createWindow(Frame*, const FrameLoadRequest&, const WindowFeatures&) const;
> + Page* createWindow(Frame*, const FrameLoadRequest&, const WindowFeatures&, const KURL&) const;
Do we really need to have both of these? Can we change the callers to all call the new one? I don't like having an extra function here.
Otherwise seems fine.

Comment on attachment 60230[details]
patch v1 with cleaning style warning and using KURL instead of String to pass target URL
WebKit/chromium/public/WebViewClient.h:43
+ #include <wtf/UnusedParam.h>
you cannot have wtf includes from webkit api headers.
please just leave the parameter unnamed down below.
WebCore/page/ChromeClient.h:97
+ virtual Page* createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const KURL& targetURL)
although it is painful, Sam reminded me recently that we should
make all ChromeClient and FrameLoaderClient methods pure virtual.
that way ports will fail to compile if they do not implement the
new methods. this can help ports not get out of sync with the
methods that they need to be implementing. arguably, they don't
need to be implementing this function, but it is better for us to
have a consistent policy that is easy to enforce.

Thanks, Darin! All done!
(In reply to comment #6)
> (From update of attachment 60230[details])
> > + KURL completedUrl =
> > + url.isEmpty() ? KURL(ParsedURLString, "") : completeURL(url);
>
> It should be completedURL, not completedUrl. See the coding style document and note what you yourself did below with targetURL.
>
> I know you just moved the code, but is KURL(ParsedURLString, "") better than KURL()? Is it different at all?
>
> > Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, bool& created);
> > + // Another version of createWindow which passes the target URL where the created window will be navigated to.
> > + Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, const KURL& targetURL, bool& created);
>
> Do we really need to have both of these? Can we change the callers to all call the new one? I don't like having an extra function here.
>
> > Page* createWindow(Frame*, const FrameLoadRequest&, const WindowFeatures&) const;
> > + Page* createWindow(Frame*, const FrameLoadRequest&, const WindowFeatures&, const KURL&) const;
>
> Do we really need to have both of these? Can we change the callers to all call the new one? I don't like having an extra function here.
>
> Otherwise seems fine.

(In reply to comment #7)
> (From update of attachment 60230[details])
> WebKit/chromium/public/WebViewClient.h:43
> + #include <wtf/UnusedParam.h>
> you cannot have wtf includes from webkit api headers.
> please just leave the parameter unnamed down below.
>
Done
> WebCore/page/ChromeClient.h:97
> + virtual Page* createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const KURL& targetURL)
> although it is painful, Sam reminded me recently that we should
> make all ChromeClient and FrameLoaderClient methods pure virtual.
> that way ports will fail to compile if they do not implement the
> new methods. this can help ports not get out of sync with the
> methods that they need to be implementing. arguably, they don't
> need to be implementing this function, but it is better for us to
> have a consistent policy that is easy to enforce.
I understand the reason to make all ChromeClient and FrameLoaderClient methods pure virtual, but since The ChromeClient is important interface which is already used by all WebKit ports, it cold make the compilation failed. Should I changed all ports? (Making the callers to all call the new one createWindow also cause compilation failed)

Comment on attachment 61050[details]
patch V2, change the createWindow method on all ports.
WebCore/loader/FrameLoader.h:125
+ Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, const KURL&, bool&);
please do not remove the "created" parameter name. that was helpful
documentation for the meaning of the bool out parameter. the rule
in webkit is to leave parameters unnamed if the type name is sufficient
to document its purpose. in this case, i think you should also give
the KURL parameter a name. targetURL seems good since you are using
that elsewhere.
actually, it just occurred to me: why do we need to pass completedURL
when we have the url stored in the frameRequest? it should not be a
different url.

(In reply to comment #13)
> (From update of attachment 61050[details])
> WebCore/loader/FrameLoader.h:125
> + Frame* createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest&, const WindowFeatures&, const KURL&, bool&);
> please do not remove the "created" parameter name. that was helpful
> documentation for the meaning of the bool out parameter. the rule
> in webkit is to leave parameters unnamed if the type name is sufficient
> to document its purpose. in this case, i think you should also give
> the KURL parameter a name. targetURL seems good since you are using
> that elsewhere.
>
> actually, it just occurred to me: why do we need to pass completedURL
> when we have the url stored in the frameRequest? it should not be a
> different url.
Yes, ideally the targetURL should be as same as the URL in the frameRequest, but according to the comments in JSDOMWindowCustom.cpp, now the code only passes the empty string for the URL. The reason is, before loading the URL, we have to set up the opener, openedByDOM, and dialogArguments values. Also, to decide whether to use the URL we currently do an allowsAccessFrom call using the window we create, which can't be done before creating it. We'd have to resolve all those issues to pass the URL instead of "".

(In reply to comment #14)
> Yes, ideally the targetURL should be as same as the URL in the frameRequest,
> but according to the comments in JSDOMWindowCustom.cpp, now the code only
> passes the empty string for the URL. The reason is, before loading the URL, we
> have to set up the opener, openedByDOM, and dialogArguments values. Also, to
> decide whether to use the URL we currently do an allowsAccessFrom call using
> the window we create, which can't be done before creating it. We'd have to
> resolve all those issues to pass the URL instead of "".
OK, I see that explained in a FIXME comment in the code.
This seems pretty unfortunate. I think the objective of that comment is to
prevent an embedder from loading the given FrameLoadRequest inside the
createWindow call. However, by plumbing the URL through the createWindow
call as your patch is doing, you are effectively making it possible for
the embedder to load the URL straight away.
It seems like we should fix all of the embedders to not load the request
inside of createWindow, and then we should be able to safely pass the
target URL as the URL of the FrameLoadRequest.
Plumbing another URL parameter alongside the FrameLoadRequest doesn't seem
like the right solution to me.

>
> OK, I see that explained in a FIXME comment in the code.
>
> This seems pretty unfortunate. I think the objective of that comment is to
> prevent an embedder from loading the given FrameLoadRequest inside the
> createWindow call. However, by plumbing the URL through the createWindow
> call as your patch is doing, you are effectively making it possible for
> the embedder to load the URL straight away.
>
> It seems like we should fix all of the embedders to not load the request
> inside of createWindow, and then we should be able to safely pass the
> target URL as the URL of the FrameLoadRequest.
>
> Plumbing another URL parameter alongside the FrameLoadRequest doesn't seem
> like the right solution to me.
I see, you are right, this additional targetURL parameter looks redundant.
I will investigate how many code will be affected if we pass target URL as the URL of the FrameLoadRequest and come up with a new patch.
Thanks!

According to the previous discussion with Darin, we should find a solution, which can fix all of the embedders to not load the request inside of createWindow. So we should be able to safely pass the target URL as the URL of the FrameLoadRequest.
According to my analysis, currently the createWindow are used in two scenarios:
1. called by window.open in JSBinding, this is the most common usage. In this scenario, before loading the URL, we have to set up the opener, openedByDOM, and dialogArguments values. Also, to decide whether to use the URL, we currently do an allowsAccessFrom call using the window we create, which can't be done before creating it, that is why WebKit passes empty URL to FrameRequest to avoid the URL navigation.
2. called by some places in where we just need to open a window to reload some resources, like a image or a webpage in a new window. It's triggered by user gesture. Currently it only was only used in ContextMenuController to respond users' actions in context menu.
In most time, currently the createWindow only do window-creating without URL navigator, just like its name. Even sometime it does the URL navigation, it can be broken into two parts: create window and navigate the specified URL.
However recently a new parameter NavigationAction was added to parameter list of createWindow (http://trac.webkit.org/changeset/70823), this parameter also can be used to indicate whether the createWindow will do URL navigation or not.
I personally think we have two options to fix this issue.
1. Explicitly Claim that the createWindow should only do window-creating without URL navigator. we remove all the URL navigation logic in all createWindow and related implementations. We pass the URL to FrameRequest parameter because some ChromeClientIMPL derived classes need the info (like do popup blocking). If caller wants to do navigation after creating window, just call newFrame->navigationScheduler()->scheduleLocationChange.
2. Add a indicator in the NavigationAction parameter to indicate whether the createWindow allow a URL navigation, all createWindow and related implementations should check this indicator before doing URL navigation.
I prefer the option 1 because it makes the createWindow's logic more clear and we can eventually move it from the huge class: FrameLoader.
My co-worker, Xianzhu Wang(phnixwxz@gmail.com) has graciously volunteered to provide the patch for this issue since he is stepping in WebKit hack in recent months, so I step back to work onthe related chromium bug (http://crbug.com/38458).
Thanks.

Created attachment 73983[details]
Patch V3
This patch changes all createWindow implementations to remove URL navigations in them.
Now full request containing the URL is passed to createWindow for it only to check if the request can be fulfilled.
WebCore::createWindow is still in FrameLoader.cpp to keep this change small. I prefer doing it later in a separate change.

Hi Adam, Darin (from Apple) and Darin (from Google),
Would you please take a look this patch and share your comments when you have time. As long as the issue gets fixed, the embedder can know the popup URL when creating window and decide to whether grant the access or not as early as possible.
Thanks!

Comment on attachment 74080[details]
Patch V3 update 1 fixed Chromium break
View in context: https://bugs.webkit.org/attachment.cgi?id=74080&action=review>> WebCore/bindings/generic/BindingDOMWindow.h:89
>> + KURL completedUrl = url.isEmpty() ? KURL() : completeURL(state, url);
>
> nit: completedUrl -> completedURL
>
> acronyms should either be lowercase or UPPERCASE depending on whether they appear at the beginning or elsewhere in a name, respectively.
Done. (now in Source/WebCore/page/DOMWindow.cpp
>> WebKit/chromium/public/WebViewClient.h:85
>> virtual WebView* createView(WebFrame* creator,
>
> nit: please add a FIXME comment saying that this method is DEPRECATED.
Done.
>> WebKit/chromium/public/WebViewClient.h:88
>> + // Sometimes it's much better for client API if a new window starts with a
>
> nit: insert a new line above this comment.
Done.
>> WebKit/chromium/public/WebViewClient.h:92
>> + virtual WebView* createView(WebFrame* creator,
>
> nit: (bikeshed comment) how about re-ordering these parameters so that it is creator, request, name, and then features? that way it more closely resembles window.open(url, name, features).
Done.
>> WebKit/chromium/public/WebViewClient.h:96
>> + (void)request;
>
> nit: please just leave the WebURLRequest parameter unnamed so that you don't need
> to write this line.
The request parameter is referenced in the comment of the method, so we should keep it. I added a FIXME here. Eventually this line will be removed.

(In reply to comment #31)
> Created an attachment (id=78309) [details]
> patch v3 update 2
Darin already reviewed your patch, you don't need to ask for review again, you can change the reviewer in ChangeLog to "Darin Fisher" in your patch and I will help you land it.