1315208 - Remove explicit call to `ReactDOM.render` in the LayoutView

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Original comment https://bugzilla.mozilla.org/show_bug.cgi?id=1314931#c5

Rendering of the panel content should be responsibility of the Sidebar component (it should render its children automatically). Note that in regular React app, there is just one `ReactDOM.render` call - the one that renders the root component and we should go towards it.

So, what about rename the layout.js to main.js and export just a single function. This function would create the root component and append it directly into the sidebar.

exports.addSidebarTab = (inspector, defaultTab) => {
  let store = Store();
  let provider = createElement(Provider, {
    store,
    id: "layoutview",
    title: INSPECTOR_L10N.getStr("inspector.sidebar.layoutViewTitle"),
    key: "layoutview",
  }, App());

  inspector.addSidebarTab(
    null,
    null,
    provider,
    defaultTab == "layoutview
  );
}

The inspector.js would call that function as follows:

if (Services.prefs.getBoolPref("devtools.layoutview.enabled")) {
  const {addSidebarTab} = this.browserRequire("devtools/client/inspector/layout/main");
  addSidebarTab(this, defaultTab);
}

Comments:
- I am not sure if 'addSidebarTab' is the best name for the function, you might find better name (e.g. registerLayoutPanel, etc.)
- You can remove the helper node in inspector.xul
- In case the `addSidebarTab` is actually part of inspector.js (a method of Inspector object) you shouldn't need to modify the BROWSER_BASED_DIRS and you don't need the main.js at all.
- Finally, we might want to improve the API and avoid the two 'null' arguments. At least we could introduce the following method in Inspector object:

addSidebarComponent: function (component, selected) {
  this.sidebar.addTab(undefined, undefined, component, selected);
}

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1315208.patch [1.0]

So, this looks different from the function you wanted to export because we still needed to return an object (LayoutView) so we can take care of destroying the view. We also require LayoutView to hold onto some of its internal variables (this.inspector) that would later be used.

One curious thing that I ran into was that addExistingTab creates an InspectorTabPanel, which doesn't quite make sense since these panels already have existing markup in inspector.xhtml that takes care of a tab panel. I don't think creating this InspectorTabPanel is necessary or otherwise we should be getting ride of the tab panel that exist in the markup.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Also, we need to provide id, title to addSidebarTab or selecting the default tab will not work.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8809285 [details] [diff] [review]
1315208.patch [1.0]

Review of attachment 8809285 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #1)
> Created attachment 8809285 [details] [diff] [review]
> 1315208.patch [1.0]
> 
> So, this looks different from the function you wanted to export because we
> still needed to return an object (LayoutView) so we can take care of
> destroying the view. We also require LayoutView to hold onto some of its
> internal variables (this.inspector) that would later be used.
Yes, but I like how you implemented it now.

> One curious thing that I ran into was that addExistingTab creates an
> InspectorTabPanel, which doesn't quite make sense since these panels already
> have existing markup in inspector.xhtml that takes care of a tab panel. I
> don't think creating this InspectorTabPanel is necessary or otherwise we
> should be getting ride of the tab panel that exist in the markup.
Precisely, InspectorTabPanel() component has been implemented to support
side-panel that use existing HTML markup. These panels are not yet converted
into React.

Since your panel is already based on React it doesn't need any helper markup
in inspector.xul and so, it doesn't need InspectorTabPanel.

This patch already look good to me, just the addSidebarTab can be improved.
(it's fine to do it in a follow up if you prefer)

R+ assuming Try is green.


Thanks Gabriel!

Honza

::: devtools/client/inspector/layout/layout.js
@@ +40,5 @@
> +      "layoutview",
> +      INSPECTOR_L10N.getStr("inspector.sidebar.layoutViewTitle"),
> +      provider,
> +      defaultTab == "layoutview"
> +    );

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #2)
> Also, we need to provide id, title to addSidebarTab or selecting the default
> tab will not work.
I couldn't reproduce this.


Both tab ID and title don't have to be specified since there are already passed as properties to the panel component (the `provider` in this case). So, following should work (I tested default selection and it seems to be working for me):

this.inspector.addSidebarTab(
  undefined,
  undefined,
  provider,
  defaultTab == "layoutview"
);

But two undefined args isn't nice and it would be better to improve the APIs. E.g. better would be to pass a config object with props to the addSidebarTab()

I couldn't find any addon (registered on AMO) that would use this API so, not sure if we have to preserver back comp.

So, the code would be as follows:

this.inspector.addSidebarTab({
  panel: provider,
  selected: defaultTab == "layoutview"
});


Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> This patch already look good to me, just the addSidebarTab can be improved.
> (it's fine to do it in a follow up if you prefer)

Gabriel I reported bug 1316608, where I'll fix the exception and improve the API.
So, your patch should be good to go.

Honza

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/487a5da50e56454385118bb8da2e9110f039e83d
Bug 1315208 - Remove explicit call to `ReactDOM.render` in the LayoutView r=Honza

Comment 6

[Carsten Book [:Tomcat]]

sorry had to back this out, because this conflicts in devtools/client/inspector/inspector.js with something else that landed on autoland/m-c .

To resolve this conflict and make a mozilla-inbound to mozilla-central merge possibly i have to back this out.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e15d138a273a
Backed out changeset 487a5da50e56 for conflicting with autoland/mozilla-inbound to m-c merge

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96967030071dd928ed0460c1ec7edcc213b6419
Bug 1315208 - Remove explicit call to `ReactDOM.render` in the LayoutView r=Honza

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d96967030071