Bug 204619 – [GTK][WPE] Web Inspector: Add new tab for GStreamer pipelines inspection

• Component: Web Inspector
• Status: NEW

[Philippe Normand]

.

[Philippe Normand]

*** Bug 204273 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Created attachment 384353 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Charlie Turner]

Comment on attachment 384353 [details]
Patch

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

I don't have much to say, but this looks fantastic! :-)
I have built the inspector and it's definitely functional and useful.

> 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.

[Charlie Turner]

UX wise, there's a few obvious issues that I know you are aware of like the janky scrolling of the SVG view. I also noticed,

  - The "select:" widget for available subbins doesn't respond to click events comfortably. You have to hold the button to select a subbin, which is different to other parts of the inspector UI.
  - Would be nice to alphabetically sort the pipelines list on the left for easier navigation when there are lots of them.
  - The filter widget on the bottom left could be used to slim the list down, at the moment it doesn't do anything for me.
  - If I drill down two levels into a pipeline, I can no longer go back up, i.e. the DOT graph for the parent bins no longer loads. For example, if I go pipeline > playsink > abin, and then from abin if I select "Parent:" playsink does nothing. But if I select the "Select:" option (that also seems like it should not be in there), the form resets and I can go back up. There's a weird bug there somewhere :-)

[Devin Rousso]

Comment on attachment 384353 [details]
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`?

[Carlos Garcia Campos]

Comment on attachment 384353 [details]
Patch

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

I'm not sure about the use of Cache here, it's not actually a cache but the list of active pipelines, no? what about using gstreamerActivePipelinesChanged for example? and then rename other like registerActivePipeline, etc. Would it be possible to add tests for this?

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:83
> +    InspectorInstrumentation::gstreamerPipelineCacheUpdated(context().document()->page());

Can document() be nullptr?

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:97
> +    InspectorInstrumentation::gstreamerPipelineCacheUpdated(context().document()->page());

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:177
> +#if USE(GSTREAMER)
> +#include "GStreamerCommon.h"
> +#endif

What do you need this for?

> Source/WebCore/html/HTMLMediaElement.cpp:7221
> +void HTMLMediaElement::gstreamerPipelinesCacheUpdated()

gstreamerPipelineCacheUpdated for consistency with InspectorInstrumentation

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:385
> +HashMap<CString, GstBin*>& gstreamerPipelinesMap()

This should be static. It's private inside GStreamerCommon, so we can remove the gstreamer prefix and simply use pipelinesMap() or activePipelinesMap()

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:394
> +

We indeed need a lock if pipelines can be registered/unregistered from any thread, otherwise add isMainThread() asserts

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:407
> +    gstreamerPipelinesMap().take(name.get());

use remove since you are not getting the value

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:415
> +    for (auto name : map.keys())

auto&

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:425
> +        return CString();

return { }

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:429
> +        return CString(data.get());

You don't need the CString() I think.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:435
> +        return CString(data.get());

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:438
> +    return CString();

{ }

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:3072
> +        unregisterGStreamerPipeline(m_pipeline.get());

You need to notify the player here too, no?

[Philippe Normand]

Thanks for the reviews, I'll try to update the patch soon.

[Philippe Normand]

Comment on attachment 384353 [details]
Patch

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

>> 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*`.

Which argument? The last one? Doesn't seem to work. The virtual method doesn't use const String*.

>> 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.

I now use d3.js ... I don't know how to conditionally process these scripts though and I'd like to avoid to touch Perl code ;)

>>> 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.

It's not removed.

>> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:23
>> +        super("gstreamer", WI.UIString("GStreamer"), true);
> 
> Do you really need `true` for `shouldAutoPruneStaleTopLevelResourceTreeElements`?

Hum, I think so? That would enable clean-up of the tab eg when a <video> element disappears, right?

[Devin Rousso]

Comment on attachment 384353 [details]
Patch

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

>>> 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*`.
> 
> Which argument? The last one? Doesn't seem to work. The virtual method doesn't use const String*.

Ah, this is an out parameter.  My mistake.

>>> 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.
> 
> I now use d3.js ... I don't know how to conditionally process these scripts though and I'd like to avoid to touch Perl code ;)

Similar to the `COMBINE_INSPECTOR_RESOURCES` environment variable, I think maybe we should have something like `INCLUDE_GSTREAMER` that could be set on a platform-by-platform basis and would determine whether certain files are included in the built Main.html.

>>>> 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.
> 
> It's not removed.

I think this shows as added because you used 8 spaces instead of 4 for the indentation.  Please use 4 spaces of indent like the rest of this file.

>>> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:23
>>> +        super("gstreamer", WI.UIString("GStreamer"), true);
>> 
>> Do you really need `true` for `shouldAutoPruneStaleTopLevelResourceTreeElements`?
> 
> Hum, I think so? That would enable clean-up of the tab eg when a <video> element disappears, right?

Every `WI.NavigationSidebarPanel` has a `WI.TreeOutline` that is used to show the navigation hierarchy/tree for the content of that tab.  `shouldAutoPruneStaleTopLevelResourceTreeElements` indicates that if the immediate (first-level) children of that `WI.TreeOutline` are `instanceof WI.ResourceTreeElement`, remove the tree element if the `WI.Frame` of the related `WI.Resource` is no longer attached to the inspected page.  Seeing as how you're not using `WI.ResourceTreeElement`, passing this won't do anything.  It also has nothing to do with whether the tab is shown/hidden.

[Philippe Normand]

Created attachment 385887 [details]
Patch

[Philippe Normand]

Created attachment 385888 [details]
Patch

[Philippe Normand]

Created attachment 385889 [details]
Patch

[Philippe Normand]

GTK & WPE EWS would need Graphviz-dev libs installed...

[Carlos Garcia Campos]

Comment on attachment 385889 [details]
Patch

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

> Source/cmake/GStreamerChecks.cmake:13
> +    find_package(GraphViz 2.43 REQUIRED)

We can't add new deps, you need to make the feature conditional to the presence of the library.

[Philippe Normand]

Created attachment 385960 [details]
Patch

[Philippe Normand]

Created attachment 385961 [details]
Patch

[Philippe Normand]

Created attachment 385963 [details]
Patch

[Devin Rousso]

Comment on attachment 385963 [details]
Patch

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

Nice progress!  This is looking much better :)

There are still some style/layering issues, but I think those can mostly be solved with the comments I mentioned, as well as if you installed/used <https://eslint.org> in each of the files you added.

Could you please include some screenshots too?

> Source/JavaScriptCore/inspector/protocol/GStreamer.json:6
> +    "types": [],

You can drop this if it's empty

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:84
> +    auto* document = context().document();
> +    if (document)

Style: you can combine these:
```
    if (auto* document = context().document())
