Created attachment 60188 [details] patch V1

Comment on attachment 60188 [details] patch V1 WebCore/bindings/generic/BindingDOMWindow.h:99 + Frame* newFrame = callingFrame->loader()->createWindow(openerFrame->loader(), frameRequest, windowFeatures, completeUrl.prettyURL(), created); What is a prettyURL? WebCore/loader/FrameLoader.cpp:261 + Frame* FrameLoader::createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL, bool& created) Why is this a String and not a KURL? WebCore/loader/FrameLoader.cpp:328 + return createWindow(frameLoaderForFrameLookup, request, features, "", created); Why isn't the URL plumbed here? WebCore/page/Chrome.cpp:166 + Page* Chrome::createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL) const Isn't there a URL in FrameLoadRequest? Is that different? WebKit/chromium/public/WebViewClient.h:86 + // The default implementation is to call the old version of createView. Are we going to remove this after we do the roll? WebKit/chromium/src/ChromeClientImpl.cpp:236 + m_webView->client()->createView(WebFrameImpl::fromFrame(frame), features, r.frameName(), KURL(ParsedURLString, targetURL))); Sad that we have to parse the URL again here. WebKit/chromium/src/ChromeClientImpl.cpp:251 + Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features) One line please. |r| is not a very descriptive variable name. WebKit/chromium/src/ChromeClientImpl.cpp:253 + return createWindow(frame, r, features, ""); More empty string sadness.

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.

Adam, Thanks for review. For the comments of using String not URL, that is my fault, I thought using String is easy way since we have different URL class implementation like KURL, WebURL, GURL, etc... I will change them to corresponding URL class. (In reply to comment #2) > (From update of attachment 60188 [details]) > WebCore/bindings/generic/BindingDOMWindow.h:99 > + Frame* newFrame = callingFrame->loader()->createWindow(openerFrame->loader(), frameRequest, windowFeatures, completeUrl.prettyURL(), created); > What is a prettyURL? > > WebCore/loader/FrameLoader.cpp:261 > + Frame* FrameLoader::createWindow(FrameLoader* frameLoaderForFrameLookup, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL, bool& created) > Why is this a String and not a KURL? > > WebCore/loader/FrameLoader.cpp:328 > + return createWindow(frameLoaderForFrameLookup, request, features, "", created); > Why isn't the URL plumbed here? > > WebCore/page/Chrome.cpp:166 > + Page* Chrome::createWindow(Frame* frame, const FrameLoadRequest& request, const WindowFeatures& features, const String& targetURL) const > Isn't there a URL in FrameLoadRequest? Is that different? > > WebKit/chromium/public/WebViewClient.h:86 > + // The default implementation is to call the old version of createView. > Are we going to remove this after we do the roll? Yes, definitely. After finishing the code on chromium side, we will remove the old version. > > WebKit/chromium/src/ChromeClientImpl.cpp:236 > + m_webView->client()->createView(WebFrameImpl::fromFrame(frame), features, r.frameName(), KURL(ParsedURLString, targetURL))); > Sad that we have to parse the URL again here. > > WebKit/chromium/src/ChromeClientImpl.cpp:251 > + Frame* frame, const FrameLoadRequest& r, const WindowFeatures& features) > One line please. |r| is not a very descriptive variable name. > > WebKit/chromium/src/ChromeClientImpl.cpp:253 > + return createWindow(frame, r, features, ""); > More empty string sadness. (

Created attachment 60230 [details] patch v1 with cleaning style warning and using KURL instead of String to pass target URL

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)

I have to leave work until July 8th. Sorry for the delay and inconvenience. I will continue working on this from July 8th.

Created attachment 61049 [details] patch V2 ()

Created attachment 61050 [details] patch V2, change the createWindow method on all ports.

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 "".

Created attachment 61057 [details] patch V2 with adding parameter name for FrameLoader::createWindow

Attachment 61050 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3474085

Created attachment 61059 [details] patch V2

Hi Adam, Darin(@Apple), Darin(@Google) Sorry for bothering. Would you like review my patch V2 again? I am pending on this CL to continue my next-step work:-) Thanks!

(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!

Comment on attachment 61059 [details] patch V2 Removing the review flag as per comment 20.

*** Bug 48991 has been marked as a duplicate of this bug. ***

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.

Attachment 73983 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5945078

Created attachment 74080 [details] Patch V3 update 1 fixed Chromium break

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 Just nits, but otherwise this LGTM. > 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. > WebKit/chromium/public/WebViewClient.h:85 > virtual WebView* createView(WebFrame* creator, nit: please add a FIXME comment saying that this method is DEPRECATED. > 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. > 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). > 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.

Thanks Darin. @Xianzhu, please fix the patch according to Darin's comments. I will commit it once you fix.

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.

Created attachment 78309 [details] patch v3 update 2

(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.

Created attachment 78346 [details] patch v3 update 2 with corrected reviewer line

Created attachment 78347 [details] patch v3 update 2 with corrected reviewer lines

The commit-queue encountered the following flaky tests while processing attachment 78347 [details]: http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 78347 [details] patch v3 update 2 with corrected reviewer lines Clearing flags on attachment: 78347 Committed r75349: <http://trac.webkit.org/changeset/75349>

All reviewed patches have been landed. Closing bug.

This broke all Chromium windows compilers (http://build.webkit.org/builders/Chromium%20Win%20Release, http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29, http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win, http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20%28dbg%29). Can you resolve/revert please?

Hi, James, We are handling the issue. Hopefully we'll provide the patch in half an hour. Thanks

Committed r75361: <http://trac.webkit.org/changeset/75361>

(In reply to comment #40) > Committed r75361: <http://trac.webkit.org/changeset/75361> Hi James, the fix has been landed in r75361, It should fix the Chromium windows compilation error. Please take a look.