1297651 - Move toolsidebar.js into tabbar.js and make it more generic

Status: RESOLVED INVALID

(DevTools :: General, enhancement)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

In working on bug 1245921 I discovered that I was pretty much duplicating the contents of toolsidebar.js inside toolbox.js.

Because we use all tabs in the same way (click one and open an iframe) we should move toolsidebar.js into tabbar.js and make it more generic.

Comment 1

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

We've been quickly chatting about this on IRC, just for the record...

The reason why ToolSidebar (React comp) exists is that it replaces the existing ToolSidebar (XUL) and provides the same API so, it's easier to integrated it with Toolbox panels. It's now used in the Inspector panel.

ToolSidebar is based on TabBar (React comp.) that implements generic interface and is used at couple of places already (e.g. HTTPi, ToolSidebar, JSON Viewer).

It makes perfect sense to me to use TabBar also for the main Toolbox tab bar.

It might also make sense to keep ToolSidebar (React comp.) around till all ToolSidebars (XUL) are replaced. Again, it has the same API and so, the replacement process is easier/safer.

Eventually, we can end up with just one generic TabBar component used everywhere (not sure how far in the future).

Honza

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] 1297651-move-toolsidebar-into-tabbar.diff

I needed to use this as the tabbar component is more complex so I changed it to an ES6 class.

WIP patch.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I am just moving the things that obviously should live in tabbar into tabbar... there will still be a ToolSidebar component.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] 1297651-move-toolsidebar-into-tabbar.diff

Works fine now... just need to update the syntax in tests e.g.

```
inspector.sidebar.toggleTab()
```

Is now:

```
inspector.sidebar.tabbar.toggleTab()
```

Which makes sense, especially when considering any other component that wants the tabbar to know how to toggle tabs.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] 1297651-move-toolsidebar-into-tabbar.diff

Just three failures to fix and one of them is just a linting issue:

1. 113 INFO TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_animated_properties_displayed.js | Uncaught exception - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/components/tabs/tabbar.js:243 - TypeError: this.props.panel.querySelector is not a function
2. 215 INFO TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_sidebar_existing_tabs.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/framework/test/browser_toolbox_sidebar_existing_tabs.js:53 - TypeError: tabbar is undefined
3. [task 2016-09-01T15:04:13.121329Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/layout/layout.js:604:48 | Block must not be padded by blank lines. (padded-blocks)

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] 1297651-move-toolsidebar-into-tabbar.diff

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8787636 [details] [diff] [review]
[WIP] 1297651-move-toolsidebar-into-tabbar.diff

Moving these tab-related methods into tabbar makes it far easier for tools to make use of the tabbar without duplicating code.

Comment 10

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

I am getting the following exception when trying out the patch.
Am I missing something?

Honza


TypeError: class constructors must be invoked with |new|
Stack trace:
Tabbar@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-proxy.js line 563 > Function:2:16
[38]</ReactCompositeComponentMixin.mountComponent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:5898:18
[78]</ReactPerf.measure/wrapper@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:12877:16

Honza

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I'll take a look... I assume you are basing it off Mozilla Central?

Comment 12

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

Comment on attachment 8787636 [details] [diff] [review]
[WIP] 1297651-move-toolsidebar-into-tabbar.diff

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

Thanks for the patch!

First round of review comments.

Honza

::: devtools/client/animationinspector/test/head.js
@@ +10,5 @@
>  Services.scriptloader.loadSubScript(
>    "chrome://mochitests/content/browser/devtools/client/inspector/test/head.js",
>    this);
>  
> +// Services.prefs.setBoolPref("devtools.dump.emit", true);

Shouldn't this be removed?