```

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:85
> +        InspectorInstrumentation::gstreamerActivePipelinesChanged(document->page());

I think it would be better to just pass the `Document*` to `InspectorInstrumentation::gstreamerActivePipelinesChanged` so that the `FAST_RETURN_IF_NO_FRONTENDS` can avoid the `document` pointer check and the `->page()` access.  You'd also want to benefit from the logic inside `InspectorInstrumentation::instrumentingAgentsForDocument` that deals with template documents.

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:100
> +    auto* document = context().document();
> +    if (document)

Ditto (83)

> Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp:101
> +        InspectorInstrumentation::gstreamerActivePipelinesChanged(document->page());

Ditto (85)

> Source/WebCore/html/HTMLMediaElement.cpp:7231
> +    InspectorInstrumentation::gstreamerActivePipelinesChanged(document().page());

Ditto (DefaultAudioDestinationNode.cpp:85), although you may need to get the address of `document()` or add an overload of `InspectorInstrumentation::gstreamerActivePipelinesChanged` for `Document&`.

> Source/WebCore/inspector/InspectorController.cpp:182
> +    m_agents.append(makeUnique<InspectorGStreamerAgent>(pageContext));

Unless this is needed by one of the other agents below, please put this at the end of the list (after `InspectorAnimationAgent` but before the `commandLineAPIHost` stuff).

> Source/WebCore/inspector/InspectorInstrumentation.h:488
> +#if USE(GSTREAMER)
> +    static void gstreamerActivePipelinesChangedImpl(InstrumentingAgents&);
> +#endif

Please match the relative positioning of the Impl with the non-Impl by moving this below `renderLayerDestroyedImpl`.

> Source/WebCore/inspector/InstrumentingAgents.h:204
> +    InspectorGStreamerAgent* m_inspectorGStreamerAgent { nullptr };

I know that many of the other agents follow this naming convention, but I'd like Web Inspector to move in a different direction for the sake of clarity:
 - "persistent*" for if the agent should be reachable so long as Web Inspector is connected
 - "enabled*" for if the agent should be reachable after that domain's `enable` command is invoked (assuming the corresponding `disable` has not been called)
 - "tracking*" for if the agent should be reachable during a timeline recording (similar to "enabled*" but instead gated by that domain's `startTracking` and `stopTracking` commands)
This way, callers can reason about the state of the agent at the callsite.  Take a look at the way `inspectorTimelineAgent` (which should be `enabledInspectorTimelineAgent`) is used vs `trackingInspectorTimelineAgent` for an example.

As such, please name this `m_persistentInspectorGStreamerAgent`.

> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:51
> +    UNUSED_PARAM(errorString);

If you're not using the `errorString`, you can just omit the parameter name:
```
    void InspectorGStreamerAgent::getActivePipelinesNames(ErrorString&, RefPtr<JSON::ArrayOf<String>>& pipelines)
```

> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:59
> +    UNUSED_PARAM(errorString);

Ditto (51)

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:40
> +    get agent() {

This should be done at the callsite, not in the manager.  The manager is effectively a singleton for the purposes of bridging controls between the single UI and the potentially multiple targets it's connected to.  We should not have the manager assume that there's only a single target.

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:50
> +        WI.FileUtilities.save(object.saveData);

Please do this at the callsite.  The manager shouldn't be concerned with IO or anything like that.  If you must do it here, at least add an assertion that the `object` is an expected type.

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:53
> +    renderSVGElement(graphRepresentation)

This feels like a layering violation, in that it generates view content (a DOM node) in the controller "layer".  Can you move this outside the manager?  Perhaps have this as a static method on `WI.GStreamerView`?

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:62
> +        this.dispatchEventToListeners(WI.GStreamerManager.Event.ActivePipelinesChanged, {});

If there is no data to send with the event, there's no need to include the `{}`.

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:35
> +    get svgElement() { return this._svgDocument.getElementsByTagName("svg")[0]; }

Style: we only use single-line getters if the body is simple (e.g. return a variable with the same (or similar) name as the getter name), so please put this on separate lines.

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:39
> +        let name = this._name;

You can inline this in the template string.

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:40
> +        let filename = WI.unlocalizedString(`GStreamer-pipeline-dump-${name}.dot`);

You can now use `suggestedName` instead of `filename`.

> Source/WebInspectorUI/UserInterface/Protocol/GStreamerObserver.js:27
> +

Style: extra newline

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:88
> +    display: block;

CSS rules that have the same properties should be merged together with a comma-separated list of CSS selectors.

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

You can omit falsy values in function calls if they are the trailing argument.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:76
> +            this.contentTreeOutline.removeChildren(true);

We prefer creating a `const` variable when including non-obvious constants as arguments to function calls.  That way, it's more understandable at the callsite and doesn't require the reader to have to find and read the function's definition.
```
    const suppressOnDeselect = true;
    this.contentTreeOutline.removeChildren(suppressOnDeselect);
