Bug 122069 – WinLauncher should be able to open cross-domain links

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alex Christensen]

When clicking on a cross-domain link that would open another tab, WinLauncher does nothing because createWebViewWithRequest hasn't been implemented.  Also, right clicking on a link and clicking "Open Link in new Window" does nothing for the same reason.  WebKit isn't even giving createWebViewWithRequest the URL.  Surely there's somebody that has opened a cross-domain link with WebKit on Windows.

[Alex Christensen]

Created attachment 212909 [details]
patch

[Alex Christensen]

Created attachment 212910 [details]
patch

The previous patch had a header inclusion I'm not using any more.

[Alex Christensen]

I'm not sure about memory management on the WebMutableURLRequest I make.  Should I use some kind of smart pointer?

[Alex Christensen]

Created attachment 212911 [details]
patch

There we go.  Fixed the memory leak.

[Brent Fulgham]

Comment on attachment 212911 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212911&action=review

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:204
> +    request.adoptRef(WebMutableURLRequest::createInstance(ResourceRequest(navigationAction.url())));

Can this not be written in our usual one-line style?

> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:542
> +    request.adoptRef(WebMutableURLRequest::createInstance(ResourceRequest(navigationAction.url())));

Ditto:

> Tools/WinLauncher/PrintWebUIDelegate.cpp:47
> +    DWORD length = GetModuleFileName(GetModuleHandle(0), buffer, ARRAYSIZE(buffer));

Please prefix WinAPI calls with '::'

> Tools/WinLauncher/PrintWebUIDelegate.cpp:51
> +    BSTR url;

If you use a _bstr_t it will clean up after itself.

> Tools/WinLauncher/PrintWebUIDelegate.cpp:63
> +        return E_FAIL;

This looks okay, but why do we spawn a new process to view a web page? We don't spawn a new WinLauncher to show the Web Inspector, can't we work the same way here?

[Alex Christensen]

(In reply to comment #5)
> (From update of attachment 212911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212911&action=review
> 
> > Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:204
> > +    request.adoptRef(WebMutableURLRequest::createInstance(ResourceRequest(navigationAction.url())));
> 
> Can this not be written in our usual one-line style?
COMPtr.h doesn't seem to have an adoptRef function.  I could write one if it doesn't already exist somewhere else.

> This looks okay, but why do we spawn a new process to view a web page? We don't spawn a new WinLauncher to show the Web Inspector, can't we work the same way here?
I tried a single-process implementation based on the one in DumpRenderTree, but it was much more complicated, and I had problems where closing child windows killed the whole process.  Maybe this can be worked around, but this is the simplest solution.

[Alex Christensen]

> We don't spawn a new WinLauncher to show the Web Inspector, can't we work the same way here?
This would also involve refactoring a lot of code to remove almost all global variables in WinLauncher, create an instance manager, etc.  I don't think this would be worth it right now.

[Alex Christensen]

Created attachment 213109 [details]
patch

After this patch gets in, we can remove all uses of adoptRef from COMPtr.h in favor of my adoptCOM function.  It would be less confusing to not have a function named adoptRef operating on two different types of smart pointers.

[WebKit Commit Bot]

Attachment 213109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/win/COMPtr.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp', u'Tools/ChangeLog', u'Tools/WinLauncher/PrintWebUIDelegate.cpp', u'Tools/WinLauncher/PrintWebUIDelegate.h']" exit_code: 1
Tools/WinLauncher/PrintWebUIDelegate.cpp:52:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 213110 [details]
patch

[Brent Fulgham]

Comment on attachment 213110 [details]
patch

r=me

[Brent Fulgham]

(In reply to comment #8)
> Created an attachment (id=213109) [details]
> patch
> 
> After this patch gets in, we can remove all uses of adoptRef from COMPtr.h in favor of my adoptCOM function.  It would be less confusing to not have a function named adoptRef operating on two different types of smart pointers.

Yes -- let's do this as a separate patch, and CC andersca as well please.

[WebKit Commit Bot]

Comment on attachment 213110 [details]
patch

Clearing flags on attachment: 213110

Committed r156729: <http://trac.webkit.org/changeset/156729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.