[webkit-reviews] review granted: [Bug 47779] Web Inspector: Introduce InspectorResourceAgent.h/cpp and ResourceManager.js to fill network panel with data. : [Attachment 70974] [PATCH] Proposed change.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 18 02:48:18 PDT 2010


Yury Semikhatsky <yurys at chromium.org> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 47779: Web Inspector: Introduce InspectorResourceAgent.h/cpp and
ResourceManager.js to fill network panel with data.
https://bugs.webkit.org/show_bug.cgi?id=47779

Attachment 70974: [PATCH] Proposed change.
https://bugs.webkit.org/attachment.cgi?id=70974&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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?


More information about the webkit-reviews mailing list