```

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:78
> +                let elt = new WI.TreeElement(name, null);

Ditto (23)

We don't use abbreviated names like this for variable names.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:97
> +        target.GStreamerAgent.dumpActivePipeline(pipelineName, "").then((payload) => {

Ditto (76)

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:99
> +            let representedObject = new WI.GStreamerPipeline(pipelineName, "", "", payload.pipelineGraphRepresentation, documentFragment);

Ditto (76)

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.css:28
> +    background-color: white;

Do you care about supporting dark mode (assuming it exists on GTK/WPE)?

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.css:33
> +.content-view.gstreamer .navigation-bar.invisible {
> +    visibility: hidden;
> +}

Why is this needed?

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:24
> +        let detailsSidebarPanelConstructors = [];

This can be omitted.

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:27
> +              detailsSidebarPanelConstructors);

Style: we don't do this kind of wrapping in Web Inspector

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:42
> +        return WI.Platform.port === "gtk" || WI.Platform.port === "wpe";

We should also only allow this if `InspectorBackend.hasDomain("GStreamer")`.

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:56
> +        return representedObject instanceof WI.GStreamerPipeline;

You'll also want to add something like this to `WI.tabContentViewClassForRepresentedObject` so that if the user closes the GStreamer Tab, we know how to reopen it.

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:66
> +    initialLayout()

Please include a `super.initialLayout()` call as well.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:36
> +        this._exportButtonNavigationItem.tooltip = WI.UIString("Export pipeline dot representation (%s)").format(WI.saveKeyboardShortcut.displayName);

This probably needs a comment explaining what "pipeline dot" means.  `WI.UIString` accepts additional arguments so that the comment you specify is included in the 'localizedStrings.js' file for localizers.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:39
> +        this._exportButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, this._handleExportButtonNavigationItemClicked.bind(this));

When using Web Inspector's event listener system, you don't (and shouldn't) need to `bind` the function.  Instead, you should provide the `thisObject` as a third argument:
```
    this._exportButtonNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, this._handleExportButtonNavigationItemClicked, this);
