1305786 - Add initial app boilerplate for creating a new layout panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

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

The purpose of this bug is to add a new layout panel to the Inspector sidebar. Hidden by default for the purpose of working on the CSS Grid panel within the layout panel. This bug will add the necessary react/redux boilerplate needed to start the CSS Grid panel project.

Comment 1

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

Attachment: 1305786.patch

Honza, I am trying to start a new redux/react app inside an inspector tab panel. 

Currently, running into the following error:
Invariant Violation: addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's `render` method, or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner).

I was wondering if you know what may be causing or the best way to approach what I am trying to achieve. The first papercut I ran into was the need to use the browser loader, so, I am using the React loaded already in the the inspector panel. 

I suspect I actually need to start redux/react app at the top level of the inspector panel when the inspector tab panel is being created. I know I can get this working in an iframe, but that is something we want to avoid.

Comment 2

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

Attachment: bug1305786-loader.patch

Patch avoiding React double load attached.

I am seeing two issues in your patch:

1) You need to use browserLoader to load top level modules. There is one available in the InspectorPanel through a getter (coming from the Toolbox) so, it can be used.

2) BrowserLoader maintains a list of hard-coded directories from which it's loading components. And since you introduced a new App component (devtools/client/inspector/layout/) the dir must be included.

This is not nice, but we need to live with that (I guess till the entire DevTools are React).

I appended "resource://devtools/client/inspector/" dir to the list, but if it's ok for you to put the App component into (for now) empty 'layout/components' directory it should work without a change. 

There is regexp, in addition to the hard-coded list:
https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/devtools/client/shared/browser-loader.js#27

And that should be enough if all components are in 'layout/components' dir.

Honza

Comment 3

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

Comment on attachment 8795995 [details] [diff] [review]
1305786.patch

Just clearing the feedback flag.

Honza

Comment 4

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

Attachment: 1305786.patch [1.0]

Comment 5

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

Comment on attachment 8796420 [details] [diff] [review]
1305786.patch [1.0]

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

Look great in general, just some inline comments.

Honza

::: devtools/client/inspector/inspector-panel.js
@@ +23,5 @@
>  const {ComputedViewTool} = require("devtools/client/inspector/computed/computed");
>  const {FontInspector} = require("devtools/client/inspector/fonts/fonts");
>  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
>  const {InspectorSearch} = require("devtools/client/inspector/inspector-search");
> +const {LayoutViewTool} = require("devtools/client/inspector/layout/layout");

inspector-panel.js has been renamed to inspector.js so, there are conflicts when applying this patch.

::: devtools/client/inspector/layout/layout.js
@@ +18,5 @@
> +    const React = this.inspector.React;
> +    const ReactDOM = this.inspector.ReactDOM;
> +    const browserRequire = this.inspector.browserRequire;
> +
> +    const { Provider } = browserRequire("devtools/client/shared/vendor/react-redux");

I know this comes from the patch I created, but I think it would be nice if 'react-redux' binding module is accessible (and shared) just like 'react' or 'react-dom' modules (Toolbox -> InspectorPanel).

See: 
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/framework/toolbox.js#L482
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/inspector/inspector.js#L412

::: devtools/client/inspector/layout/reducers/grids.js
@@ +16,5 @@
> +  let reducer = reducers[action.type];
> +  if (!reducer) {
> +    return grids;
> +  }
> +  return reducer(grids, action);

What do you compose reducers this way?
I am also seeing index.js file (in reducers dir) that should
do this job using 'combineReducers' no?

E.g.

export default combineReducers({
  grids,
  secondReducer: myReducer,
  etc,
});

Comment 6

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

Attachment: 1305786.patch [2.0]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Comment on attachment 8796420 [details] [diff] [review]
> 1305786.patch [1.0]
> 
> Review of attachment 8796420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Look great in general, just some inline comments.
> 
> Honza
> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +23,5 @@
> >  const {ComputedViewTool} = require("devtools/client/inspector/computed/computed");
> >  const {FontInspector} = require("devtools/client/inspector/fonts/fonts");
> >  const {HTMLBreadcrumbs} = require("devtools/client/inspector/breadcrumbs");
> >  const {InspectorSearch} = require("devtools/client/inspector/inspector-search");
> > +const {LayoutViewTool} = require("devtools/client/inspector/layout/layout");
> 
> inspector-panel.js has been renamed to inspector.js so, there are conflicts
> when applying this patch.
> 

Fixed.

> ::: devtools/client/inspector/layout/layout.js
> @@ +18,5 @@
> > +    const React = this.inspector.React;
> > +    const ReactDOM = this.inspector.ReactDOM;
> > +    const browserRequire = this.inspector.browserRequire;
> > +
> > +    const { Provider } = browserRequire("devtools/client/shared/vendor/react-redux");
> 
> I know this comes from the patch I created, but I think it would be nice if
> 'react-redux' binding module is accessible (and shared) just like 'react' or
> 'react-dom' modules (Toolbox -> InspectorPanel).
> 
> See: 
> https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/framework/
> toolbox.js#L482
> https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/inspector/
> inspector.js#L412
> 

Fixed. Added ReactRedux to toolbox.js and inspector.js

> ::: devtools/client/inspector/layout/reducers/grids.js
> @@ +16,5 @@
> > +  let reducer = reducers[action.type];
> > +  if (!reducer) {
> > +    return grids;
> > +  }
> > +  return reducer(grids, action);
> 
> What do you compose reducers this way?
> I am also seeing index.js file (in reducers dir) that should
> do this job using 'combineReducers' no?
> 
> E.g.
> 
> export default combineReducers({
>   grids,
>   secondReducer: myReducer,
>   etc,
> });

I am following the way we compose reducers in the responsive.html project. See responsive.html/reducers/. It is indeed different from what you will see in the DOM view, console and debugger. I plan to stick to the methodology used in responsive.html

Comment 7

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

Attachment: 1305786.patch [3.0]

Comment 8

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

Comment on attachment 8796782 [details] [diff] [review]
1305786.patch [3.0]

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

Looks good now!

Honza

Comment 9

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

https://hg.mozilla.org/integration/fx-team/rev/7aeb526759fe3a57a87f0af96a35d64b4f23f4ab
Bug 1305786 -  Add initial react/redux boilerplate for creating a new layout panel r=honza

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7aeb526759fe

Comment 11

[Ton [:Tonnes]]

(In reply to Carsten Book [:Tomcat] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/7aeb526759fe
> 
> +# LOCALIZATION NOTE (inspector.sidebar.computedViewTitle):
> +# This is the title shown in a tab in the side panel of the Inspector panel
> +# that corresponds to the tool displaying layout information defined in the page.
> +inspector.sidebar.layoutViewTitle=Layout

Note and entity mismatch.

Additionally, "Layout" and "Box Model" appear to be inconsistent and cause confusion, looking at various locales, likely because it used to be called Box Model and changed silently without entity change.
Can this be fixed?

https://hg.mozilla.org/mozilla-central/rev/f249501590e6
https://hg.mozilla.org/mozilla-central/rev/41f5b98f7b55
https://transvision.mozfr.org/string/?entity=devtools/client/inspector.properties:inspector.sidebar.layoutViewTitle&repo=aurora

Comment 12

[Julian Descottes [:jdescottes]]

(In reply to Ton from comment #11)
> 
> Note and entity mismatch.
> 
> Additionally, "Layout" and "Box Model" appear to be inconsistent and cause
> confusion, looking at various locales, likely because it used to be called
> Box Model and changed silently without entity change.
> Can this be fixed?
> 

Thanks for the heads up! Will fix this in Bug 1321504.