Summary: | WinLauncher should be able to open cross-domain links | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <alex.christensen> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows 7 | ||||||||||||||
Attachments: |
|
Description
Alex Christensen
2013-09-28 17:17:22 PDT
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. |