[webkit-reviews] review denied: [Bug 204619] [GTK][WPE] Web Inspector: Add new tab for GStreamer pipelines inspection : [Attachment 385963] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 18 11:36:50 PST 2019
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 385963: Patch
https://bugs.webkit.org/attachment.cgi?id=385963&action=review
--- Comment #19 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 385963
--> https://bugs.webkit.org/attachment.cgi?id=385963
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
More information about the webkit-reviews
mailing list