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

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied 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 72236: patch
https://bugs.webkit.org/attachment.cgi?id=72236&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.