[webkit-reviews] review denied: [Bug 65105] Web Inspector: Add resource initiator column to network panel. : [Attachment 101903] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 65105: Web Inspector: Add resource initiator column to network panel.
https://bugs.webkit.org/show_bug.cgi?id=65105

Attachment 101903: Patch
https://bugs.webkit.org/attachment.cgi?id=101903&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101903&action=review


> Source/WebCore/inspector/Inspector.json:466
> +		   "description": "Information about the cached resource.",

cached resource load initiator?

> Source/WebCore/inspector/Inspector.json:468
> +		       { "name": "type", "$ref": "InitiatorType",
"description": "Type of this initiator." },

You could inline enum definition here, no need for top-level class definition.

> Source/WebCore/inspector/Inspector.json:469
> +		       { "name": "url", "type": "string", "description":
"Initiator URL." },

Please use existing Location type.

> Source/WebCore/inspector/Inspector.json:608
>		       { "name": "resource", "$ref": "CachedResource",
"description": "Cached resource data." }

Drive-by comment: why does it miss stackTrace field?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:392
> +	   resourceAgent->willRecalculateStyle();

This sounds weird: theoretically, network agent should not care about styles.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:410
> +	   resourceAgent->didScheduleStyleRecalculation(document);

ditto

> Source/WebCore/inspector/InspectorResourceAgent.cpp:367
> +    m_scheduledStyleRecalculationInitiator = buildInitiatorObject(document);


You should either ASSERT or check for it being nullptr prior to assigning.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:379
> +	   lineNumber = stackTrace->at(0).lineNumber();

I'd suggest that you send the entire stack.