::: devtools/client/inspector/toolsidebar.js
@@ +69,5 @@
>        "devtools/client/shared/components/tabs/tabbar"));
>  
> +    let tabbar = Tabbar({
> +      namespace: this._namespace,
> +      options: this._options,

I don't see 'options' prop being used in Tabbar. Why it's passed in?

@@ +73,5 @@
> +      options: this._options,
> +      panel: this._toolPanel,
> +      panelDoc: this._panelDoc,
> +      showAllTabsMenu: true,
> +      tabbox: this._tabbox,

I don't see 'tabbox' being used in Tabbar.

@@ +80,2 @@
>        onSelect: this.handleSelectionChange.bind(this),
> +      onPanelReady: this.handlePanelReady.bind(this),

It would be great to keep the Tabbar component generic. I am worried about the new props: panel, panelDoc, toolbox, onPanelReady. They are all 'panel' specific. What about cases where we want to use Tabbar, but there is not panel or toolbox?

What if we keep Tabbar as is and introduce PanelTab? (wrapping Tabbar). Could this object be the one which is actually emitting the events?

::: devtools/client/shared/components/tabs/tabbar.js
@@ +29,5 @@
>      };
> +
> +    this.onTabChanged = this.onTabChanged.bind(this);
> +
> +    EventEmitter.decorate(this);

This feels like something that should be avoided. Emitting events doesn't seem like something that should be React component's responsibility.

It would be better if there is separate object that is being decorated and some callbacks are rather passed into the component as props. In fact, this was one of the reasons why the API has been part of the ToolSidebar object.

@@ +123,5 @@
> +   * got created.
> +   * @param {String} tabPanelId Optional. If provided, this ID will be used
> +   * instead of the tabId to retrieve and remove the corresponding <tabpanel>
> +   */
> +  * removeTab(tabId, tabPanelId) {

Remove the asterisk at the begging of the line.

Comment 13

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11)
> I'll take a look... I assume you are basing it off Mozilla Central?
I am using fx-team

Honza

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] 1297651-move-toolsidebar-into-tabbar.diff

Still need to address event handling.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> Comment on attachment 8787636 [details] [diff] [review]
> [WIP] 1297651-move-toolsidebar-into-tabbar.diff
> 
> Review of attachment 8787636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!
> 
> First round of review comments.
> 
> Honza
> 
> ::: devtools/client/animationinspector/test/head.js
> @@ +10,5 @@
> >  Services.scriptloader.loadSubScript(
> >    "chrome://mochitests/content/browser/devtools/client/inspector/test/head.js",
> >    this);
> >  
> > +// Services.prefs.setBoolPref("devtools.dump.emit", true);
> 
> Shouldn't this be removed?
> 

No, we have two prefs that should always be easily togglable in head.js:
- devtools.debugger.log
- devtools.dump.emit

By the letter I guess I could raise another bug for this but it isn't really worth it.

> ::: devtools/client/inspector/toolsidebar.js
> @@ +69,5 @@
> >        "devtools/client/shared/components/tabs/tabbar"));
> >  
> > +    let tabbar = Tabbar({
> > +      namespace: this._namespace,
> > +      options: this._options,
> 
> I don't see 'options' prop being used in Tabbar. Why it's passed in?
> 

It isn't used, well spotted.

> @@ +73,5 @@
> > +      options: this._options,
> > +      panel: this._toolPanel,
> > +      panelDoc: this._panelDoc,
> > +      showAllTabsMenu: true,
> > +      tabbox: this._tabbox,
> 
> I don't see 'tabbox' being used in Tabbar.
> 

Again, well spotted.

> @@ +80,2 @@
> >        onSelect: this.handleSelectionChange.bind(this),
> > +      onPanelReady: this.handlePanelReady.bind(this),
> 
> It would be great to keep the Tabbar component generic. I am worried about
> the new props: panel, panelDoc, toolbox, onPanelReady. They are all 'panel'
> specific. What about cases where we want to use Tabbar, but there is not
> panel or toolbox?
> 
> What if we keep Tabbar as is and introduce PanelTab? (wrapping Tabbar).
> Could this object be the one which is actually emitting the events?
> 

I have added PanelTab, which extends TabBar.

> ::: devtools/client/shared/components/tabs/tabbar.js
> @@ +29,5 @@
> >      };
> > +
> > +    this.onTabChanged = this.onTabChanged.bind(this);
> > +
> > +    EventEmitter.decorate(this);
> 
> This feels like something that should be avoided. Emitting events doesn't
> seem like something that should be React component's responsibility.
> 
> It would be better if there is separate object that is being decorated and
> some callbacks are rather passed into the component as props. In fact, this
> was one of the reasons why the API has been part of the ToolSidebar object.
> 

Removed event emitters from tabbar and paneltab. All events are now emitted from toolsidebar via callbacks now.

> @@ +123,5 @@
> > +   * got created.
> > +   * @param {String} tabPanelId Optional. If provided, this ID will be used
> > +   * instead of the tabId to retrieve and remove the corresponding <tabpanel>
> > +   */
> > +  * removeTab(tabId, tabPanelId) {
> 
> Remove the asterisk at the begging of the line.

No, this is how generator functions are used in es6 classes.

Comment 17

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

Mike, I can't cleanly apply the patch, there are following conflicts:

applying bug1297651.patch
patching file devtools/client/inspector/components/inspector-tab-panel.js
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/components/inspector-tab-panel.js.rej
patching file devtools/client/inspector/inspector-panel.js
Hunk #1 FAILED at 399
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/inspector-panel.js.rej
patching file devtools/client/inspector/toolsidebar.js
Hunk #1 FAILED at 16
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/inspector/toolsidebar.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1297651.patch


Honza

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

Rebased a little although I don't get any of your errors.

This is based on mozilla-inbound so you will need to use that repo to apply the patch. As far as I understand it, fx-team is going into retirement.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> Created attachment 8793297 [details] [diff] [review]
> 1297651-move-toolsidebar-into-tabbar.diff
> 
> Rebased a little although I don't get any of your errors.
> 
> This is based on mozilla-inbound so you will need to use that repo to apply
> the patch. As far as I understand it, fx-team is going into retirement.

Even if it is going into retirement it seems to be what Honza is still working on so I will rebase the patch on top of fx-team.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

Considering the amount of changes needed to rebase this for fx-team lets put it through try again.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 22

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

Comment on attachment 8793362 [details] [diff] [review]
1297651-move-toolsidebar-into-tabbar.diff

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

I like how it's done now!

I am still having some conflicts when applying the patch on fx-team.
After rebasing and applying the patch I can't open the Inspector panel's sidebar. I am seeing the following exception:

Console Service ERROR [JavaScript Error: "TypeError: sidePaneContainer is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js" line: 1337}] [JavaScript Error: "TypeError: sidePaneContainer is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js" line: 1337}]

