1314931 - LayoutView should be browserRequire as the root component

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

We should use browserRequire to only load the root component in the Layout View. In this case, the LayoutViewTool should browserRequire CssLayoutView component. The root component can consequently load the entire tree of component using the standard require, and help with consuming any webpack bundles in the Layout View.

Comment 1

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

Attachment: 1314931.patch [1.0]

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f95c2c883b9

Comment 3

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

Comment on attachment 8807139 [details] [diff] [review]
1314931.patch [1.0]

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

What is actually the main purpose of LayoutViewTool and CssLayoutView objects?

The existing side-panels are using similar structure, but it's there mainly because there are not yet converted to React. In case a new side-panel that is fully implemented on top of React (like yours) those objects seem to be redundant. Unless I am missing something.

If the whole point of these objects is to just render content of the side panel using React component than I think we could use the following and simpler construct (in inspector.js, or in layout/main.js as a single exported function):

var MyTabPanel = React.createClass({
  render: function () {
    return (
      div({className: "tab-panel"},
        "Hello From Side Panel!"
      )
    );
  }
});

var myTabPanel = React.createFactory(MyTabPanel);
inspector.addSidebarTab(
  "my-tab-panel", // ID
  "My Tab",  // Title
  myTabPanel, // A component rendering panel's content
  false // not selected by default
);

You can also see this simple add-on example that appends a new side-panel to the Inspector panel.
https://github.com/janodvarko/prototypes/blob/master/inspector-sidebar/index.js

Honza

Comment 4

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

Attachment: 1314931.patch [2.0]

I have removed the concepts of LayoutViewTool and CssLayoutView here. Originally, we had main.js (LayoutViewTool) which basically did the browserRequire of the root component, but we can simply browserRequire from inspector.js. I renamed CssLayoutView to LayoutView which is still needed to initialize the react/redux app.

Comment 5

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

Comment on attachment 8807165 [details] [diff] [review]
1314931.patch [2.0]

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

Yes, this is step in the right direction and I am thinking to move it yet one step further. I am specifically thinking about removing the explicit `ReactDOM.render` call. 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);
}

... but this might be a follow up.


What do you think?

Honza

Comment 6

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

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Comment on attachment 8807165 [details] [diff] [review]
> 1314931.patch [2.0]
> 
> Review of attachment 8807165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this is step in the right direction and I am thinking to move it yet
> one step further. I am specifically thinking about removing the explicit
> `ReactDOM.render` call. 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);
> }
> 
> ... but this might be a follow up.
> 
> 
> What do you think?
> 
> Honza

Honza, let's move this to a separate bug in the future. I want to see the LayoutView mature a little bit after implementing some non-trivial features to see what the code looks like. I am sure similar to the rule and computed view we will listen to various inspector selection events and dispatch events to update the layout state. I think we will have a better idea of how to refactor out some pieces afterwards.

Comment 7

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

Comment on attachment 8807165 [details] [diff] [review]
1314931.patch [2.0]

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

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #6)
> Honza, let's move this to a separate bug in the future. I want to see the
> LayoutView mature a little bit after implementing some non-trivial features
> to see what the code looks like. I am sure similar to the rule and computed
> view we will listen to various inspector selection events and dispatch
> events to update the layout state. I think we will have a better idea of how
> to refactor out some pieces afterwards.
OK, doing this in a follow up is fine. Note that the important thing for me is that we get rid of the explicit `ReactDOM.render` call.

R+ assuming there is a follow-up report filed.

Thanks you!

Honza

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d032c10f6cc6d16ee04533ea7a218bf1ec4491d
Bug 1314931 - LayoutView should be browserRequire as the root component r=Honza

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d032c10f6cc

Comment 10

[Surentharan]

[bugday-20170215]