Summary: | Windows: Update resource tracking when moving a frame between documents | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jenn Braithwaite <jennb> | ||||||||||||
Component: | Page Loading | Assignee: | Jenn Braithwaite <jennb> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aroben, commit-queue, dimich, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows 7 | ||||||||||||||
Bug Depends on: | 44713 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Jenn Braithwaite
2010-10-26 12:34:24 PDT
Created attachment 72236 [details]
patch
Comment on attachment 72236 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=72236&action=review > WebKit/win/Interfaces/IWebResourceLoadDelegate.idl:193 > interface IWebResourceLoadDelegate : IUnknown > - (void)webView:(WebView *)sender plugInFailedWithError:(NSError *)error dataSource:(WebDataSource *)dataSource; > */ > HRESULT plugInFailedWithError([in] IWebView* webView, [in] IWebError* error, [in] IWebDataSource* dataSource); > + > + /*! > + @method webView:removeIdentifierForRequest > + @param webView The WebView sending the message. > + @param identifier An identifier that can be used to track the progress of a resource load across > + multiple call backs. > + @discussion This message is sent to notify the delegate to stop using the identifier > + to track the progress of a resource load. > + - (void)webView:(WebView *)sender removeIdentifierForRequest:(id)identifier; > + */ > + HRESULT removeIdentifierForRequest([in] IWebView* webView, [in] unsigned long identifier); > } Adding this to IWebResourceLoadDelegate will break nightly builds (because Safari implements IWebResourceLoadDelegate but not your new function, so you'll crash when you try to call it). You need to add a new interface, IWebResourceLoadDelegatePrivate2, and add the new function to that. > WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp:1203 > - if (FAILED(webView->setResourceLoadDelegate(sharedResourceLoadDelegate.get()))) > + if (FAILED(webView->setResourceLoadDelegate(new ResourceLoadDelegate))) This means you're now leaking every ResourceLoadDelegate. WebView will call AddRef/Release on them, but you need to Release the initial reference that the ResourceLoadDelegate constructor gives you. > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:171 > + ASSERT(urlMap().find(identifier) != urlMap().end()); You can write this instead: ASSERT(urlMap().contains(identifier)); I think that's clearer. > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:188 > - descriptionSuitableForTestResult(identifier).c_str(), > + makeDescriptionSuitableForTestResult(identifier).c_str(), I don't think the change in the name of the function is needed. > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:245 > + printf("%S - didReceiveAuthenticationChallenge - Responding with %s:%s\n", > + makeDescriptionSuitableForTestResult(identifier).c_str(), > + user, password); Please put this all on one line. > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:322 > +wstring ResourceLoadDelegate::makeDescriptionSuitableForTestResult( > + unsigned long identifier) Please put this all on one line. This can be a const member function. > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:333 > +wstring ResourceLoadDelegate::makeDescriptionSuitableForTestResult( > + IWebError* error, unsigned long identifier) Please put this all on one line. This can be a const member function. Created attachment 72382 [details]
updated patch
Attachment 72382 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp:1205: One space before end of line comments [whitespace/comments] [5]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 72385 [details] updated patch (take 2) - delete extra space (In reply to comment #2) > (From update of attachment 72236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72236&action=review > > > WebKit/win/Interfaces/IWebResourceLoadDelegate.idl:193 > > interface IWebResourceLoadDelegate : IUnknown > > + HRESULT removeIdentifierForRequest([in] IWebView* webView, [in] unsigned long identifier); > > } > > Adding this to IWebResourceLoadDelegate will break nightly builds (because Safari implements IWebResourceLoadDelegate but not your new function, so you'll crash when you try to call it). You need to add a new interface, IWebResourceLoadDelegatePrivate2, and add the new function to that. Moved to IWebResourceLoadDelegatePrivate2. > > > WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp:1203 > > - if (FAILED(webView->setResourceLoadDelegate(sharedResourceLoadDelegate.get()))) > > + if (FAILED(webView->setResourceLoadDelegate(new ResourceLoadDelegate))) > > This means you're now leaking every ResourceLoadDelegate. WebView will call AddRef/Release on them, but you need to Release the initial reference that the ResourceLoadDelegate constructor gives you. > Changed, but not sure if I did it correctly. > > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:171 > > + ASSERT(urlMap().find(identifier) != urlMap().end()); > > You can write this instead: > > ASSERT(urlMap().contains(identifier)); > > I think that's clearer. Agreed. Changed. > > > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:188 > > - descriptionSuitableForTestResult(identifier).c_str(), > > + makeDescriptionSuitableForTestResult(identifier).c_str(), > > I don't think the change in the name of the function is needed. > Build error results when a member function and static non-member function have the same name due to confusion about finding the correct overloaded function. Instead of changing the name, I made all the descriptionSuitableForTestResult functions member functions, 2 of which are static and 2 of which are const. Makes for fewer edits in the file. > > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:245 > > + printf("%S - didReceiveAuthenticationChallenge - Responding with %s:%s\n", > > + makeDescriptionSuitableForTestResult(identifier).c_str(), > > + user, password); > > Please put this all on one line. > > > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:322 > > +wstring ResourceLoadDelegate::makeDescriptionSuitableForTestResult( > > + unsigned long identifier) > > Please put this all on one line. > > This can be a const member function. > > > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:333 > > +wstring ResourceLoadDelegate::makeDescriptionSuitableForTestResult( > > + IWebError* error, unsigned long identifier) > > Please put this all on one line. > > This can be a const member function. Both done. Comment on attachment 72385 [details] updated patch (take 2) - delete extra space View in context: https://bugs.webkit.org/attachment.cgi?id=72385&action=review Unfortunately it looks like you forgot to svn add the new file! Otherwise this looks great. > WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:754 > + COMPtr<IWebResourceLoadDelegatePrivate2> oldResourceLoadDelegatePrivate2; > + if (FAILED(oldResourceLoadDelegate->QueryInterface(IID_IWebResourceLoadDelegatePrivate2, reinterpret_cast<void**>(&oldResourceLoadDelegatePrivate2)))) > + return; There's a special COMPtr constructor that does the QueryInterface for you: COMPtr<IWebResourceLoadDelegatePrivate2> oldResourceLoadDelegatePrivate2(Query, oldResourceLoadDelegate); if (!oldResourceLoadDelegatePrivate2) return; > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.cpp:75 > -static wstring descriptionSuitableForTestResult(unsigned long identifier) > +wstring ResourceLoadDelegate::descriptionSuitableForTestResult(unsigned long identifier) const > { > - IdentifierMap::iterator it = urlMap().find(identifier); > + IdentifierMap::const_iterator it = m_urlMap.find(identifier); > > - if (it == urlMap().end()) > + if (it == m_urlMap.end()) > return L"<unknown>"; > > return urlSuitableForTestResult(it->second); > } > > -static wstring descriptionSuitableForTestResult(IWebURLRequest* request) > +wstring ResourceLoadDelegate::descriptionSuitableForTestResult(IWebURLRequest* request) > { > if (!request) > return L"(null)"; Thanks for making this portion of the patch so much more readable! > WebKitTools/DumpRenderTree/win/ResourceLoadDelegate.h:107 > protected: > ULONG m_refCount; This can probably be made private, too, and moved to the end of the class declaration. Created attachment 72392 [details]
updated patch
New idl file included this time. D'oh. My first idl file ever - look closely.
Thanks for the ctor tip. Much prettier.
Attachment 72392 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/win/Interfaces/IWebResourceLoadDelegatePrivate2.idl:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 51 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 72392 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=72392&action=review Please fix the line-endings identified by the style bot. Other than that, looks great! > WebKit/win/Interfaces/IWebResourceLoadDelegatePrivate2.idl:25 > + */ > +#ifndef DO_NO_IMPORTS Please add a blank line after the license comment. Created attachment 72533 [details]
updated patch - fix CRLF, add blank line
The commit-queue encountered the following flaky tests while processing attachment 72533 [details]: compositing/iframes/iframe-size-from-zero.html Please file bugs against the tests. These tests were authored by ajwong@chromium.org. The commit-queue is continuing to process your patch. Comment on attachment 72533 [details] updated patch - fix CRLF, add blank line Clearing flags on attachment: 72533 Committed r71097: <http://trac.webkit.org/changeset/71097> All reviewed patches have been landed. Closing bug. |