Bug 25347 - Add a FrameLoaderClient callback for the ResourceRetrievedByXMLHttpRequest.
: Add a FrameLoaderClient callback for the ResourceRetrievedByXMLHttpRequest.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-23 14:10 PDT by Pavel Feldman
Modified: 2009-05-01 08:25 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2009-04-23 14:10:19 PDT
It is the only resource-related information that is available in InspectorController that is missing in the FrameLoaderClient.
Comment 1 Pavel Feldman 2009-04-23 14:12:30 PDT
Created attachment 29714 [details]
patch
Comment 2 Sam Weinig 2009-04-23 18:23:56 PDT
Why is this necessary?  Please include that kind of information in the future.
Comment 3 Pavel Feldman 2009-04-23 22:12:34 PDT
(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 Sam Weinig 2009-04-23 22:23:26 PDT
It will take me some time to split InspectorController and I don't want
Chromium guys to be blocked by that
Comment 5 Sam Weinig 2009-04-23 22:27:43 PDT
(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 Pavel Feldman 2009-04-23 22:46:02 PDT
(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 Sam Weinig 2009-04-24 10:20:32 PDT
So, to clarify, this is not something that is need for unforking chromium?
Comment 8 Pavel Feldman 2009-04-25 02:02:10 PDT
(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 Pavel Feldman 2009-04-27 12:14:04 PDT
(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 Timothy Hatcher 2009-04-29 10:47:59 PDT
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
Comment 11 Pavel Feldman 2009-04-29 11:53:00 PDT
Created attachment 29887 [details]
patch

Updated after Timothy's review.
Comment 12 Pavel Feldman 2009-04-29 12:06:49 PDT
(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 Pavel Feldman 2009-04-30 12:06:20 PDT
(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 Timothy Hatcher 2009-04-30 12:22:21 PDT
Comment on attachment 29887 [details]
patch

Please add the bug URL to the ChangeLog next time. Add before landing.
Comment 15 Pavel Feldman 2009-05-01 02:35:33 PDT
Created attachment 29937 [details]
patch

Added bug url to the ChangeLog.
Comment 16 Pavel Feldman 2009-05-01 03:12:54 PDT
Created attachment 29938 [details]
patch

Removed space that squeezed into the method declaration.
Comment 17 Dimitri Glazkov (Google) 2009-05-01 08:25:51 PDT
Landed as http://trac.webkit.org/changeset/43113.