I am also seeing (could be the culprit) JSM error: TypeError: class constructors must be invoked with |new|

Do you also see it?

Honza

::: devtools/client/inspector/toolsidebar.js
@@ +83,3 @@
>      });
>  
> +    this.tabbar = this.ReactDOM.render(tabbar, this._tabbox);

Every render method should have a return value.

::: devtools/client/shared/components/tabs/paneltab.js
@@ +14,5 @@
> +const { div } = DOM;
> +
> +/**
> + * Renders Tabbar component.
> + */

It would be great to have a comment explaining why this component exists and when to use it.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

Sorry I took so long to back to you, I was sick.

I'll add the comment and the return from render.

I can't reproduce the errors that you mention... here is a rebased patch. Can you try to reproduce the errors?

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

I'll have added the comment and the return from render.

I can't reproduce the errors that you mention... here is a rebased patch. Can you try to reproduce the errors?

Comment 25

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

Comment on attachment 8797151 [details] [diff] [review]
1297651-move-toolsidebar-into-tabbar.diff

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #24)
> Created attachment 8797151 [details] [diff] [review]
> 1297651-move-toolsidebar-into-tabbar.diff
> 
> I'll have added the comment and the return from render.
> 
> I can't reproduce the errors that you mention... here is a rebased patch.
> Can you try to reproduce the errors?
Yes, I am still seeing the same errors.

chrome://devtools/content/inspector/inspector.js (1374)

and

TypeError: class constructors must be invoked with |new|


Honza

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

Another huge rebase.

@Honza: Can you try to reproduce the errors again with this patch? If you can then can you tell me exactly what you are doing to trigger the errors you mention? I can't reproduce them at all, not even with a debug build.

If you can reproduce them can you let me know which version of Windows you are running as it may be a Windows only issue.

Comment 27

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

Comment on attachment 8797676 [details] [diff] [review]
1297651-move-toolsidebar-into-tabbar.diff

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #26)
> Created attachment 8797676 [details] [diff] [review]
> If you can reproduce them can you let me know which version of Windows you
> are running as it may be a Windows only issue.
OK, the problem was related to "devtools.loader.hotreload" pref introduced by James while ago. I forgot to flip it off. When the pref is false (default, off) the exception doesn't happen!

Honza

