[webkit-reviews] review denied: [Bug 204619] [GTK][WPE] Web Inspector: Add new tab for GStreamer pipelines inspection : [Attachment 384353] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 204619: [GTK][WPE] Web Inspector: Add new tab for GStreamer pipelines
inspection
https://bugs.webkit.org/show_bug.cgi?id=204619

Attachment 384353: Patch

https://bugs.webkit.org/attachment.cgi?id=384353&action=review




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 384353
  --> https://bugs.webkit.org/attachment.cgi?id=384353
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384353&action=review

r-, overall nice job on Web Inspector code :)

I think there was a lot of copy/paste of the frontend code that isn't correct. 
I'd also like to see this guarded so unnecessary resources aren't being loaded
for ports that won't use them.

> Source/JavaScriptCore/inspector/protocol/Page.json:153
> +	       "name": "getGStreamerPipelinesNames",

Instead of adding these to the `Page` domain (which doesn't really have
anything to do with media), can we create a `GStreamer` domain (and associated
agent) and have it be guarded by `USE(GSTREAMER)` (see 'IndexedDB.json' for an
example).  This way, non-GStreamer platforms don't have these commands and
don't have to worry about them (which would also make feature checking in the
frontend easier).

> Source/JavaScriptCore/inspector/protocol/Page.json:388
> +	       "parameters": []

No need to include this if there are no `parameters`.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1276
> +    if (InspectorPageAgent* inspectorPageAgent =
instrumentingAgents.inspectorPageAgent())

`auto`

> Source/WebCore/inspector/InspectorInstrumentation.h:316
> +    static void gstreamerPipelineCacheUpdated(Page*);

Can we wrap these in `USE(GSTREAMER)`?

> Source/WebCore/inspector/InspectorInstrumentation.h:1607
> +

Oops.

> Source/WebCore/inspector/InspectorInstrumentation.h:1618
> +    if (InstrumentingAgents* instrumentingAgents =
instrumentingAgentsForPage(page))

`auto`

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:1018
> +void InspectorPageAgent::dumpGStreamerPipeline(ErrorString& errorString,
const String& pipeline, const String& child, String*
pipelineGraphRepresentation)

This should be `const String*`.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:387
> +    static NeverDestroyed<HashMap<CString, GstBin*>> gstreamerPipelines;

Do you want to wrap accesses to this with a lock?  We've done similar things
like this in the past.	See `CanvasRenderingContext` for an example.

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:344
> +    # Combine the svg-pan-zoom.js JavaScript files in Production builds into
a single file (SvgPanZoom.js).

Is this necessary?  I'd like to avoid including this on non-GStreamer ports.

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:360
>> +	       '--output-script-name', 'Three.js');
> 
> Is this still used somewhere? It looks like you remove it above.

This is used by the Layers Tab.

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:362
> +    # Combine the viz.js JavaScript files in Production builds into a single
file (Viz.js).

Ditto

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:21
> +    // Public

We put this below the `constructor`.

> Source/WebInspectorUI/UserInterface/Images/GStreamer.svg:58
> +

Extra newlines?

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:46
> +

Unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:19
> +.sidebar > .panel.navigation.debugger > .content {

I think you mean `.gstreamer` instead of `.debugger`?

Most of these styles also seem wrong.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:23
> +	   super("gstreamer", WI.UIString("GStreamer"), true);

Do you really need `true` for
`shouldAutoPruneStaleTopLevelResourceTreeElements`?