```

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:47
> +        this._containerElement.id = "pipeline-container";

Please avoid using id selectors unless you're using them with a `for` attribute.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:72
> +            this._scopeBar.removeEventListener(WI.ScopeBar.Event.SelectionChanged, this._scopeBarSelectionDidChange);

You'll also need to specify the `thisObject` if it was specified in `addEventListener`.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:76
> +        if (this._label) {

Style: we don't add braces to single-line control flow

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:78
> +        }

Style: please add a newline after

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:79
> +        var name = gstreamerPipeline.binName || gstreamerPipeline.name;

Please use `let`

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:80
> +        this._label = new WI.TextNavigationItem("gst-pipeline-bin-name", "Displaying " + name + ". Available sub-bins: ");

Should this be localized?

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:88
> +            var scopeBarItems = [];

Ditto (79)

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:89
> +            var defaultItem = new WI.ScopeBarItem("<placeholder>", "Select...");

Ditto (79)

Also, please make the identifier name more specific like `gst-pipeline-scope-bar-placeholder`, as we keep `WI.Setting` objects per scope bar item to preserve the selected state.  Also, there's no reason to include "<" and ">" with a better naming scheme.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:90
> +            defaultItem.__parent = "";

Adding an expando property that is inherently falsy is discouraged.  It's preferable to have a fallback `|| ""` at the callsite instead.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:94
> +            var scopeBarItem = new WI.ScopeBarItem(gstreamerPipeline.parentBinName, "Parent: " + gstreamerPipeline.parentBinName);

Ditto (79)

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:96
> +            scopeBarItem.__parent = "";

Ditto (90)

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:99
> +            for (var binName of payload.binNames) {

Ditto (79)

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:100
> +                var scopeBarItem = new WI.ScopeBarItem(binName, binName);

Ditto (79)

Also, this name shadows the name from outside this scope, so please rename it (or the outer one).

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:117
> +        var svg = d3.select("#pipeline-container"),

Ditto (79)

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:118
> +            g = svg.select("g");

Style: we don't use comma initialization for variables, and instead include a `let` for each variable

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:121
> +            g.attr("transform", d3.event.transform)

Style: missing semicolon

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:129
> +        for (var it of event.target.items) {

Ditto (79)

Also, please use a more descriptive name than `it`.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:130
> +            if ((it.id !== "<placeholder>") && it.selected) {

Style: extra parenthesis are not needed

[Philippe Normand]

Comment on attachment 385963 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:88
>> +    display: block;
> 
> CSS rules that have the same properties should be merged together with a comma-separated list of CSS selectors.

I think I've copied that from an existing stylesheet. I'm sorry I don't understand what you mean though, I'm generally not very fluent in CSS :)

>> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.css:28
>> +    background-color: white;
> 
> Do you care about supporting dark mode (assuming it exists on GTK/WPE)?

In GTK yes, but not in WPE.

>> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:47
>> +        this._containerElement.id = "pipeline-container";
> 
> Please avoid using id selectors unless you're using them with a `for` attribute.

I don't understand what you mean here with `for` attribute.

[Philippe Normand]

Created attachment 393471 [details]
screenshot

[Philippe Normand]

Created attachment 393472 [details]
Patch

[Philippe Normand]

(In reply to Philippe Normand from comment #21)
> Created attachment 393471 [details]
> screenshot

As the SVG can potentially be huge, the user can zoom (alt + mouse wheel) and pan with the mouse as well.

[Konstantin Tokarev]

Comment on attachment 393472 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:450
> +    if (gvRenderData(context, graph, "svg"_s, &svgData, &length)) {

Linking to graphviz libraries makes them yet another run-time dependency which must be distributed alongside with WebKit. Wouldn't it be a better idea to invoke external "dot" program and read SVG from its stdout?

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:465
> +    return "<svg height=\"30\" width=\"200\"><text x=\"0\" y=\"15\" fill=\"red\">Pipeline cannot be diplayed. Please install Graphviz along with its development headers and recompile WebKit. ¯\\_(ã)_/¯</text></svg>";

This could sound like "install dot and try again", no recompilation or even WebKit restart.

[Philippe Normand]

Comment on attachment 393472 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:450
>> +    if (gvRenderData(context, graph, "svg"_s, &svgData, &length)) {
> 
> Linking to graphviz libraries makes them yet another run-time dependency which must be distributed alongside with WebKit. Wouldn't it be a better idea to invoke external "dot" program and read SVG from its stdout?

I'm not a big fan of spawning an external process if it can be avoided. Could also be an issue if the WebProcess is sandboxed? I don't know. No strong opposition though, I defer that to the reviewers.

[Adrian Perez]

(In reply to Philippe Normand from comment #25)
> Comment on attachment 393472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393472&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:450
> >> +    if (gvRenderData(context, graph, "svg"_s, &svgData, &length)) {
> > 
> > Linking to graphviz libraries makes them yet another run-time dependency which must be distributed alongside with WebKit. Wouldn't it be a better idea to invoke external "dot" program and read SVG from its stdout?
> 
> I'm not a big fan of spawning an external process if it can be avoided.
> Could also be an issue if the WebProcess is sandboxed? I don't know. No
> strong opposition though, I defer that to the reviewers.

Then the “dot” program will need to be available inside the sandbox. In
general I would prefer to avoid spawning an external process, but I can
empathize for not wanting yet another runtime dependency linked in...

How about building a small loadable shared library which links against
Graphviz and exports a convertGStreamerBinToSVG() function, and then
make exportBinToSVG() load that using dlopen()? If loading the library
fails at runtime, the exportBinToSVG() would also return the SVG which
tells that Graphviz is not available. This way we would not need the
WebKit library to link to Graphviz, only the loadable .so would, and
distributors can choose to build the support at build time, but provide
the .so in a separate package without needing to make their main WebKit
package depend on Graphviz 🤔
dependency would be required at build

[Konstantin Tokarev]

Comment on attachment 393472 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:450
>>>> +    if (gvRenderData(context, graph, "svg"_s, &svgData, &length)) {
>>> 
>>> Linking to graphviz libraries makes them yet another run-time dependency which must be distributed alongside with WebKit. Wouldn't it be a better idea to invoke external "dot" program and read SVG from its stdout?
>> 
>> I'm not a big fan of spawning an external process if it can be avoided. Could also be an issue if the WebProcess is sandboxed? I don't know. No strong opposition though, I defer that to the reviewers.
> 
> Then the “dot” program will need to be available inside the sandbox. In
> general I would prefer to avoid spawning an external process, but I can
> empathize for not wanting yet another runtime dependency linked in...
> 
> How about building a small loadable shared library which links against
> Graphviz and exports a convertGStreamerBinToSVG() function, and then
> make exportBinToSVG() load that using dlopen()? If loading the library
> fails at runtime, the exportBinToSVG() would also return the SVG which
> tells that Graphviz is not available. This way we would not need the
> WebKit library to link to Graphviz, only the loadable .so would, and
> distributors can choose to build the support at build time, but provide
> the .so in a separate package without needing to make their main WebKit
> package depend on Graphviz 🤔
> dependency would be required at build

This sounds more complicated then just making dot available in sandbox

[Carlos Garcia Campos]

I don't see any problem with the dependency as long as it's optional.

[Konstantin Tokarev]

What about license? AFAIK Graphviz is not GPL-compatible.

[Carlos Alberto Lopez Perez]

(In reply to Konstantin Tokarev from comment #24)
> Comment on attachment 393472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393472&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:450
> > +    if (gvRenderData(context, graph, "svg"_s, &svgData, &length)) {
> 
> Linking to graphviz libraries makes them yet another run-time dependency
> which must be distributed alongside with WebKit. Wouldn't it be a better
> idea to invoke external "dot" program and read SVG from its stdout?

In the end you have to also distribute graphviz one way or the other.

But if you call dot externally you have to also distribute the dot program on top of graphviz.

[Konstantin Tokarev]

>But if you call dot externally you have to also distribute the dot program on top of graphviz.

The don't have to be distributed together, if user installs graphviz and GPL program starts calling it, it doesn't violate GPL. AFAIK Linux distros with binary packages are fine with such loose coupling, but they will likely disable graphviz if it's linked to WebKit and will have to be installed as a dependency.

[Konstantin Tokarev]

And if we speak about WebKit-using software distributed directly, it also can invoke dot installed from system packages without any troubles.

[Carlos Alberto Lopez Perez]

(In reply to Konstantin Tokarev from comment #31)
> >But if you call dot externally you have to also distribute the dot program on top of graphviz.
> 
> The don't have to be distributed together, if user installs graphviz and GPL
> program starts calling it, it doesn't violate GPL. AFAIK Linux distros with
> binary packages are fine with such loose coupling, but they will likely
> disable graphviz if it's linked to WebKit and will have to be installed as a
> dependency.

Yeah right. 

Graphviz is EPL-1.0, which its not GPL compatible according to https://en.wikipedia.org/wiki/Eclipse_Public_License


So it looks the best way forward is calling the dot program externally.

If this program its not available (not installed, etc) then this can show (instead of the graph) a warning asking the user to install it

[Philippe Normand]

Devin, can you review the Inspector bits please?

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #33)
> (In reply to Konstantin Tokarev from comment #31)
> > >But if you call dot externally you have to also distribute the dot program on top of graphviz.
> > 
> > The don't have to be distributed together, if user installs graphviz and GPL
> > program starts calling it, it doesn't violate GPL. AFAIK Linux distros with
> > binary packages are fine with such loose coupling, but they will likely
> > disable graphviz if it's linked to WebKit and will have to be installed as a
> > dependency.
> 
> Yeah right. 
> 
> Graphviz is EPL-1.0, which its not GPL compatible according to
> https://en.wikipedia.org/wiki/Eclipse_Public_License
> 
> 
> So it looks the best way forward is calling the dot program externally.
> 
> If this program its not available (not installed, etc) then this can show
> (instead of the graph) a warning asking the user to install it

Awesome, so I need to make this asynchronous too now...

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #35)
> (In reply to Carlos Alberto Lopez Perez from comment #33)
> > (In reply to Konstantin Tokarev from comment #31)
> > > >But if you call dot externally you have to also distribute the dot program on top of graphviz.
> > > 
> > > The don't have to be distributed together, if user installs graphviz and GPL
> > > program starts calling it, it doesn't violate GPL. AFAIK Linux distros with
> > > binary packages are fine with such loose coupling, but they will likely
> > > disable graphviz if it's linked to WebKit and will have to be installed as a
> > > dependency.
> > 
> > Yeah right. 
> > 
> > Graphviz is EPL-1.0, which its not GPL compatible according to
> > https://en.wikipedia.org/wiki/Eclipse_Public_License
> > 
> > 
> > So it looks the best way forward is calling the dot program externally.
> > 
> > If this program its not available (not installed, etc) then this can show
> > (instead of the graph) a warning asking the user to install it
> 
> Awesome, so I need to make this asynchronous too now...


Or you can wait for the command dot to finish processing and give you the image file. How is this any different than waiting for the graphviz call? (likely its a bit slower because of the overhead of calling an external program, but not fundamentally different)

[Philippe Normand]

Of course I can wait, but as the inspector agent can execute tasks asynchronously, in theory it should be possible to chain this down to the `dot` program execution. Is it worth?

[Philippe Normand]

Created attachment 394093 [details]
Patch

[Devin Rousso]

Comment on attachment 394093 [details]
Patch

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

Is the GStreamer information you're exposing in Web Inspector aimed at WebKit/GStreamer engineers, or the wider web developer audience?  If the former, I'd ask that you add a setting so that this isn't shown to web developers, possibly confusing them as to what they're looking at (you could even make it an engineering setting, if this UI is not something you'd ever want to ship to customers, meaning that it would only be accessible in local builds).

> Source/JavaScriptCore/inspector/protocol/GStreamer.json:11
> +                { "name": "pipelines", "type": "array", "items": { "$ref": "string"}, "description": "Array of pipeline names." }

This should just be `"type": "string"`.

NIT: there should be a space after the `"string"` and before the `}`

> Source/JavaScriptCore/inspector/protocol/GStreamer.json:22
> +                { "name": "binNames", "type": "array", "items": { "$ref": "string"}, "description": "Array of bin names." }

Ditto (11)

> Source/JavaScriptCore/inspector/protocol/GStreamer.json:41
> +            "description": "Fired when the internal registry of active GStreamer pipelines was updated, eg when a new pipeline was registered or when a pipeline was removed from the registry."

NIT: `e.g.`

> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:58
> +    for (auto item : activePipelinesNames())

Should you use `auto&`?

> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:65
> +    for (auto item : getBinChildren(pipeline, child))

Ditto (58)

> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:74
> +    if (m_instrumentingAgents.persistentInspectorGStreamerAgent() != this) {
> +        dumpCallback->sendFailure("GStreamer domain must be enabled"_s);
> +        return;
> +    }

This shouldn't be possible/necessary, because making this agent "persistent" means that the frontend should _always_ be able to invoke any `GStreamer` commands (this is identified by the fact that there are no `GStreamer.enable` or `GStreamer.disable` commands).

If you don't want this (which is usually a better approach), please add `GStreamer.enable` and `GStreamer.disable` commands to 'GStreamer.json' which set and unset the `m_instrumentingAgents.enabledInspectorGStreamerAgent`.  The main reason that you may want to do this is so that the frontend isn't notified about `GStreamer.activePipelinesChanged` when the GStreamer Tab is not open (see `WI.GraphicsTabContentView` or `WI.StorageTabContentView` or `WI.TimelineTabContentView` for examples of what I mean).

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:397
> +static HashMap<CString, GstBin*>& activePipelinesMap()

Is there any need to worry about multiple threads?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:178
> +localizedStrings["Automatically refresh the pipeline"] = "Automatically refresh the pipeline";

Please include localization comments for each of these strings, especially for the more generic ones (e.g. "Pipelines").  Comments can be added by calling `WI.UIString` with additional arguments: `WI.UIString(string, key, comment)` where `string` is the text to be displayed/localized, `key` is the identifier in the `localizedStrings` object in this file, and `comment` is a comment that gets added on the line before in this file.  Our recent style has been to make the `key` somewhat descriptive of the location that this string will be seen (e.g. `"Automatically refresh the pipeline @ Navigation Item in GStreamer Pipeline View"`) and `comment` would describe that and what it does (e.g. `"Text toggle navigation item in GStreamer views that controls whether the shown pipeline is automatically refreshed on an interval"`).

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:356
> +        system($perl, $combineResourcesCmd,

Would it be possible to move _all_ of the GStreamer related files to be behind this flag?  I'd prefer to avoid shipping code that will never be used :(

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:434
> +        seedFile($targetPanZoomJS, "");

Why is this needed?

> Source/WebInspectorUI/UserInterface/Base/Main.js:504
> +    if (WI.Platform.port === "gtk" || WI.Platform.port === "wpe") {
> +        productionTabClasses.push(WI.GStreamerTabContentView);
> +    }

Style: we omit the `{ }` when the body of the `if` is only one line

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:4
> + *  This library is free software; you can redistribute it and/or

This is a different license than what Web Inspector ends up shipping with.  See 'copy-user-interface-resource.pl'.

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:25
> +        this._enabled = WI.Platform.port === "gtk" || WI.Platform.port === "wpe";

This is never used.

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:30
> +    get domains() { return ["GStreamer"]; }

Style: we usually only put "redundant" getters on one line (e.g. `get element() { return this._element; }`)

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:32
> +    activateExtraDomain(domain)

This shouldn't be necessary, as this path is only used by TVML, which doesn't use GStreamer :)

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:43
> +

Style: extra newline

> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:47
> +    ActivePipelinesChanged: "gstreamer-manager-active-pipelines-were-changed",

NIT: drop the `were`

> Source/WebInspectorUI/UserInterface/Images/GStreamer.svg:5
> +   xmlns:dc="http://purl.org/dc/elements/1.1/"

This is a LOT of extra/unnecessary attributes.  Can we clean this up a bit?

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:35
> +    get binName() { return this._binName; }
> +    get svgElement()

Style: missing newline

> Source/WebInspectorUI/UserInterface/Models/GStreamerPipeline.js:44
> +            url: WI.FileUtilities.inspectorURLForFilename(suggestedName),

Rather than provide a `url`, you can just use the `suggestedName` in the returned object.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:34
> +}
> +.sidebar > .panel.navigation.gstreamer > .navigation-bar .gstreamer-breakpoints.activated {

Style: missing newline

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:43
> +}
> +.sidebar > .panel.navigation.gstreamer > .navigation-bar .gstreamer-pause-resume.activated {

Ditto (33)

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:58
> +.sidebar > .panel.navigation.gstreamer .tree-outline.single-thread > .item.thread {

Where does this come from?  It seems like you just copy/paste part of 'SourcesNavigationSidebarPanel.css' :(

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:63
> +    --tree-outline-single-thread-margin-start: -10px;

You can probably just use `-webkit-margin-start` instead of a variable and two `body[dir=ltr]` and `body[dir=rtl]` CSS rules.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:19
> +WI.GStreamerSidebarPanel = class GStreamerSidebarPanel extends WI.NavigationSidebarPanel

NIT: I've been trying to shift us such that subclasses of `WI.NavigationSidebarPanel` also have "Navigation" in the name, so please rename this class/file to `WI.GStreamerNavigationSidebarPanel`.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:25
> +        this.contentTreeOutline.addEventListener(WI.TreeOutline.Event.SelectionDidChange, this._treeSelectionDidChange, this);

NIT: I usually try to have any event listener callbacks have `_handle` as the prefix (`this._handleTreeSelectionDidChange`) so that it's immediately obvious what it's for

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:27
> +        WI.gstreamerManager.addEventListener(WI.GStreamerManager.Event.ActivePipelinesChanged, this._refreshPipelinesList, this);

These event listeners should be moved to `initialLayout()`, as there's no reason to listen for tree selection changes or the event to update the tree itself when we haven't even shown the tree yet.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:51
> +    matchTreeElementAgainstCustomFilters(treeElement, flags)

Why is this needed?  The default behavior of the filter in any `WI.NavigationSidebarPanel` is to do a `simpleGlobStringToRegExp` search on every `WI.TreeElement`, so this code is doing basically the same thing again as the default.  Is our default implementation missing some edge-case?

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:73
> +        if (target.hasDomain("GStreamer"))

It shouldn't be possible to reach this point unless `InspectorBackend.hasDomain("GStreamer")`.  I realize you're testing against the specific target instead of the debuggable,  but for things like tabs and sidebar panels, there only exists one for the entirety of Web Inspector, so they aren't tied to a specific target.  Furthermore, you're testing the main target in this code, which _is_ the debuggable, meaning that it will have the exact same result as `InspectorBackend.hasDomain("GStreamer")` as that's basically where it's result is derived from.

```js
    let target = WI.assumingMainTarget();
    target.GStreamerAgent.getActivePipelineNames((error, pipelines) => {
        if (error) {
            WI.reportInternalError(error);
            return;
        }

        this.contentTreeOutline.removeChildren();
        
        for (let name of pipelines) {
            const className = "gstreamer-pipeline";
            this.contentTreeOutline.appendChild(new WI.GeneralTreeElement(className, name));
        }
    });
