Bug 47779 - Web Inspector: Introduce InspectorResourceAgent.h/cpp and ResourceManager.js to fill network panel with data.
Summary: Web Inspector: Introduce InspectorResourceAgent.h/cpp and ResourceManager.js ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-17 06:30 PDT by Pavel Feldman
Modified: 2010-10-18 03:15 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed change. (72.00 KB, patch)
2010-10-17 09:30 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff
[PATCH] For bots. (73.27 KB, patch)
2010-10-18 02:42 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 2010-10-17 06:30:22 PDT
New InspectorResourceAgent is introduced, its lifetime is equal to the front-end's. This new resource agent does not have state, instead it covers two functions: 1) forwards loader client messages to the front-end 2) is capable of building a tree of cached resources. (1) feeds network panel with data, (2) shows the resource tree in the new ResourcesPanel concept. This bug is for extracting this new InspectorResourceAgent class and its javascript counterpart. Once resources panel is refactored for the new concept, InspectorResource, InspectorController's resource-related routines, inspector.js's code dealing with resources, they all will be gone.
Comment 1 Pavel Feldman 2010-10-17 09:30:55 PDT
Created attachment 70974 [details]
[PATCH] Proposed change.
Comment 2 Pavel Feldman 2010-10-17 09:43:16 PDT
Comment on attachment 70974 [details]
[PATCH] Proposed change.

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

Annotating the change since it is quite big. View it in context ^^.

> WebCore/inspector/Inspector.idl:115
> +        [handler=Resource] void cachedResources(out Array resources);

Instead of the stateful InspectorResource collection, we now pass all the events to the front-end. We collect nothing while there is no front-end.

> WebCore/inspector/InspectorController.cpp:502
> +    // FIXME: enable resource agent once front-end is ready.

The new agent is not yet enabled, so we are not breaking any code.

> WebCore/inspector/InspectorController.cpp:1054
> +void InspectorController::didReceiveResponse(unsigned long identifier, DocumentLoader* loader, const ResourceResponse& response)

I needed a loader here, so I now push it to inspector. I think it was so long ago as well.

> WebCore/inspector/InspectorResource.cpp:-193
> -    if (!m_cached && m_loadTiming && m_loadTiming->requestTime)

This just moved to the front-end, it Resource.js is now responsible for using precise network timing.

> WebCore/inspector/InspectorResource.cpp:338
> +static InspectorResource::Type cachedResourceType(CachedResource* cachedResource)

Is now only used here, so moving it back from InspectorResourceAgent.

> WebCore/inspector/InspectorResource.cpp:-381
> -    if (!InspectorResourceAgent::resourceContent(m_frame->document(), m_requestURL, &result))

I changed the signature of static InspectorResourceAgent methods to receive frame, not document.

