Alexandre Poirot [:ochameau] Assignee
	Description • 8 years ago

Once about:debugging is going to support remote target, it will be very useful to list tabs. So that we can debug Firefox Mobile or tabs from Valence targets.

But before that, it would allow to generate the about:devtools-toolbox urls for local tabs and start working on devtools loaded in a tab.
It won't be that useful for web developers, but can be for devtools contributor.
It should also ease working on devtools.html by loading our code closer to what we are aiming for.

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 8 years ago

Mostly miss tests and svg icon.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 8 years ago

Forgot to include l10n diff.

	Jan Keromnes [:janx]
	Comment 3 • 8 years ago

Good idea! Thank you for adding this. Like you said it's not the most useful feature on Desktop, but I still think there is value in listing every browser tab you can debug (and maybe group by window?).

Note that Chrome's behavior for this feature is to switch to the corresponding tab and open the regular toolbox, instead of opening a separate one. We don't have to match this now, but I think in the future (when we have remote debugging, and most about:debugging targets use about:devtools-toolbox URLs) we might want to reconsider doing the same for Desktop tabs.

	Jan Keromnes [:janx]
	Comment 4 • 8 years ago

Comment on attachment 8743378 [details] [diff] [review]
patch v2

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

Really like where this is going!

FYI: Chrome calls this "Pages", but I also like the name "Tabs".

::: devtools/client/aboutdebugging/components/aboutdebugging.js
@@ +17,5 @@
>    () => createFactory(require("./addons-tab")));
>  loader.lazyGetter(this, "WorkersTab",
>    () => createFactory(require("./workers-tab")));
> +loader.lazyGetter(this, "TabsTab",
> +  () => createFactory(require("./tabs-tab")));

Fun name! I hadn't thought of this.

@@ +37,5 @@
> +  id: "tabs",
> +  name: Strings.GetStringFromName("tabs"),
> +  icon: "chrome://devtools/skin/images/debugging-tabs.svg",
> +  component: TabsTab
> +}

Nit: Maybe sort target types alphabetically, i.e. "addons", "tabs", "workers"?

::: devtools/client/aboutdebugging/components/moz.build
@@ +11,5 @@
>      'service-worker-target.js',
>      'tab-header.js',
>      'tab-menu-entry.js',
>      'tab-menu.js',
> +    'tab-target.js',

Hm, hadn't thought of this. The naming gets confusing here because 'tab-target' (a target representing a browser tab) has nothing to do with 'tab-menu' (menu of all about:debugging tabs) or 'tab-header' (an about:debugging tab header).

I think we might have to rename 'tab-{header,menu-entry,menu}.js' to 'panel-{header,menu-entry,menu}.js'.

@@ +12,5 @@
>      'tab-header.js',
>      'tab-menu-entry.js',
>      'tab-menu.js',
> +    'tab-target.js',
> +    'tabs-tab.js',

If we go with the rename suggested above, it would also make sense to rename '{addons,tabs,workers}-tab.js' to '{addons,tabs,workers}-panel.js' (and the '{Addons,Tabs,Workers}Tab' components to '{Addons,Tabs,Workers}Panel').