```

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:93
> +        if (!this.selected)
> +            return;

This shouldn't be necessary because there is only ever one `WI.NavigationSidebarPanel`, so it will always be selected by default.

> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.js:101
> +        let child = "";

Why not make `child` optional in the protocol?

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:26
> +        });
> +        this._gstreamerView = null;

Style: missing newline

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:45
> +    get type() { return WI.GStreamerTabContentView.Type; }

Style: we usually only put "redundant" getters on one line (e.g. `get element() { return this._element; }`)

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:59
> +        this._gstreamerView.displayPipeline(representedObject);

By doing this, you're skipping over all of the mechanisms already in place for doing model<->view linking.  Normally, we attach a single `WI.ContentView` to any given `representedObject` and reuse that `WI.ContentView` whenever attempting to show that `WI.ContentView`.  This is one of the ways that we keep UI/DOM code out of our models, as we can effectively preserve UI/DOM related values in the view, which is kept alive so long as the model exists.  In your case specifically, this could be used to eliminate `WI.GStreamerView.renderSVGElement` and instead have that logic be added to `WI.GStreamerPipelineView.prototype.initialLayout` (which you'd want to rename), which makes `WI.GStreamerPipeline` a much "cleaner" object.  Furthermore, you could also remove some of the logic inside `WI.GStreamerNavigationSidebarPanel` if you link the `WI.GStreamerPipeline` model object to the `WI.TreeElement` you create for it's name, as we default to showing any associated `WI.ContentView` for the `representedObject` of the newly selected `WI.TreeElement`.

> Source/WebInspectorUI/UserInterface/Views/GStreamerTabContentView.js:67
> +        super.initialLayout();
> +        this._gstreamerView = new WI.GStreamerView("general", WI.UIString("General"));

Style: missing newline

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:21
> +    constructor(identifier, displayName)

Why are these two arguments needed?  They don't appear to be used anywhere.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:40
> +        this._exportButtonNavigationItem.tooltip = WI.UIString("Export pipeline representation to Graphviz dot file (%s)", "The Graphviz dot file format can then be converted to PNG and other picture formats with third-party tools.").format(WI.saveKeyboardShortcut.displayName);

Are you expecting that using two arguments will add a newline?  If so, that is unfortunately not how `WI.UIString` works 😅

Please see my comment on 'localizedStrings.js:178'.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:98
> +            this._label = new WI.TextNavigationItem("gst-pipeline-bin-name", WI.UIString("Displaying %s.", "").format(name));

It really seems to me like this should really be in the `WI.TreeOutline` in the `WI.GStreamerNavigationSidebarPanel`.  That's Web Inspector's normal pattern.  Typically, our flow is to have the navigation sidebar be used for choosing what is shown in the main content area, and then use the details sidebar to show additional information (usually related or "meta" data) about what is selected.

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:121
> +                innerScopeBarItem.__parent = gstreamerPipeline.binName;

We really prefer avoiding using expando properties like this, as it's harder to track where things are being used and can possibly cause naming collisions in the future if a different class (or `WI.ScopeBarItem` itself) does something similar.  Perhaps use a `Map` on `this`?

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:135
> +    static renderSVGElement(graphRepresentation)

Style: please put any `static` functions in a `// Static` section above the `// Public` section

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:174
> +    async _displayBin(binName, parentBinName)

