[webkit-reviews] review canceled: [Bug 48364] Windows: Update resource tracking when moving a frame between documents : [Attachment 72382] updated patch

[bugzilla-daemon at webkit.org]

Jenn Braithwaite <jennb at chromium.org> has canceled Jenn Braithwaite
<jennb at chromium.org>'s request for review:
Bug 48364: Windows: Update resource tracking when moving a frame between
documents
https://bugs.webkit.org/show_bug.cgi?id=48364

Attachment 72382: updated patch
https://bugs.webkit.org/attachment.cgi?id=72382&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.