Bug 25347 - Add a FrameLoaderClient callback for the ResourceRetrievedByXMLHttpRequest.
: Add a FrameLoaderClient callback for the ResourceRetrievedByXMLHttpRequest.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-04-23 14:10 PST by
Modified: 2009-05-01 08:25 PST (History)


Attachments
patch (18.63 KB, patch)
2009-04-23 14:12 PST, Pavel Feldman
timothy: review-
Review Patch | Details | Formatted Diff | Diff
patch (18.73 KB, patch)
2009-04-29 11:53 PST, Pavel Feldman
timothy: review+
Review Patch | Details | Formatted Diff | Diff
patch (19.07 KB, patch)
2009-05-01 02:35 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff
patch (19.07 KB, patch)
2009-05-01 03:12 PST, Pavel Feldman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-04-23 14:10:19 PST
It is the only resource-related information that is available in InspectorController that is missing in the FrameLoaderClient.
------- Comment #1 From 2009-04-23 14:12:30 PST -------
Created an attachment (id=29714) [details]
patch
------- Comment #2 From 2009-04-23 18:23:56 PST -------
Why is this necessary?  Please include that kind of information in the future.
------- Comment #3 From 2009-04-23 22:12:34 PST -------
(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.
------- Comment #4 From 2009-04-23 22:23:26 PST -------
It will take me some time to split InspectorController and I don't want
Chromium guys to be blocked by that
------- Comment #5 From 2009-04-23 22:27:43 PST -------
(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?
------- Comment #6 From 2009-04-23 22:46:02 PST -------
(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.
------- Comment #7 From 2009-04-24 10:20:32 PST -------
So, to clarify, this is not something that is need for unforking chromium?
------- Comment #8 From 2009-04-25 02:02:10 PST -------
(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?
------- Comment #9 From 2009-04-27 12:14:04 PST -------
(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).
------- Comment #10 From 2009-04-29 10:47:59 PST -------
(From update of attachment 29714 [details])
+        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
------- Comment #11 From 2009-04-29 11:53:00 PST -------
Created an attachment (id=29887) [details]
patch

Updated after Timothy's review.
------- Comment #12 From 2009-04-29 12:06:49 PST -------
(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!
------- Comment #13 From 2009-04-30 12:06:20 PST -------
(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
------- Comment #14 From 2009-04-30 12:22:21 PST -------
(From update of attachment 29887 [details])
Please add the bug URL to the ChangeLog next time. Add before landing.
------- Comment #15 From 2009-05-01 02:35:33 PST -------
Created an attachment (id=29937) [details]
patch

Added bug url to the ChangeLog.
------- Comment #16 From 2009-05-01 03:12:54 PST -------
Created an attachment (id=29938) [details]
patch

Removed space that squeezed into the method declaration.
------- Comment #17 From 2009-05-01 08:25:51 PST -------
Landed as http://trac.webkit.org/changeset/43113.