::: devtools/client/aboutdebugging/components/tab-target.js
@@ +25,5 @@
> +  displayName: "TabTarget",
> +
> +  debug() {
> +    let { client, target } = this.props;
> +    window.open("about:devtools-toolbox?type=tab&id=" + target.outerWindowID);

Wow, I love how these are becoming one-liners! But I guess that for remote debugging, you'll have to pass some reference to `client` via the URL right?

::: devtools/client/aboutdebugging/components/tabs-tab.js
@@ +27,5 @@
> +    };
> +  },
> +
> +  componentDidMount() {
> +    let client = this.props.client;

Nit: `let { client } = this.props;`.

@@ +33,5 @@
> +    this.update();
> +  },
> +
> +  componentWillUnmount() {
> +    let client = this.props.client;

Nit: Same as above.

@@ +53,5 @@
> +    let { client } = this.props;
> +    let { tabs } = this.state;
> +
> +    return dom.div({
> +      id: "tab-tab",

Nit: You probably meant "tabs-tab", but if you agree with my suggested rename above, this might become "tabs-panel".

@@ +65,5 @@
> +    }),
> +    dom.div({ className: "inverted-icons" },
> +      TargetList({
> +        client,
> +        id: "tabs2",

If the original "tabs" becomes "panels", this "tabs2" could become the new "tabs".

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 8 years ago

(In reply to Jan Keromnes [:janx] from comment #4)
> FYI: Chrome calls this "Pages", but I also like the name "Tabs".

To be Tab is more common. But Pages would also work. It would be great to gather some external feedback about this.

> ::: devtools/client/aboutdebugging/components/moz.build
> @@ +11,5 @@
> >      'service-worker-target.js',
> >      'tab-header.js',
> >      'tab-menu-entry.js',
> >      'tab-menu.js',
> > +    'tab-target.js',
> 
> Hm, hadn't thought of this. The naming gets confusing here because
> 'tab-target' (a target representing a browser tab) has nothing to do with
> 'tab-menu' (menu of all about:debugging tabs) or 'tab-header' (an
> about:debugging tab header).
> 
> I think we might have to rename 'tab-{header,menu-entry,menu}.js' to
> 'panel-{header,menu-entry,menu}.js'.

Yes, in that precise context, may be panel is better than tab?
Hopefully we won't ever debug any panel?!!

> ::: devtools/client/aboutdebugging/components/tab-target.js
> @@ +25,5 @@
> > +  displayName: "TabTarget",
> > +
> > +  debug() {
> > +    let { client, target } = this.props;
> > +    window.open("about:devtools-toolbox?type=tab&id=" + target.outerWindowID);
> 
> Wow, I love how these are becoming one-liners! But I guess that for remote
> debugging, you'll have to pass some reference to `client` via the URL right?

Yes. I would like this module to also support specifying a remote via url:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/target-from-url.js
If that can help, I can work on this.

Also, I'll most likely come up with a reverse of this helper: give a target and retrieve a set of query parameters.

	Alexandre Poirot [:ochameau] Assignee
	Comment 6 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 8 years ago

As discussed offline. Rename tab to panel everywhere
and also create subfolders in components/
one for each panel: Addons, Workers (and tabs in next patch)

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 8 years ago

Something we should have done for a long time... (instead of pulling or introducing refresh button ;))
It watches title changes, as that's what I'm displaying first in about:debugging
and also because it is actually easier to watch than location change.
But we could possibly listen for both?

I haven't added test. Tell me if you want an test specific to this patch.
Note that this code is covered by the about:debugging patch checking for tab title change.

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 8 years ago

With the nits fixed and new file names.

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 11 • 8 years ago

Missing a moz.build file.

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 8 years ago

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

	Jan Keromnes [:janx]
	Comment 13 • 8 years ago

Comment on attachment 8743947 [details] [diff] [review]
Rename about debugging Tab(s) to Panel(s) - v1

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

Thanks a lot for going through the trouble of renaming all these!

R+ with a few nits. Also, you might want to rebase on top of bug 1210778's accessibility improvements.

::: devtools/client/aboutdebugging/components/addons/panel.js
@@ +107,5 @@
>      let name = Strings.GetStringFromName("extensions");
>      let targetClass = AddonTarget;
>  
>      return dom.div({
> +      id: "panel-addons",

Nit: While you're at it, maybe "addons-panel" is more correct?

@@ +115,3 @@
>      },
> +    PanelHeader({
> +      id: "panel-addons-header-name",

Nit: "addons-panel-header-name"?

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +103,5 @@
>      let { client } = this.props;
>      let { workers } = this.state;
>  
>      return dom.div({
> +      id: "panel-workers",

Nit: "workers-panel"?

@@ +111,3 @@
>      },
> +    PanelHeader({
> +      id: "panel-workers-header-name",

Nit: "workers-panel-header-name"?

	Jan Keromnes [:janx]
	Comment 14 • 8 years ago

Comment on attachment 8743977 [details] [diff] [review]
Show tabs in about:debugging - v3

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

Looks good!

A few comments, and the patch also needs a rebase after bug 1210778's accessibility improvements.

::: devtools/client/aboutdebugging/components/aboutdebugging.js
@@ +17,5 @@
>    () => createFactory(require("./addons/panel")));
>  loader.lazyGetter(this, "WorkersPanel",
>    () => createFactory(require("./workers/panel")));
> +loader.lazyGetter(this, "TabsPanel",
> +  () => createFactory(require("./tabs/panel")));

Nit: Maybe sort alphabetically, "Tabs" before "Workers"?

@@ +30,5 @@
>    component: AddonsPanel
>  }, {
> +  id: "tabs",
> +  name: Strings.GetStringFromName("tabs"),
> +  icon: "chrome://devtools/skin/images/debugging-tabs.svg",

This icon doesn't actually exist. Maybe we can ask Helen if she'd be willing to create one?

::: devtools/client/aboutdebugging/components/tabs/panel.js
@@ +36,5 @@
> +  },
> +
> +  update() {
> +    this.props.client.mainRoot.listTabs()
> +        .then(({ tabs }) => {

Nit: No need to break `.then` onto a new line. Also, please only indent with 2 spaces below.

@@ +40,5 @@
> +        .then(({ tabs }) => {
> +          // Set a name attribute in order for TargetList to sort tabs correctly
> +          tabs.forEach(tab => {
> +            tab.name = tab.title;
> +          });

Nit: I see you're passing `tab` directly as a `target` object. I do like this idea, but is there a way to set `target.icon` to the tab's favicon? Or, if unavailable, to a generic "tab" icon? (e.g. the same as for the menu entry) I remember that WebIDE did something like that.

@@ +50,5 @@
> +    let { client } = this.props;
> +    let { tabs } = this.state;
> +
> +    return dom.div({
> +      id: "tabs-tab",

Nit: "tabs-panel"?

@@ +51,5 @@
> +    let { tabs } = this.state;
> +
> +    return dom.div({
> +      id: "tabs-tab",
> +      className: "tab",

Nit: "panel"?

@@ +56,5 @@
> +      role: "tabpanel",
> +      "aria-labelledby": "tab-tab-header-name"
> +    },
> +    PanelHeader({
> +      id: "tab-tab-header-name",

Nit: "tabs-panel-header-name"?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 15 • 8 years ago

Comment on attachment 8743963 [details] [diff] [review]
Send tabListChanged messages when tab title changes - v1

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

Cool! :) Please update the comment above BrowserTabList to note the new event (TabOpen, etc. are already mentioned there).

However, won't this still miss tab URL changes?  Or is it "pretty close", since "usually" the tab title will change if the URL changes?  IIRC, we only catch tab URL changes (before this patch at least) for tabs we are attached to, due to the DebuggerProgressListener noticing them.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 16 • 8 years ago

Comment on attachment 8743963 [details] [diff] [review]
Send tabListChanged messages when tab title changes - v1

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

Would be nice to get this working for Fennec in a follow up as well...  (At first glance, they don't appear to have a TabAttrModified event currently.)

	Alexandre Poirot [:ochameau] Assignee
	Comment 17 • 8 years ago

Hi Helen, Would you have a minute to craft an icon for tabs?
This is like Addons and Workers. About:debugging is going to display tabs.
This isn't much useful for local tabs, but will be when about:debugging start supporting remote debugging (bug 1212802).
I'm also doing that to allow crafting about:devtools-toolbox url for local tabs, so that you can easily open a toolbox in a tab.

In term of icons. There is the default one displayed on the left vertical panel.
And there is the default one, displayed in the list of tabs, when a website doesn't have any favicon. But I can reuse the same svg, like what we do for workers and addons.

	Alexandre Poirot [:ochameau] Assignee
	Comment 18 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 19 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 20 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 21 • 8 years ago

Rebased.
Jan, Note that we now have weird things in aboutdebugging.js
In the `panels` global array. Each have an "id" which is the hash used to identify
a panel, and "panelId" which is the root dom element id of the panel.
We pass along id/panelId and it is often disturbing to do panel.id and panel.panelId.
See panel-menu.js for ex.

Do not hesitate to comment, I can tune that in a followup. It doesn't seem to be
entirely related to this change?

	Alexandre Poirot [:ochameau] Assignee
	Comment 22 • 8 years ago

Now uses a message manager event in addition to the TabAttrModified,
which *should* work for fennec and already works for oop tabs.

I also fixed various edge cases:
* tabs that becomes OOP. When you open a tab, it lives in parent and then
when you open a regular website it switches to OOP and mess up with BrowserTabActorList
and its cached actor map. We have to instanciate new type of TabActor instances
based on their new remote status.
* fix potential exception in TabActor.form when it is being called on a dying document.
It was happening when trying to use a TabActor on a <browser> which changed from in-parent to in-child,
but it could also happen on a <browser> that has been garbaged.

And yes, title changes would hopefully match location changes.
In about debugging, I'm especially displaying titles (I also display urls in tooltips,
and fallback to url when there is no title), so it seems to make sense to watch for title changes
rather than location. But I imagine we could possibly also watch for location changes.

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 8 years ago

Hopefully, try is going to be green now!
I had to make the sort be optional in TargetList as I don't want tabs to be sorted here.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 24 • 8 years ago

Comment on attachment 8746032 [details] [diff] [review]
Send tabListChanged messages when tab title changes - v2

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

Looks good overall, thanks for making all these bonus changes. :)

I think there are still a few edge cases left, so one more round I think.

::: devtools/server/actors/webbrowser.js
@@ +214,5 @@
>   * XUL window each tab belongs to.
> + *
> + * - Title changes:
> + *
> + * For tabs living in the child process, we listent for DOMTitleChange message

Nit: listen

@@ +300,5 @@
>      }
>    }
>  };
>  
>  BrowserTabList.prototype._getChildren = function(aWindow) {

Fennec does override this method[1] but I did not see an obvious way to check for the closing tab state for them, so probably fine as is.

[1]: https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/mobile/android/modules/dbg-browser-actors.js#67

@@ +305,5 @@
> +  if (!aWindow.gBrowser) {
> +    return [];
> +  }
> +  let { gBrowser } = aWindow;
> +  return gBrowser.browsers.filter(browser => {

The previous version seems to support the case where `gBrowser.browsers` is null, while this would break in that case.  Not sure how common that is, but perhaps we should still guard for it.

@@ +488,5 @@
> +   * We also listen for title changed from the child process.
> +   * This allows listening for title changes from Fennec and OOP tabs in Fx.
> +   */
> +  this._listenForMessagesIf(this._onListChanged && this._mustNotify,
> +                            "_listeningForTitlechange",

Nit: Titlechange -> TitleChange

@@ +516,5 @@
>    }
>  };
>  
> +/*
> + * Add or remove event listeners for all XUL windows.

Nit: message

@@ +519,5 @@
> +/*
> + * Add or remove event listeners for all XUL windows.
> + *
> + * @param aShouldListen boolean
> + *    True if we should add event handlers; false if we should remove them.

Nit: message listener

@@ +522,5 @@
> + * @param aShouldListen boolean
> + *    True if we should add event handlers; false if we should remove them.
> + * @param aGuard string
> + *    The name of a guard property of 'this', indicating whether we're
> + *    already listening for those events.

Nit: messages

@@ +649,1 @@
>      }

Do you need to check `_listeningForTitleChange` and add a message listener here?

	Jan Keromnes [:janx]
	Comment 25 • 8 years ago

Comment on attachment 8746033 [details] [diff] [review]
Show tabs in about:debugging - v4

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

(In reply to Alexandre Poirot [:ochameau] from comment #21)
> Rebased.

Thanks Alex! Looks good, but definitely needs a tabs icon before we can move forward.

> Jan, Note that we now have weird things in aboutdebugging.js
> In the `panels` global array. Each have an "id" which is the hash used to
> identify
> a panel, and "panelId" which is the root dom element id of the panel.
> We pass along id/panelId and it is often disturbing to do panel.id and
> panel.panelId.
> See panel-menu.js for ex.
> 
> Do not hesitate to comment, I can tune that in a followup. It doesn't seem
> to be entirely related to this change?

Yes, it got weird. It seems `panelId` can always be derived from `id`, so let's get rid of `panel.panelId` and fix all its uses in a follow-up.

(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Hopefully, try is going to be green now!
> I had to make the sort be optional in TargetList as I don't want tabs to be
> sorted here.

Fair enough.

::: devtools/client/aboutdebugging/components/addons/panel.js
@@ +123,5 @@
>      }),
>      AddonsControls({ debugDisabled }),
>      dom.div({ id: "addons" },
> +      TargetList({ name, targets, client, debugDisabled, targetClass,
> +                   sort: true })

Nit: Local style places one prop per line when the props object becomes too large for one line:

    TargetList({
      name,
      targets,
      // ...
      sort: true
    })

::: devtools/client/aboutdebugging/components/panel-menu.js
@@ +15,5 @@
>      let { panels, selectedPanelId, selectPanel } = this.props;
>      let panelLinks = panels.map(({ id, panelId, name, icon }) => {
>        let selected = id == selectedPanelId;
>        return PanelMenuEntry({
> +        id, panelId, name, icon, selected, selectPanel

Nit: Please place one prop per line here too.

::: devtools/client/aboutdebugging/components/tabs/panel.js
@@ +39,5 @@
> +
> +  update() {
> +    this.props.client.mainRoot.listTabs().then(({ tabs }) => {
> +      // listTabs returns array with null items for closed tabs, we have to
> +      // remove them for react to correctly remove these entries.

Nit: "Filter out closed tabs (represented as `null`)."

@@ +43,5 @@
> +      // remove them for react to correctly remove these entries.
> +      tabs = tabs.filter(tab => !!tab);
> +      tabs.forEach(tab => {
> +        // Also try to fetch low-res favicon. But we should use actor support
> +        // for this to get the high-res one (bug 1061654).

Yes. This comment should probably be a FIXME.

@@ +70,5 @@
> +    return dom.div({
> +      id: "tabs-panel",
> +      className: "panel",
> +      role: "tabpanel",
> +      "aria-labelledby": "tab-tab-header-name"

Please also rename "tab-tab-" to "tabs-panel-" here.

@@ +76,5 @@
> +    PanelHeader({
> +      id: "tabs-panel-header-name",
> +      name: Strings.GetStringFromName("tabs")
> +    }),
> +    dom.div({ className: "inverted-icons" },

You don't want this className here (it's a CSS filter to make white icons grey, which doesn't make sense for favicons).

@@ +84,5 @@
> +        name: Strings.GetStringFromName("tabs"),
> +        sort: false,
> +        targetClass: TabTarget,
> +        targets: tabs
> +      })

Nit: Is there a way to group tabs by window? I feel this would be more useful than always having a single "Tabs" TargetList inside the "Tabs" panel.

::: devtools/client/aboutdebugging/components/tabs/target.js
@@ +32,5 @@
> +      }),
> +      dom.div({ className: "target" },
> +        // If the title is empty, display the url instead.
> +        dom.div({ className: "target-name", title: target.url },
> +                target.title || target.url)

Nit: No need to left-pad, please indent with 2 spaces only when breaking a line.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +20,5 @@
>  
>    render() {
> +    let { client, debugDisabled, targetClass, targets, sort } = this.props;
> +    if (sort) {
> +      targets = this.props.targets.sort(LocaleCompare);

Nit: No need for "this.props." here.

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +129,1 @@
>          name: Strings.GetStringFromName("sharedWorkers"),

Nit: Please set `sort: true` after `name`.

@@ +137,1 @@
>          name: Strings.GetStringFromName("otherWorkers"),

Nit: Please set `sort: true` after `name`.

	Alexandre Poirot [:ochameau] Assignee
	Comment 26 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24)
> Comment on attachment 8746032 [details] [diff] [review]
> @@ +300,5 @@
> >      }
> >    }
> >  };
> >  
> >  BrowserTabList.prototype._getChildren = function(aWindow) {
> 
> Fennec does override this method[1] but I did not see an obvious way to
> check for the closing tab state for them, so probably fine as is.

I'm not sure it is important. It sounds like a race specific to existing desktop codebase.
Tab closing on fennec may be more instant.

> @@ +305,5 @@
> > +  if (!aWindow.gBrowser) {
> > +    return [];
> > +  }
> > +  let { gBrowser } = aWindow;
> > +  return gBrowser.browsers.filter(browser => {
> 
> The previous version seems to support the case where `gBrowser.browsers` is
> null, while this would break in that case.  Not sure how common that is, but
> perhaps we should still guard for it.

Fixed.

> @@ +649,1 @@
> >      }
> 
> Do you need to check `_listeningForTitleChange` and add a message listener
> here?

Yes, good catch!

	Alexandre Poirot [:ochameau] Assignee
	Comment 27 • 8 years ago

(In reply to Jan Keromnes [:janx] from comment #25)
> Comment on attachment 8746033 [details] [diff] [review]
> @@ +76,5 @@
> > +    PanelHeader({
> > +      id: "tabs-panel-header-name",
> > +      name: Strings.GetStringFromName("tabs")
> > +    }),
> > +    dom.div({ className: "inverted-icons" },
> 
> You don't want this className here (it's a CSS filter to make white icons
> grey, which doesn't make sense for favicons).

Oh, that explains why icons were grayed \o/

> @@ +84,5 @@
> > +        name: Strings.GetStringFromName("tabs"),
> > +        sort: false,
> > +        targetClass: TabTarget,
> > +        targets: tabs
> > +      })
> 
> Nit: Is there a way to group tabs by window? I feel this would be more
> useful than always having a single "Tabs" TargetList inside the "Tabs" panel.

Unfortunately no. listTabs returns just an array of tabs.
We would have to tune the server to do that, which sounds like outside
the scope of this bug.

	Jan Keromnes [:janx]
	Comment 28 • 8 years ago

Comment on attachment 8746604 [details] [diff] [review]
Show tabs in about:debugging.

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

LGTM! But please don't land before /devtools/client/themes/images/debugging-tabs.svg actually exists.

Potential follow-ups:

(From Jan Keromnes [:janx] in comment #25)
> Yes, it got weird. It seems `panelId` can always be derived from `id`, so
> let's get rid of `panel.panelId` and fix all its uses in a follow-up.

Please either upload a patch or open a dependent bug for it.

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> (In reply to Jan Keromnes [:janx] from comment #25)
> > Nit: Is there a way to group tabs by window? I feel this would be more
> > useful than always having a single "Tabs" TargetList inside the "Tabs" panel.
> 
> Unfortunately no. listTabs returns just an array of tabs.
> We would have to tune the server to do that, which sounds like outside
> the scope of this bug.

Please either upload a patch or open a dependent bug for it.

	Jan Keromnes [:janx]
	Comment 29 • 8 years ago

Helen or Tim, do you happen to know of an SVG icon somewhere in Firefox that we could use to represent "a browser tab" or "browser tabs" in about:debugging? And if not, would you be interested in contributing such an icon?

Please see attached "Screenshot" to get an idea what this is about. Thanks!

	Tim Nguyen :ntim
	Comment 30 • 8 years ago

(In reply to Jan Keromnes [:janx] from comment #29)
> Helen or Tim, do you happen to know of an SVG icon somewhere in Firefox that
> we could use to represent "a browser tab" or "browser tabs" in
> about:debugging? And if not, would you be interested in contributing such an
> icon?
> 
> Please see attached "Screenshot" to get an idea what this is about. Thanks!

We currently use this [0] for the synced tabs toolbar button in the main UI.
For the missing favicon, I would fallback to the globe we use [1]

[0]: http://firefoxux.github.io/firefox-svg-icons/icons/navigation/tab.svg
[1]: http://firefoxux.github.io/firefox-svg-icons/icons/security/identity-not-secure.svg

If it helps in the future, we have an icon viewer here: http://firefoxux.github.io/firefox-svg-icons/viewer/

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 31 • 8 years ago

Thanks Tim!

	Alexandre Poirot [:ochameau] Assignee
	Comment 32 • 8 years ago

Was about using netmon monitor icon (security-state-local.svg),
which matches the default tab icon in firefox?

If you are ok with that, I will merge that into previous patch.

	Jan Keromnes [:janx]
	Comment 33 • 8 years ago

Comment on attachment 8747741 [details] [diff] [review]
Tabs icon

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

Thanks Alex!

The icon looks great in the target list, but in the menu, I feel the grey-scale globe doesn't play well with the solid-white menu icons usually found in about:debugging and about:preferences.

Maybe the menu would benefit from a solid-white icons similar to debugging-addons.svg and debugging-workers.svg?

	Jan Keromnes [:janx]
	Comment 34 • 8 years ago

Tim, what do you think of the globe icon in the left-side menu? I feel that it doesn't play well with the other solid-white icons we usually find in about:debugging and about:preferences.

Do you think we should make a solid-white version of the globe icon? Or maybe switch to more colourful menu icons like in about:addons?

	Jan Keromnes [:janx]
	Comment 35 • 8 years ago

Comment on attachment 8745356 [details]
Screenshot

(This screenshot is now obsolete.)

	Tim Nguyen :ntim
	Comment 36 • 8 years ago

Comment on attachment 8747922 [details]
tabs-icon.png

(In reply to Jan Keromnes [:janx] from comment #34)
> Created attachment 8747922 [details]
> tabs-icon.png
> 
> Tim, what do you think of the globe icon in the left-side menu? I feel that
> it doesn't play well with the other solid-white icons we usually find in
> about:debugging and about:preferences.
For the left side menu, I would use this icon: http://firefoxux.github.io/firefox-svg-icons/icons/navigation/tab.svg . It's the icon we use in our main UI to designate tabs, we use it for the Synced tabs and the new tab icon (when placed inside the menu).

> Do you think we should make a solid-white version of the globe icon? Or
> maybe switch to more colourful menu icons like in about:addons?
I could make a solid white version of the globe icon, but I think it doesn't fit well for the terminology of "tabs", but rather fits as "default page icon".

Btw, the colourful icons in about:addons are placeholder since we don't currently have SVG versions of them :) There's a bug filed about updating them.

	Alexandre Poirot [:ochameau] Assignee
	Comment 37 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 38 • 8 years ago

Are you ok with these icons?

	Alexandre Poirot [:ochameau] Assignee
	Comment 39 • 8 years ago

Is yes, here is the related patch.

	Jan Keromnes [:janx]
	Comment 40 • 8 years ago

Comment on attachment 8748090 [details]
screenshot

Looks great! Thank you Alex

	Jan Keromnes [:janx]
	Comment 41 • 8 years ago

Comment on attachment 8748091 [details] [diff] [review]
icon patch

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

LGTM!

Please don't forget about the follow-up requests from comment 28.

::: devtools/client/jar.mn
@@ +290,5 @@
>      skin/images/tool-debugger.svg (themes/images/tool-debugger.svg)
>      skin/images/tool-debugger-paused.svg (themes/images/tool-debugger-paused.svg)
>      skin/images/debugging-addons.svg (themes/images/debugging-addons.svg)
>      skin/images/debugging-devices.svg (themes/images/debugging-devices.svg)
> +    skin/images/tabs-icon.svg (themes/images/tabs-icon.svg)

Nit: Placing "skin/images/tabs*" in the middle of "skin/images/debugging*" icons is weird, and would maybe make more sense somewhere else.

Then again all the "skin/images/debugging*" icons themselves are stranded in a sea of "skin/images/tool*" icons, and the whole file doesn't make much sense either, so I don't really care about sorting here (feel free to ignore this nit).

	Alexandre Poirot [:ochameau] Assignee
	Comment 42 • 8 years ago

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

	Alexandre Poirot [:ochameau] Assignee
	Comment 43 • 8 years ago

Rebased.

	Alexandre Poirot [:ochameau] Assignee
	Comment 44 • 8 years ago

Rebased against eslint fixes.

	Alexandre Poirot [:ochameau] Assignee
	Comment 45 • 8 years ago

Merged the icon.

	Alexandre Poirot [:ochameau] Assignee
	Comment 46 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/b3ed87fd43a2c3476c989f9dc9b2794f53ccd54d
Bug 1266128 - Rename about debugging Tab(s) to Panel(s). r=janx

https://hg.mozilla.org/integration/fx-team/rev/abbda387e2ef1a709fa9372b6d0897c1670c37ec
Bug 1266128 - Send tabListChanged messages when tab title changes. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/62c4097c0c01a1b23c81c9ad36bbe3d04f8e2302
Bug 1266128 - Show tabs in about:debugging. r=janx

	Tim Nguyen :ntim
	Comment 47 • 8 years ago

Comment on attachment 8748090 [details]
screenshot

LGTM

	Carsten Book [:Tomcat]
	Comment 48 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b3ed87fd43a2
https://hg.mozilla.org/mozilla-central/rev/abbda387e2ef
https://hg.mozilla.org/mozilla-central/rev/62c4097c0c01

	Chris Mills [:cmills]
	Comment 49 • 8 years ago

I've added a section about the Tabs page to 

https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Tabs

And added a note to the Fx 49 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools

Let me know if you think this needs anything else; thanks!

	Sayed [:LiquidO2]
	Comment 50 • 8 years ago

This bug was created to show "Tabs" in about:debugging. I have seen the implementation in latest release 49.0 with Ubuntu 16.04 64bit.

The bug's fix is now verified on latest release 49.0.

Build ID 	20160920074044
User Agent 	Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0