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.
Created attachment 212909 [details] patch
Created attachment 212910 [details] patch The previous patch had a header inclusion I'm not using any more.
I'm not sure about memory management on the WebMutableURLRequest I make. Should I use some kind of smart pointer?
Created attachment 212911 [details] patch There we go. Fixed the memory leak.
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?
(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.
> 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.
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.
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.
Created attachment 213110 [details] patch
Comment on attachment 213110 [details] patch r=me
(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.
Comment on attachment 213110 [details] patch Clearing flags on attachment: 213110 Committed r156729: <http://trac.webkit.org/changeset/156729>
All reviewed patches have been landed. Closing bug.