[webkit-reviews] review requested: [Bug 48364] Windows: Update resource tracking when moving a frame between documents : [Attachment 72385] updated patch (take 2) - delete extra space
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 29 13:47:31 PDT 2010
Jenn Braithwaite <jennb at chromium.org> has asked for review:
Bug 48364: Windows: Update resource tracking when moving a frame between
documents
https://bugs.webkit.org/show_bug.cgi?id=48364
Attachment 72385: updated patch (take 2) - delete extra space
https://bugs.webkit.org/attachment.cgi?id=72385&action=review
------- Additional Comments from Jenn Braithwaite <jennb at chromium.org>
(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.
More information about the webkit-reviews
mailing list