> WebCore/inspector/InspectorResource.cpp:-411
> -    if (actualEndTime) {

This logic just moved to Resource.js

> WebCore/inspector/InspectorResourceAgent.cpp:135
> +static PassRefPtr<InspectorObject> buildObjectForHeaders(const HTTPHeaderMap& headers)

Here and below, these are new instance methods that will replace InspectorResource.cpp and InspectorController's networking aspect soon.

> WebCore/inspector/InspectorResourceAgent.cpp:289
> +    String type = "Other";

This logic is taken from InspectorResource.cpp. That's the one I need DocumentLoader instance for (in the provisional load cases).

> WebCore/inspector/InspectorResourceAgent.cpp:358
> +// Create human-readable binary representation, like "01:23:45:67:89:AB:CD:EF".

This is taken from InspectorResource, original will get nuked once we migrate.

> WebCore/inspector/front-end/ExtensionServer.js:-243
> -        resource = typeof id === "number" ? WebInspector.resources[id] : WebInspector.resourceForURL(id);

Ids can be non-numbers, changed extension code a bit.

> WebCore/inspector/front-end/NetworkPanel.js:130
> +            // Glue status to bottom.

Drive-by fix.

> WebCore/inspector/front-end/NetworkPanel.js:365
> +            if (this._summaryBarElement._isDisplayingWarning)

Drive-by 'no resources' status to avoid confusion with the blank network panel.

> WebCore/inspector/front-end/NetworkPanel.js:-728
> -        delete this.currentQuery;

There is no search support in network panel yet, just nuking what has left from the ResourcesPanel cloning.

> WebCore/inspector/front-end/ResourceManager.js:31
> +WebInspector.ResourceManager = function()

This guy is the javascript counterpart of the InspectorResourceAgent. It binds notify methods using _registerNotifyHandlers.

> WebCore/loader/appcache/ApplicationCacheGroup.cpp:-526
> -            page->inspectorController()->didReceiveResponse(m_currentResourceIdentifier, response);

This thing shows the diff poorly to me. See text diff for real changes in case you hit the same issue.
Comment 3 Ilya Tikhonovsky 2010-10-17 13:41:36 PDT
Comment on attachment 70974 [details]
[PATCH] Proposed change.

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

> WebCore/inspector/InspectorController.cpp:1061
> +    if (m_resourceAgent)
> +        m_resourceAgent->didReceiveResponse(identifier, loader, response);
> +

What do you think about moving such calls under control of InspectorInstrumentation API?
Comment 4 Pavel Feldman 2010-10-18 02:24:54 PDT
(In reply to comment #3)
> (From update of attachment 70974 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70974&action=review
> 
> > WebCore/inspector/InspectorController.cpp:1061
> > +    if (m_resourceAgent)
> > +        m_resourceAgent->didReceiveResponse(identifier, loader, response);
> > +
> 
> What do you think about moving such calls under control of InspectorInstrumentation API?

Yep, as soon as InspectorResourceAgent becomes the only guy receiving network events, they will get routed via InspectorInstrumentation.
Comment 5 Pavel Feldman 2010-10-18 02:42:52 PDT
Created attachment 71010 [details]
[PATCH] For bots.
Comment 6 WebKit Review Bot 2010-10-18 02:45:46 PDT
Attachment 71010 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/InspectorResourceAgent.cpp:223:  More than one command on the same line  [whitespace/newline] [4]
WebCore/inspector/InspectorResourceAgent.cpp:225:  More than one command on the same line  [whitespace/newline] [4]
WebCore/inspector/InspectorResourceAgent.cpp:231:  More than one command on the same line  [whitespace/newline] [4]
WebCore/inspector/InspectorResourceAgent.cpp:233:  More than one command on the same line  [whitespace/newline] [4]
WebCore/inspector/InspectorResourceAgent.cpp:334:  More than one command on the same line  [whitespace/newline] [4]
WebCore/inspector/InspectorResourceAgent.cpp:336:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Yury Semikhatsky 2010-10-18 02:48:18 PDT
Comment on attachment 70974 [details]
[PATCH] Proposed change.

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

> WebCore/inspector/Inspector.idl:116
> +        [handler=Resource] void resourceContent(in unsigned long frameID, in String url, out String content);

should be getResourceContent

> WebCore/inspector/InspectorController.cpp:503
> +    // m_resourceAgent = InspectorResourceAgent::create(m_inspectedPage, m_frontend.get());

This code should be hidden behind a macro definition.

>> WebCore/inspector/InspectorResourceAgent.cpp:135
>> +static PassRefPtr<InspectorObject> buildObjectForHeaders(const HTTPHeaderMap& headers)
> 
> Here and below, these are new instance methods that will replace InspectorResource.cpp and InspectorController's networking aspect soon.

These are static functions, seems like you meant the methods a few screens below:)

> WebCore/inspector/InspectorResourceAgent.cpp:223
> +        type = "Image"; break;

Prefer early exit, just return "Image";

> WebCore/inspector/InspectorResourceAgent.cpp:225
> +        type = "Font"; break;

ditto

> WebCore/inspector/InspectorResourceAgent.cpp:231
> +        type = "Stylesheet"; break;

ditto

> WebCore/inspector/InspectorResourceAgent.cpp:233
> +        type = "Script"; break;

ditto

> WebCore/inspector/InspectorResourceAgent.cpp:235
> +        type = "Other";

ditto

> WebCore/inspector/InspectorResourceAgent.cpp:272
> +void InspectorResourceAgent::identifierForInitialRequest(unsigned long identifier, const KURL& url, DocumentLoader* loader, bool isMainResource)

There is no much sense in keeping clones of these methods on IC since they are pure delegates, are you going to remove it from there?

> WebCore/inspector/InspectorResourceAgent.cpp:362
> +    static const char hexDigits[17] = "0123456789ABCDEF";

Why not move this code to the FE?

>> WebCore/inspector/front-end/ExtensionServer.js:-243
>> -        resource = typeof id === "number" ? WebInspector.resources[id] : WebInspector.resourceForURL(id);
> 
> Ids can be non-numbers, changed extension code a bit.

Previous version was clearer, can you revert this?

> WebCore/inspector/front-end/Resource.js:169
> +            return this.timing.requestTime + this.timing.receiveHeadersEnd / 1000.0;

The comment is inconsistent with the code and I don't understand why the formula will give you response receive time, could you provide more details?

> WebCore/inspector/front-end/Resource.js:630
> +            InspectorBackend.getResourceContent(this.identifier, false, callback);

there is no getResourceContent in Inspector.idl, only resourceContent. Please fix this.

>> WebCore/loader/appcache/ApplicationCacheGroup.cpp:-526
>> -            page->inspectorController()->didReceiveResponse(m_currentResourceIdentifier, response);
> 
> This thing shows the diff poorly to me. See text diff for real changes in case you hit the same issue.

I see the line is deleted even in the plain text diff. Should the "else" be removed too? or how is it supposed to compiled?
Comment 8 Pavel Feldman 2010-10-18 02:59:44 PDT
Thanks for the review!

(In reply to comment #7)
> (From update of attachment 70974 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70974&action=review
> 
> > WebCore/inspector/Inspector.idl:116
> > +        [handler=Resource] void resourceContent(in unsigned long frameID, in String url, out String content);
> 
> should be getResourceContent
> 

There alreadty is one (for the old code path that will go away with InspectorResource).

> > WebCore/inspector/InspectorController.cpp:503
> > +    // m_resourceAgent = InspectorResourceAgent::create(m_inspectedPage, m_frontend.get());
> 
> This code should be hidden behind a macro definition.
> 

Please let me ignore this for now.

> >> WebCore/inspector/InspectorResourceAgent.cpp:135
> >> +static PassRefPtr<InspectorObject> buildObjectForHeaders(const HTTPHeaderMap& headers)
> > 
> > Here and below, these are new instance methods that will replace InspectorResource.cpp and InspectorController's networking aspect soon.
> 
> These are static functions, seems like you meant the methods a few screens below:)
> 

Yep.

> > WebCore/inspector/InspectorResourceAgent.cpp:223
> > +        type = "Image"; break;
> 
> Prefer early exit, just return "Image";
> 
> > WebCore/inspector/InspectorResourceAgent.cpp:225
> > +        type = "Font"; break;
> 
> ditto
> 
> > WebCore/inspector/InspectorResourceAgent.cpp:231
> > +        type = "Stylesheet"; break;
> 
> ditto
> 
> > WebCore/inspector/InspectorResourceAgent.cpp:233
> > +        type = "Script"; break;
> 
> ditto
> 
> > WebCore/inspector/InspectorResourceAgent.cpp:235
> > +        type = "Other";
> 
> ditto
> 

Done. It was extracted late, could not use return inline.

> > WebCore/inspector/InspectorResourceAgent.cpp:272
> > +void InspectorResourceAgent::identifierForInitialRequest(unsigned long identifier, const KURL& url, DocumentLoader* loader, bool isMainResource)
> 
> There is no much sense in keeping clones of these methods on IC since they are pure delegates, are you going to remove it from there?
> 

Exactly.

> > WebCore/inspector/InspectorResourceAgent.cpp:362
> > +    static const char hexDigits[17] = "0123456789ABCDEF";
> 
> Why not move this code to the FE?
> 

I will think about it :) FIXME for now.

> >> WebCore/inspector/front-end/ExtensionServer.js:-243
> >> -        resource = typeof id === "number" ? WebInspector.resources[id] : WebInspector.resourceForURL(id);
> > 
> > Ids can be non-numbers, changed extension code a bit.
> 
> Previous version was clearer, can you revert this?
> 

Not really, it is important that IDs can be non-numbers,

> > WebCore/inspector/front-end/Resource.js:169
> > +            return this.timing.requestTime + this.timing.receiveHeadersEnd / 1000.0;
> 
> The comment is inconsistent with the code and I don't understand why the formula will give you response receive time, could you provide more details?
> 

It was simply moved from native code. Added comment.

> > WebCore/inspector/front-end/Resource.js:630
> > +            InspectorBackend.getResourceContent(this.identifier, false, callback);
> 
> there is no getResourceContent in Inspector.idl, only resourceContent. Please fix this.
> 

There is one.

> >> WebCore/loader/appcache/ApplicationCacheGroup.cpp:-526
> >> -            page->inspectorController()->didReceiveResponse(m_currentResourceIdentifier, response);
> > 
> > This thing shows the diff poorly to me. See text diff for real changes in case you hit the same issue.
> 
> I see the line is deleted even in the plain text diff. Should the "else" be removed too? or how is it supposed to compiled?

There is a good diff - that file was corrupt. Bots compile!
Comment 9 Pavel Feldman 2010-10-18 03:15:15 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/English.lproj/localizedStrings.js
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/InspectorApplicationCacheAgent.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorResource.cpp
	M	WebCore/inspector/InspectorResourceAgent.cpp
	M	WebCore/inspector/InspectorResourceAgent.h
	M	WebCore/inspector/InspectorStyleSheet.cpp
	M	WebCore/inspector/front-end/DataGrid.js
	M	WebCore/inspector/front-end/ExtensionServer.js
	M	WebCore/inspector/front-end/NetworkPanel.js
	M	WebCore/inspector/front-end/Resource.js
	A	WebCore/inspector/front-end/ResourceManager.js
	M	WebCore/inspector/front-end/ResourcesPanel.js
	M	WebCore/inspector/front-end/SourceView.js
	M	WebCore/inspector/front-end/WebKit.qrc
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/networkPanel.css
	M	WebCore/loader/ResourceLoadNotifier.cpp
	M	WebCore/loader/appcache/ApplicationCacheGroup.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
Committed r69948