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
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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
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
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
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?
> 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
> 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.
Created attachment 169073 [details] Patch
Created attachment 169205 [details] Patch
This is another example of a use-case we have for tracking who initiated a load.
Bug 84883 has another approach for doing something similar.
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.
Created attachment 175621 [details] Patch
Created attachment 175651 [details] Patch
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
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.
Created attachment 175771 [details] Patch
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
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
Created attachment 175795 [details] Patch
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
Created attachment 175800 [details] Patch
The latest patch also renames the new class to WebCachedURLRequest (as discussed). abarth@, could you have a look?
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.
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.
The general approach in this patch looks good. We just need to clean up the API design a bit.
Created attachment 176263 [details] Patch
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.
Created attachment 176266 [details] Patch
Comment on attachment 175800 [details] Patch Thanks for comments!
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.
Created attachment 176285 [details] Patch
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.
Comment on attachment 176285 [details] Patch Clearing flags on attachment: 176285 Committed r135872: <http://trac.webkit.org/changeset/135872>
All reviewed patches have been landed. Closing bug.
This broke the clang compile. I landed a fix in http://trac.webkit.org/changeset/135877.
tony@: Ahh, I see. Thanks for fixing it!