::: devtools/client/shared/components/tabs/paneltab.js
@@ +22,5 @@
> + *
> + * The PanelTab component should be used for creating tabs that are used for
> + * switching between panels as opposed to switching between e.g. divs.
> + */
> +class PanelTab extends Tabbar { //eslint-disable-line

When chatting with James about this patch, he noticed that we should not use inheritance for React components and I agree.
There are better approaches like composition. I think that the PanelTab should rather wrap the Tabbar instead of extending it.
Can we have that in the patch?

Comment 28

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

Just clearing NI (see comment 27).

Honza

Comment 29

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

Composition is a difficult subject to get your head around... especially considering the simplicity of online examples.

PanelTab now uses compositing to add a bunch of methods to TabBar:

```
module.exports = PanelTab(Tabbar);
```

It would be very easy to copy this pattern for other plugin style components e.g. PanelTab is basically a plugin for Tabbar. The plugins don't even need a render method because this style of compositing uses inverse inheritance.

So in a nutshell I really like this approach but it was a really difficult concept to grasp.

Comment 30

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 31

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

Comment on attachment 8802133 [details] [diff] [review]
1297651-move-toolsidebar-into-tabbar.diff

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

::: devtools/client/shared/components/tabs/paneltab.js
@@ +19,5 @@
> + *
> + * This component should be used for creating tabs that are used for
> + * switching between panels as opposed to switching between e.g. divs.
> + */
> +var PanelTab = ComposedComponent => class extends ComposedComponent {

I was rather hoping that we can avoid inheritance completely.


Briefly, I thought we can do something like as follows:

import Tabs from './tabs';

class PanelTab extends React.Component {
  static displayName = 'PanelTab';

  // ... new APIs

  render() {
    return (
      div({className: "devtools-sidebar-tabs"},
        Tabs({ ...props },
          /* tabs */
        )
      )
    );
  }
}

* Embed Tabs component within PanelTab.
* Use Tabs component to render the UI.
* Implement new APIs for PanelTab (like e.g. addTab, addExistingTab, etc.)
* Pass necessary props to the embedded Tabs component.


Or am I missing something? Anyway, I might be wrong, let's see what James thinks.

Honza

Comment 32

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

James, please see comment #31

Honza

Comment 33

[James Long (:jlongster)]

Agreed, Jan's approach is more idiomatic React code. The gist is what if you wanted to add other behaviors to `PanelTab`? With inheritance suddenly you get into multiple inheritance, which I don't think ES6 even supports, and is very complicated to manage. With composition, you simply wrap the output with another component that implements whatever behavior you what. Composition is a lot more flexible than inheritance.

Comment 34

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So compositing with Inverse Inheritence is a no go... I am now implementing it according to Honza'a suggestions.

Comment 35

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

To be honest I am a bit lost. You are saying to use composition and to completely avoid inheritance but we need the methods inside TabBar to be available through PanelTab.

PanelTab contains all of the Panel specific methods (addTab, addFrameTab etc.)
TabBar contains the generic stuff e.g. getCurrentTabId().

So if the inspector uses PanelTab it has no access to getCurrentTabId() or any other TabBar methods.

If PanelTab extends TabBar the inspector would have full access to getCurrentTabid() etc.

You say not to use inheritance... I guess I could delegate e.g.

```
getCurrentTabId() {
  return this._TabBar.getCurrentTabId();
}
```

But doing that for every method makes no sense.

I could expose TabBar as a property of panelTab but that doesn't involve compositing.

I guess I don't know how you want me to use compositing to achieve this and I suspect that you have something very specific in mind.

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I guess I could use refs if I had to:

```
var Vehicle = React.createClass({
    ...
});

var Airplane = React.createClass({
    methodA: function() {
      if (this.refs != null) return this.refs.vehicle.methodA();
    },
    ...
    render: function() {
        return (
            <Vehicle ref="vehicle">
                <h1>J/K I'm an airplane</h1>
            </Vehicle>
        );
    }
});
```

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I've decided to just pass the methods in as props.

Comment 38

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

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #35)
> Created attachment 8806000 [details] [diff] [review]
> 1297651-move-toolsidebar-into-tabbar.diff
> 
> To be honest I am a bit lost. You are saying to use composition and to
> completely avoid inheritance but we need the methods inside TabBar to be
> available through PanelTab.
> 
> PanelTab contains all of the Panel specific methods (addTab, addFrameTab
> etc.)
> TabBar contains the generic stuff e.g. getCurrentTabId().
> 
> So if the inspector uses PanelTab it has no access to getCurrentTabId() or
> any other TabBar methods.
> 
> If PanelTab extends TabBar the inspector would have full access to
> getCurrentTabid() etc.
> 
> You say not to use inheritance... I guess I could delegate e.g.
> 
> ```
> getCurrentTabId() {
>   return this._TabBar.getCurrentTabId();
> }
> ```
> 
> But doing that for every method makes no sense.
> 
> I could expose TabBar as a property of panelTab but that doesn't involve
> compositing.

Both these approaches (method delegation or exposing TabBar as a property) work for me. I think it make sense. Both represent an alternative to inheritance. And both should be good enough in order to reuse the new PanelTab object at both places: for the Inspector side bar as well as for the main Toolbox tabs - bug 1245921 (and btw. also for the Netmonitor sidebar when needed).

I am not 100% sure what are the exact requirements for the PanelTab in bug 1245921 (Toolbox tabs), but I am thinking about:

* Adding a new tab
* Get current tab
* Select a tab
* Handling emitted events

Note that the current ToolSidebar isn't even a Component. It's wrapping the Tabbar object and using it's API (and it delegates some of the methods as well, like e.g. toggleTab, select, getCurrentTabId, ...). A question: would it be easier if the new PanelTab is done similarly? The point here is that we have something more generic than ToolSidebar, right?

Also note that in standard React based (web) application, Components don't expose any API (the app doesn't call Component's methods directly). In our case it's rather a workaround that should be removed as soon as we have one tree of React components.

Honza

Comment 39

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1297651-move-toolsidebar-into-tabbar.diff

So the challenge is to add the following methods to tabbar.js from paneltab.js without using inheritance:
- InspectorTabPanel()
- addTab()
- addFrameTab()
- onPanelMounted()
- getTabPanel()

And obviously to delegate the render method.

Comment 40

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8806657 [details] [diff] [review]
1297651-move-toolsidebar-into-tabbar.diff

Switching to m-c and mozreview.

Comment 41

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1297651 - Move toolsidebar.js into tabbar.js and make it more generic

We were previously working at cross purposes as I thought Honza wanted me to use high order functions to compose and decorate the components. Having asked and gained clarification I understand that Honza wanted me to pass the extra methods in using props. Actually, it is quite tidy this way.

Review commit: https://reviewboard.mozilla.org/r/90104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90104/

Comment 42

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8806804 [details]
Bug 1297651 - Move toolsidebar.js into tabbar.js and make it more generic

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90104/diff/1-2/

Comment 43

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8806804 [details]
Bug 1297651 - Move toolsidebar.js into tabbar.js and make it more generic

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90104/diff/2-3/

Comment 44

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

Comment on attachment 8806804 [details]
Bug 1297651 - Move toolsidebar.js into tabbar.js and make it more generic

https://reviewboard.mozilla.org/r/90104/#review90806

Mike, please see my inline comment.

Honza

::: devtools/client/inspector/toolsidebar.js:77
(Diff revision 3)
> -    let sidebar = Tabbar({
> -      toolbox: this._toolPanel._toolbox,
> +    let tabbar = Tabbar({
> +      panel: this._toolPanel,
> +      panelDoc: this._panelDoc,
>        showAllTabsMenu: true,
> +      idPrefix: this._idPrefix,
>        onSelect: this.handleSelectionChange.bind(this),
> +      onPanelReady: this.handlePanelReady.bind(this),
> +      onNewTabRegistered: this.handleNewTabRegistered.bind(this),
> +      onToolReady: this.handleToolReady.bind(this),
> +      onTabUnregistered: this.handleTabUnregistered.bind(this),
> +
> +      InspectorTabPanel: this.InspectorTabPanel.bind(this),
> +      addTab: PanelTab.addTab,
> +      addExistingTab: PanelTab.addExistingTab,
> +      addFrameTab: PanelTab.addFrameTab,
> +      onPanelMounted: PanelTab.onPanelMounted,
> +      getTabPanel: PanelTab.getTabPanel,

I am still unsure whether this is the right approach. I might be wrong, but I still think that Tabbar shouldn't know about 'toolbox panels' and stay rather generic so, it can be used even in cases where there is no Toolbox and toolbox panel involved (e.g. HTTP Inspector and JSON Viewer, both no Toolbox involved).

See also my comment #12 ("It would be great to keep the Tabbar component generic. I am worried about the new props: panel, panelDoc, toolbox, onPanelReady.").

Even if some of the methods are now moved into separate file (paneltab.js), they are depending on specific state and properties coming from the Tabbar. It's quite wired together anyway.

If we decided that it's fine for Tabbar() to know about 'toolbox & panels' I think it's better if all these methods are just defined in Tabbar as standard React Components methods. It's more convenient.

I might actually be missing something here. Specifically, I think, the requirements from bug 1245921.

If I understand correctly, this bug has been reported to avoid code duplication of the toolsidebar.js (client/inspector/toolsidebar.js)


Let me recap the current state.

We have three things: 

1) Tabs (client/shared/components/tabs/tabs.js)
This is basic React component rendering tabs. Individual tabs are passed in as child components.

2) Tabbar (client/shared/components/tabs/tabbar.js)
Build on top of Tabs component. It adds support for dynamic addition and removal of tabs and also support for 'all tabs context menu'. It's using the 'Tabs' component to render the result. It's generic and it doesn't know about Toolbox or Toolbox panels.

3) Toolsidebar (client/inspector/ToolSidebar)
This is a helper object (not a component) that uses Tabbar to render 'side panel tabs' (aka Panel sidebar). This object know about a Toolbox panel and it appends side bar to the right hand side of the panel.

---

Now, AFAIU, the Toolsidebar could be nicely reused in Bug 1245921 to implement the main Toolbox tabs. This sounds great and makes perfect sense. The difficult part is that the Toolsidebar can't be used as is since it isn't generic enough - it's designed to build panel-side-bar, correct?

So, we need to extract some parts of the Toolsidebar and move them somewhere. There are two options (or maybe more!).

1) Let's move that code to Tabbar().
It won't be that generic anymore, but perhaps not a problem even if there are new properties (like e.g. toolbox, panel, etc.) they don't have to be set and all still work for HTTPi and JSONv.

2) Let's move that code to new Paneltab().
This would be an object built on top of generic Tabbar() (just like the Toolsidebar now) and adding features that involve the Toolbox and Toolbox panels. This objet would be used by Toolsidebar as well as by 'Main Toolbox tabs' -> that's what we want in the first place.

This is what I though you want to do, but the tabbar.js file doesn't seem like it implements a reusable component.

It would be greate to provide some recap of what the bug 1245921 is expecting and what are the requirements. For example, why Toolsidebar can't be used as is and what needs to go out? 

Does that make sense?

Honza

Comment 45

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

Greg, I noticed that you are assigned to bug 1245921.
Please see my comment #44 above.

Honza

Comment 46

[Greg Tatum [:gregtatum]]

I'm kind of concerned about sharing the code for something like a tool's sidebar tab implementation with the main toolbox's implementation. I think the requirements between the two may be fairly different especially given Helen's designs. I think conflating the requirements here will make for harder to maintain code. Here are the differences I'm seeing.

Keyboard Navigation:

The Toolbox Toolbar has the behavior that when you hit tab, it lands only once inside of the toolbox. You can then hit the arrow keys to move left and right within that space. The navigation needs to know about the following

 * Element Picker
 * Toolbox Tabs
 * Command Buttons
 * Host Buttons
 * Close Buttons

Each of which is configurable.

Additionally the current keyboard navigation behavior is to only switch tabs when you hit spacebar or enter, which makes sense for a mix of tabs and command buttons. The inspector side panel does not have this complexity, as when you hit the arrow keys it switches between the tabs.

Helen's designs also call for different functionality for making the UX better for the Toolbox Toolbar that may not make sense from a sidebar perspective.

And finally, it's taking a bit of work translating from XUL -> React component, especially with the current state management. It seems hard to create a great shared abstraction when the problem-space of the current implementation hasn't been totally sorted out. It seems to me that it would be good to create the two separate implementations, and then merge together any shared abstractions where it makes sense.

Right now I'm concentrating on getting feature parity with the current toolbox toolbar, and then using that as a base to implement Helen's work. It seems like it will be harder to do that with worrying about making it compliant with a shared component with different requirements. I should have a first working pass completed in the next day or two on the Toolbox Toolbar.

Comment 47

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #46)
> Right now I'm concentrating on getting feature parity with the current
> toolbox toolbar, and then using that as a base to implement Helen's work. It
> seems like it will be harder to do that with worrying about making it
> compliant with a shared component with different requirements. I should have
> a first working pass completed in the next day or two on the Toolbox Toolbar.

You are right, if the requirements are so different it is going to be far easier not to share the functionality until everything is converted.

Thanks for the comment... there is no point wasting time on something that nobody wants (in a good way).

Comment 48

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

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #46)
> It seems to me that it would
> be good to create the two separate implementations, and then merge together
> any shared abstractions where it makes sense.
Yes, I support this approach.

Honza