Bug 25347 – Add a FrameLoaderClient callback for the ResourceRetrievedByXMLHttpRequest.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Pavel Feldman]

It is the only resource-related information that is available in InspectorController that is missing in the FrameLoaderClient.

[Pavel Feldman]

Created attachment 29714 [details]
patch

[Sam Weinig]

Why is this necessary?  Please include that kind of information in the future.

[Pavel Feldman]

(In reply to comment #2)
> Why is this necessary?  Please include that kind of information in the future.
> 

This is a continuation of https://bugs.webkit.org/show_bug.cgi?id=25342, copying notes from there:

Our async/out-of-process version of WebInspector is currently not based on
InspectorController. The reason was that we did not want to interfere with
Dmitry's unforking effort, yet wanted to experiment. So we came up with these
agents concept that basically mimic InspectorController, but separating 'agent'
nature from the 'transport'. Now that InspectorController is unforked, I am
planning to bring these concepts into the WebKit land and use what we have in
Chromium as a proof of concept / experimental playground.

It will take me some time to split InspectorController and I don't want
Chromium guys to be blocked by that. Hence I want to expose events that are
missing in clients, namely: console severity and xmlhttprequest data. I realize
that this is somewhat a temporary measure, but I think it will serve us all
well.

[Sam Weinig]

It will take me some time to split InspectorController and I don't want
Chromium guys to be blocked by that

[Sam Weinig]

(In reply to comment #4)
> It will take me some time to split InspectorController and I don't want
> Chromium guys to be blocked by that

Blerg, not what I meant to paste.  I meant to ask is if this is to help the unforking of chromium or to some other experimental work?

[Pavel Feldman]

(In reply to comment #5)
> (In reply to comment #4)
> > It will take me some time to split InspectorController and I don't want
> > Chromium guys to be blocked by that
> 
> Blerg, not what I meant to paste.  I meant to ask is if this is to help the
> unforking of chromium or to some other experimental work?
> 

Here is a broader context: I am currently working on Elements and Resources tabs to allow their async operation. Yury is working on debugger, Mikhail is working on Profiler. The 'work' that they are doing is about making Chromium's internals ready to provide information required and patch/refactor WebInspector to allow async operation in respective areas.

Before I dive into splitting InspectorController, I would like to make sure that they are not blocked with what I am doing. As I explained above, our experimental agents are outside WebCore (we needed those to make this thing running out of process and not to clash with Dmitry's effort). But they are missing some information. In particular, Yury is being blocked with the XmlHttp data absence and I am trying to fix this. That is why I need it, but I am sure that having it exposed to loader client is anyway a good thing.

Now about our 'experimental' work: it is entirely open, in Chromium repository, located in webkit/glue/devtools folder. It is really not much - a thousand lines of code that mimic InspectorController and establish an RPC-alike interaction with WebInspector. That is something we are using as a proof of concept when bringing code into WebKit.

[Sam Weinig]

So, to clarify, this is not something that is need for unforking chromium?

[Pavel Feldman]

(In reply to comment #7)
> So, to clarify, this is not something that is need for unforking chromium?
> 

Sorry, I thought I made it clear. I do confirm, this is not for unforking. This is rather for not forking. Apart from the Chromium business, do you think this change makes sense?

[Pavel Feldman]

(In reply to comment #8)
> (In reply to comment #7)
> > So, to clarify, this is not something that is need for unforking chromium?
> > 
> 
> Sorry, I thought I made it clear. I do confirm, this is not for unforking. This
> is rather for not forking. Apart from the Chromium business, do you think this
> change makes sense?
> 

Ping.

Note, that I've shared a doc/proposal with you (http://docs.google.com/Doc?id=dcf65mvz_0dp6xt4c8) that shows how I think we should approach the InspectorController refactoring. Once this refactoring is done, this change will become obsolete. But as I mentioned before, it will take some time to settle this plan down. In the meanwhile, this change is helpful and does not seem to produce harm (please correct me if I am wrong).

[Timothy Hatcher]

Comment on attachment 29714 [details]
patch

+        virtual void dispatchResourceRetrievedByXMLHttpRequest(unsigned long, const WebCore::ScriptString&);

The name for this function does not match other client functions.

Something like this would fit better:

dispatchDidLoadResourceByXMLHttpRequest

+        void resourceRetrievedByXMLHttpRequest(unsigned long identifier, const ScriptString& sourceString);

This should be renamed to something that matches too. Maybe:

didLoadResourceByXMLHttpRequest

[Pavel Feldman]

Created attachment 29887 [details]
patch

Updated after Timothy's review.

[Pavel Feldman]

(In reply to comment #10)
> (From update of attachment 29714 [details] [review])
> +        virtual void dispatchResourceRetrievedByXMLHttpRequest(unsigned long,
> const WebCore::ScriptString&);
> 
> The name for this function does not match other client functions.
> 
> Something like this would fit better:
> 
> dispatchDidLoadResourceByXMLHttpRequest
> 
> +        void resourceRetrievedByXMLHttpRequest(unsigned long identifier, const
> ScriptString& sourceString);
> 
> This should be renamed to something that matches too. Maybe:
> 
> didLoadResourceByXMLHttpRequest
> 

Done. Thanks for the review!

[Pavel Feldman]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 29714 [details] [review] [review])
> > +        virtual void dispatchResourceRetrievedByXMLHttpRequest(unsigned long,
> > const WebCore::ScriptString&);
> > 
> > The name for this function does not match other client functions.
> > 
> > Something like this would fit better:
> > 
> > dispatchDidLoadResourceByXMLHttpRequest
> > 
> > +        void resourceRetrievedByXMLHttpRequest(unsigned long identifier, const
> > ScriptString& sourceString);
> > 
> > This should be renamed to something that matches too. Maybe:
> > 
> > didLoadResourceByXMLHttpRequest
> > 
> 
> Done. Thanks for the review!
> 

Ping. Could you give it another look please?

Thanks
Pavel

[Timothy Hatcher]

Comment on attachment 29887 [details]
patch

Please add the bug URL to the ChangeLog next time. Add before landing.

[Pavel Feldman]

Created attachment 29937 [details]
patch

Added bug url to the ChangeLog.

[Pavel Feldman]

Created attachment 29938 [details]
patch

Removed space that squeezed into the method declaration.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/43113.