[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