Bug 92761 – Add callbacks to the FrameLoaderClient when a resource is requested

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Add callbacks to the FrameLoaderClient for when a resource request finished for a script or image element

Created attachment 155552 [details]
Patch

Adam, can you have a look plz

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 155552 [details]
Patch

Attachment 155552 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13386888

New failing tests:
canvas/philip/tests/2d.composite.image.source-over.html
http/tests/cache/stopped-revalidation.html
canvas/philip/tests/2d.composite.globalAlpha.canvas.html
canvas/philip/tests/2d.composite.globalAlpha.fill.html
canvas/philip/tests/2d.composite.clip.source-over.html
canvas/philip/tests/2d.composite.globalAlpha.imagepattern.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
accessibility/loading-iframe-sends-notification.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.clip.lighter.html
canvas/philip/tests/2d.composite.image.source-in.html
canvas/philip/tests/2d.composite.image.lighter.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.composite.image.destination-out.html
canvas/philip/tests/2d.composite.image.destination-atop.html
canvas/philip/tests/2d.clearRect.path.html
accessibility/image-link-inline-cont.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.clip.destination-out.html
accessibility/content-changed-notification-causes-crash.html
canvas/philip/tests/2d.composite.canvas.lighter.html
accessibility/img-alt-tag-only-whitespace.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.composite.clip.source-in.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[WebKit Review Bot]

Created attachment 155562 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 155552 [details]
Patch

Attachment 155552 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13400486

Created attachment 155734 [details]
Patch

Created attachment 155735 [details]
Patch

Created attachment 155773 [details]
Patch

[Adam Barth]

Comment on attachment 155773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155773&action=review

I'm tempted to r+ this with comments, but I think I'd like to see one more round, if that's ok with you.

> Source/WebCore/loader/FrameLoaderClient.h:167
> +        virtual void dispatchImageLoadFinished(Element*, CachedResource*) { }
> +        virtual void dispatchScriptLoadFinished(Element*, CachedResource*) { }

Why not provide more concerete types?  e.g., dispatchImageLoadFinished(Element*, CachedImage*) { }

> Source/WebCore/loader/ImageLoader.cpp:270
> +    document()->loader()->frameLoader()->client()->dispatchImageLoadFinished(client()->sourceElement(), resource);

Going through the DocumentLoader for these calls doesn't seem quite right.  It makes more sense to me to go through the Frame:

document()->frame()->loader()->client()->dispatchImageLoadFinished(client()->sourceElement(), resource);

Also, do we need to null-check the frame?

> Source/WebKit/chromium/public/WebFrameClient.h:227
> +    // The URL request for the given image element finished loading.
> +    virtual void imageLoadFinished(WebFrame*, const WebElement&, const WebURLRequest&) { }

imageLoadFinished -> didFinishImageLoad

That fits the naming pattern of the functions above.

> Source/WebKit/chromium/public/WebFrameClient.h:230
> +    // The URL request for the given script element finished loading.
> +    virtual void scriptLoadFinished(WebFrame*, const WebElement&, const WebURLRequest&) { }

ditto

> Source/WebKit/chromium/src/FrameLoaderClientImpl.h:111
> +    virtual void dispatchImageLoadFinished(WebCore::Element* sourceElement, WebCore::CachedResource*);
> +    virtual void dispatchScriptLoadFinished(WebCore::Element* sourceElement, WebCore::CachedResource*);

These should also probably be like dispatchDidFinishImageLoad to match the similar callbacks above.

I have another question for you:

as is, the callbacks are only invoked when the requested resource was not in the cache but had to be loaded. I guess that's ok for the targetted use case, because we'd return a temporary error for the resource load, and I expect that every element will end up trying to load the resource over the network. Is that correct?

[Adam Barth]

> as is, the callbacks are only invoked when the requested resource was not in the cache but had to be loaded. I guess that's ok for the targetted use case, because we'd return a temporary error for the resource load, and I expect that every element will end up trying to load the resource over the network. Is that correct?

Doesn't didLoadResourceFromMemoryCache cover that case?  Maybe we should have a unified didLoadResourceFromNetwork callback rather than per-type notifications.