Why is this marked as `async` if you don't ever use `await` or return a `Promise`?

> Source/WebInspectorUI/UserInterface/Views/GStreamerView.js:178
> +        if (target.hasDomain("GStreamer"))

Ditto (GStreamerSidebarPanel.js:73)

[Philippe Normand]

Comment on attachment 394093 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorGStreamerAgent.cpp:74
>> +    }
> 
> This shouldn't be possible/necessary, because making this agent "persistent" means that the frontend should _always_ be able to invoke any `GStreamer` commands (this is identified by the fact that there are no `GStreamer.enable` or `GStreamer.disable` commands).
> 
> If you don't want this (which is usually a better approach), please add `GStreamer.enable` and `GStreamer.disable` commands to 'GStreamer.json' which set and unset the `m_instrumentingAgents.enabledInspectorGStreamerAgent`.  The main reason that you may want to do this is so that the frontend isn't notified about `GStreamer.activePipelinesChanged` when the GStreamer Tab is not open (see `WI.GraphicsTabContentView` or `WI.StorageTabContentView` or `WI.TimelineTabContentView` for examples of what I mean).

OK I think this agent shouldn't be persistent.

>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:397
>> +static HashMap<CString, GstBin*>& activePipelinesMap()
> 
> Is there any need to worry about multiple threads?

