[webkit-reviews] review denied: [Bug 92108] Web Inspector: Resource agent's reference to cached resources should be weak, destroyed resources should be removed from front-end. : [Attachment 154051] 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 92108: Web Inspector: Resource agent's reference to cached resources should
be weak, destroyed resources should be removed from front-end.
https://bugs.webkit.org/show_bug.cgi?id=92108

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

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


This change looks like a hack both from the API and implementation standpoint.
Is there a way to implement it in a more elegant manner?

> Source/WebCore/inspector/Inspector.json:871
> +		       { "name": "resourceId", "$ref": "Page.ResourceId",
"description": "Identifier of the relevant resource." },

I don't think we should introduce resourceId concept. Why is requestId not
sufficient in the API?

> Source/WebCore/inspector/InspectorController.cpp:186
> +   
InspectorInstrumentation::inspectedPageDestroyed(m_instrumentingAgents.get());

This is very strange - InspectorController has pointers to agents explicitly as
well as to m_instrumentingAgents. Why does it use InspectorInstrumentation ?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:854
> +    if (!cachedResourceToAgentsMap ||
!cachedResourceToAgentsMap->contains(cachedResource))

InspectorInstrumentation is just a dispatch, it should not have logic like
this.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1167
> +void InspectorInstrumentation::inspectedPageDestroyed(InstrumentingAgents*
instrumentingAgents)

ditto

> Source/WebCore/inspector/InspectorPageAgent.cpp:960
> +	  
InspectorInstrumentation::addInstrumentingAgentsForCachedResource(cachedResourc
e, m_instrumentingAgents);

This looks like a hack.