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

[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 102135: Patch, comments addressed
https://bugs.webkit.org/attachment.cgi?id=102135&action=review

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


> Source/WebCore/inspector/Inspector.json:467
> +		       { "name": "type", "type": "string", "enum": ["Parser",
"Script", "Other"], "description": "Type of this initiator." },

we tend to keep enum values lower-case

> Source/WebCore/inspector/Inspector.json:468
> +		       { "name": "stackTrace", "type": "array", "items": {
"$ref": "Console.CallFrame"}, "description": "Initiator JavaScript stack trace,
set for Script only." },

Isn't there a stack trace structure?

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

"optional": true for all of them.

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

willStartInitiation sounds weird. scheduleDeferredResourceLoad() ?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:392
> +    initiatorObject->setString("url", url);

You should either put top stack frame values here or not send them at all (does
not make much sense to send blank values here, they are optional).