Bug 48364 – Windows: Update resource tracking when moving a frame between documents

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Jenn Braithwaite]

Windows fix for bug reported in 44713.  That patch provides the framework for the fix and fixes the Mac platform.  Separate patches will be submitted for other platforms, thus this separate bug.

[Jenn Braithwaite]

Created attachment 72236 [details]
patch

[Adam Roben (:aroben)]

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.

[Jenn Braithwaite]

Created attachment 72382 [details]
updated patch

[WebKit Review Bot]

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.

[Jenn Braithwaite]

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.

[Adam Roben (:aroben)]

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.

[Jenn Braithwaite]

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.

[WebKit Review Bot]

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.

[Adam Roben (:aroben)]

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.

[Jenn Braithwaite]

Created attachment 72533 [details]
updated patch - fix CRLF, add blank line

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.