I don't think so. Pipelines should be always disposed from the WebProcess main thread, but I'll check and possibly add an ASSERT.

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:356
>> +        system($perl, $combineResourcesCmd,
> 
> Would it be possible to move _all_ of the GStreamer related files to be behind this flag?  I'd prefer to avoid shipping code that will never be used :(

There's already an environment variable, isn't that enough?

>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:434
>> +        seedFile($targetPanZoomJS, "");
> 
> Why is this needed?

For no reason ;)

>> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:4
>> + *  This library is free software; you can redistribute it and/or
> 
> This is a different license than what Web Inspector ends up shipping with.  See 'copy-user-interface-resource.pl'.

Is it a requirement that the WebInspector has to use a single license?

>> Source/WebInspectorUI/UserInterface/Views/GStreamerSidebarPanel.css:58
>> +.sidebar > .panel.navigation.gstreamer .tree-outline.single-thread > .item.thread {
> 
> Where does this come from?  It seems like you just copy/paste part of 'SourcesNavigationSidebarPanel.css' :(

I remember copying another stylesheet indeed... I'll try to clean this up a bit.

[Philippe Normand]

(In reply to Devin Rousso from comment #39)
> Comment on attachment 394093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394093&action=review
> 
> Is the GStreamer information you're exposing in Web Inspector aimed at
> WebKit/GStreamer engineers, or the wider web developer audience?  If the
> former, I'd ask that you add a setting so that this isn't shown to web
> developers, possibly confusing them as to what they're looking at (you could
> even make it an engineering setting, if this UI is not something you'd ever
> want to ship to customers, meaning that it would only be accessible in local
> builds).
> 

The audience wouldn't be web developers indeed, but rather multimedia engineers. What are engineering builds BTW? This seems to be an XCode concept?

[Konstantin Tokarev]

(In reply to Philippe Normand from comment #41)
> The audience wouldn't be web developers indeed, but rather multimedia
> engineers.

Also end users, trying to fix their multimedia issues

[Devin Rousso]

Comment on attachment 394093 [details]
Patch

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

(In reply to Konstantin Tokarev from comment #42)
> (In reply to Philippe Normand from comment #41)
> > (In reply to Devin Rousso from comment #39)
> > > Is the GStreamer information you're exposing in Web Inspector aimed at WebKit/GStreamer engineers, or the wider web developer audience?  If the former, I'd ask that you add a setting so that this isn't shown to web developers, possibly confusing them as to what they're looking at (you could even make it an engineering setting, if this UI is not something you'd ever want to ship to customers, meaning that it would only be accessible in local builds)
> > The audience wouldn't be web developers indeed, but rather multimedia engineers.
> Also end users, trying to fix their multimedia issues
So it seems that although this isn't necessarily geared at web developers, it is something that may be used by non-WebKit engineers in a shipping browser (i.e. this is not just something for the benefit of GTK/WPE WebKit maintainers).  Is that correct?  If so, this should _not_ be behind a setting, but instead just be gated on the existence of the `GStreamer` domain (which is gated by `USE(GSTREAMER)`).

(In reply to Philippe Normand from comment #41)
> What are engineering builds BTW? This seems to be an XCode concept?
I understood the term "engineering build" to mean a build done by an engineer (i.e. a local build from having downloaded and built the source not intended for shipping/production).  Web Inspector already has some "engineering" settings that are intended to assist WebKit engineers in the process of building new features or debugging existing ones, but are not intended to be released with a shipping browser (e.g. Safari Technology Preview).

>>> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:356
>>> +        system($perl, $combineResourcesCmd,
>> 
>> Would it be possible to move _all_ of the GStreamer related files to be behind this flag?  I'd prefer to avoid shipping code that will never be used :(
> 
> There's already an environment variable, isn't that enough?

Sorry, I wasn't clear.  What you have right now is great, but I'm wondering if we could take it a step further by making it so that _all_ of the new files included in 'WebInspectorUI/UserInterface/Main.html' could be guarded by this environment variable, not just 'PanZoom.js'.  I think this is the first time that Web Inspector will add something port specific (i.e. unlikely to be used by any other port).  Basically, I'm trying to avoid having extra unnecessary/unused code be shipped on ports that don't use GStreamer.  This would likely involve having to have more checks in the frontend as to whether certain types existing (e.g. checking that `WI.GStreamerManager` exists before using it), but I think that that is an OK tradeoff to make in order to avoid having to ship the entire source file that is guaranteed to not be used.

>>> Source/WebInspectorUI/UserInterface/Controllers/GStreamerManager.js:4
>>> + *  This library is free software; you can redistribute it and/or
>> 
>> This is a different license than what Web Inspector ends up shipping with.  See 'copy-user-interface-resource.pl'.
> 
> Is it a requirement that the WebInspector has to use a single license?

I don't know what the rules are for this 😅

I'd suggest asking in #WebKit #dev or sending an email to <[e-mail]>.

[Blaze Burg]

Comment on attachment 394093 [details]
Patch

Clearing r? from stale Web Inspector patches. Please address outstanding feedback and send a new patch :)