(In reply to comment #12)
> > as is, the callbacks are only invoked when the requested resource was not in the cache but had to be loaded. I guess that's ok for the targetted use case, because we'd return a temporary error for the resource load, and I expect that every element will end up trying to load the resource over the network. Is that correct?
> 
> Doesn't didLoadResourceFromMemoryCache cover that case?  Maybe we should have a unified didLoadResourceFromNetwork callback rather than per-type notifications.

that callback doesn't have the element that requested the resource. The main point of the new callbacks is to connect the element with the request for the embedder.

Of course, not all requests have an associated element, so I didn't come up with a good name and figured individual callbacks would be clearer

[Adam Barth]

> Of course, not all requests have an associated element, so I didn't come up with a good name and figured individual callbacks would be clearer

I mean, not all image loads have an element either, so we need to solve that problem in any case.

[Marja Hölttä]

Created attachment 169073 [details]
Patch

[Marja Hölttä]

Created attachment 169205 [details]
Patch

[Adam Barth]

This is another example of a use-case we have for tracking who initiated a load.

[Adam Barth]

Bug 84883 has another approach for doing something similar.

[Adam Barth]

Comment on attachment 169205 [details]
Patch

We discussed this over VC.  We're going to merge this work with the work on the bug mentioned above.  The plan is to create a CachedResourceRequest and then to have a frameloaderclient callback similar to willSendRequest but for this CachedResourceRequest.

[Marja Hölttä]

Created attachment 175621 [details]
Patch

[Marja Hölttä]

Created attachment 175651 [details]
Patch

[Build Bot]

Comment on attachment 175651 [details]
Patch

Attachment 175651 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14961377

Comment on attachment 175651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175651&action=review

> Source/WebCore/loader/FrameLoaderClient.h:170
> +        virtual void dispatchResourceRequested(const AtomicString& initiatorName, const KURL&) { }

you need to omit "initatorName" as it's unused

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:426
> +    if (Frame* f = frame()) {

do you know when frame is 0?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:430
> +            f->loader()->client()->dispatchResourceRequested(request.initiatorName(), request.resourceRequest().url());

what should happen if initiatorName is ""?

> LayoutTests/platform/chromium/fast/loader/resource-request-callbacks.html:62
> +  This test checks that the correct callbacks for resource requests are

nit. indent 4 spaces

[Adam Barth]

Comment on attachment 175651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175651&action=review

> Source/WebCore/loader/FrameLoaderClient.h:169
> +        virtual void dispatchResourceRequested(Element*, const KURL&) { }

These are really "WillRequestResource" since the request hasn't happened yet.

Also, why not pass the entire CachedResourceRequest?  That would seem to give the WebKit layer the most flexibility.

[Marja Hölttä]

Created attachment 175771 [details]
Patch

[Marja Hölttä]

Comment on attachment 175651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175651&action=review

>> Source/WebCore/loader/FrameLoaderClient.h:169
>> +        virtual void dispatchResourceRequested(Element*, const KURL&) { }
> 
> These are really "WillRequestResource" since the request hasn't happened yet.
> 
> Also, why not pass the entire CachedResourceRequest?  That would seem to give the WebKit layer the most flexibility.

Renamed this to dispatchWillRequestResource & renamed other funcs correspondingly. Added WebCachedResourceRequest and passed that to the WebKit layer.

>> Source/WebCore/loader/FrameLoaderClient.h:170
>> +        virtual void dispatchResourceRequested(const AtomicString& initiatorName, const KURL&) { }
> 
> you need to omit "initatorName" as it's unused

Done

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:426
>> +    if (Frame* f = frame()) {
> 
> do you know when frame is 0?

In requestImage, there is check for frame() being 0, and it calls this func even in that case -> we need to check here, too.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:430
>> +            f->loader()->client()->dispatchResourceRequested(request.initiatorName(), request.resourceRequest().url());
> 
> what should happen if initiatorName is ""?

This is no longer an issue since we're passing a WebCachedResourceRequest.

>> LayoutTests/platform/chromium/fast/loader/resource-request-callbacks.html:62
>> +  This test checks that the correct callbacks for resource requests are
> 
> nit. indent 4 spaces

Done

[WebKit Review Bot]

Comment on attachment 175771 [details]
Patch

Attachment 175771 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14961729

New failing tests:
fast/text/atsui-small-caps-punctuation-size.html

Comment on attachment 175771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175771&action=review

> Source/WebKit/chromium/public/WebCachedResourceRequest.h:32
> +#define WebCachedResourceRequest_h

I think this should go into Source/Platform/chromium/public

> Source/WebKit/chromium/public/WebCachedResourceRequest.h:53
> +    WEBKIT_EXPORT WebURL url() const;

why not return a WebURLRequest?

> Source/WebKit/chromium/public/WebCachedResourceRequest.h:65
> +protected:

why not private?

(In reply to comment #28)
> (From update of attachment 175771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175771&action=review
> 
> > Source/WebKit/chromium/public/WebCachedResourceRequest.h:32
> > +#define WebCachedResourceRequest_h
> 
> I think this should go into Source/Platform/chromium/public

Actually, disregard that comment, the location is already correct

[Marja Hölttä]

Created attachment 175795 [details]
Patch

[Marja Hölttä]

Comment on attachment 175771 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175771&action=review

>> Source/WebKit/chromium/public/WebCachedResourceRequest.h:53
>> +    WEBKIT_EXPORT WebURL url() const;
> 
> why not return a WebURLRequest?

Done

>> Source/WebKit/chromium/public/WebCachedResourceRequest.h:65
>> +protected:
> 
> why not private?

Done

looks quite reasonable, I like it. Let's wait for Adam

[Marja Hölttä]

Created attachment 175800 [details]
Patch

[Marja Hölttä]

The latest patch also renames the new class to WebCachedURLRequest (as discussed).

abarth@, could you have a look?

[Adam Barth]

Comment on attachment 175800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175800&action=review

> Source/WebKit/chromium/public/WebCachedURLRequest.h:48
> +    ~WebCachedURLRequest();

This isn't quite the right pattern for the dtor.  The problem is that you haven't exported the dtor and it's neither virtual nor inline.  Take a look at WebURLRequest for the correct pattern.  Basically, you want an inline dtor that calls an exported reset() function.

> Source/WebKit/chromium/public/WebCachedURLRequest.h:53
> +    void assign(const WebCachedURLRequest&);
> +    bool equals(const WebCachedURLRequest&) const;

These need to be exported.  See WebURLRequest for an example.

> Source/WebKit/chromium/public/WebCachedURLRequest.h:56
> +    WEBKIT_EXPORT WebString charset() const;

Don't you need to include WebString.h for these declaration?

> Source/WebKit/chromium/public/WebCachedURLRequest.h:59
> +    WEBKIT_EXPORT WebElement initiatorElement() const;

I would expect you'd need to include WebElement as well.

> Source/WebKit/chromium/src/WebCachedURLRequest.cpp:37
> +#include <public/WebElement.h>

There's no public/ prefix for WebElement.

We need to improve the header including patterns, but headers in the Client part of the API (the part in WebKit/chromium/public) are just included with their base name.  Headers in the Platform part of the API (the part in Platform/chromium/public) need the public/WebFoo.h pattern.

[Adam Barth]

Comment on attachment 175800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175800&action=review

> Source/WebKit/chromium/public/WebCachedURLRequest.h:50
> +    WebCachedURLRequest(const WebCachedURLRequest& r) { assign(r); }

The problem with this API is that it implies that the embedder can retain a WebCachedURLRequest object.  However, because m_private is a raw pointer, the embedder has no control over the lifetime of the object m_private points to and can easily end up with a garbage pointer.

The solution to this problem is to move the constructor, destructor, and operator= to the private section of the class declaration.  That implies that the embedder can receive a pointer (or a reference) to this type of object but cannot make a copy.

> Source/WebKit/chromium/src/WebCachedURLRequest.cpp:52
> +    m_private = other.m_private;

This assign() function doesn't move over m_resourceRequestWrapper.  That's likely because m_resourceRequestWrapper is a WebPrivateOwnPtr, which prevents copies.  Instead of having a wonky asign() function, we just need to prevent copies of WebCachedURLRequest as well.

[Adam Barth]

The general approach in this patch looks good.  We just need to clean up the API design a bit.

[Marja Hölttä]

Created attachment 176263 [details]
Patch

[Adam Barth]

Comment on attachment 176263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176263&action=review

> Source/WebKit/chromium/public/WebCachedURLRequest.h:38
> +#include <wtf/Noncopyable.h>

We cannot include WTF headers in the API.

> Source/WebKit/chromium/public/WebCachedURLRequest.h:50
> +WTF_MAKE_NONCOPYABLE(WebCachedURLRequest);

You need to do this manually rather than using the macro.

[Marja Hölttä]

Created attachment 176266 [details]
Patch

[Marja Hölttä]

Comment on attachment 175800 [details]
Patch

Thanks for comments!

[Adam Barth]

Comment on attachment 176266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176266&action=review

Thanks for iterating on the API.  This patch looks good with one minor nit below.

> Source/WebKit/chromium/public/WebCachedURLRequest.h:37
> +#include "platform/WebCommon.h"
> +#include "platform/WebPrivateOwnPtr.h"
> +#include <public/WebString.h>

These includes are inconsistent.  I think we use the "platform/" style in the API.

[Marja Hölttä]

Created attachment 176285 [details]
Patch

[Adam Barth]

Comment on attachment 176285 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176285&action=review

> Source/WebKit/chromium/public/WebCachedURLRequest.h:44
> +class WebElement;
> +class WebString;

These aren't needed anymore, but they're also not harmful.

[WebKit Review Bot]

Comment on attachment 176285 [details]
Patch

Clearing flags on attachment: 176285

Committed r135872: <http://trac.webkit.org/changeset/135872>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

This broke the clang compile. I landed a fix in http://trac.webkit.org/changeset/135877.

[Marja Hölttä]

tony@: Ahh, I see. Thanks for fixing it!