[webkit-reviews] review denied: [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]

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 72385: updated patch (take 2) - delete extra space
https://bugs.webkit.org/attachment.cgi?id=72385&action=review

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