Closed Bug 1266128 Opened 4 years ago Closed 4 years ago

Show tabs in about:debugging

Categories

(DevTools :: about:debugging, defect, P2)

defect

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 15 obsolete files)

33.35 KB, image/png
ntim
: feedback+
janx
: feedback+
Details
19.45 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
10.49 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
81.68 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → poirot.alex
Depends on: 1233463
Attached patch patch v1 (obsolete) — Splinter Review
Mostly miss tests and svg icon.
Attached patch patch v2 (obsolete) — Splinter Review
Forgot to include l10n diff.
Attachment #8743368 - Attachment is obsolete: true
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.
Priority: -- → P2
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".
Attachment #8743378 - Flags: feedback+
(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.
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)
Attachment #8743947 - Flags: review?(janx)
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.
Attachment #8743963 - Flags: review?(jryans)
With the nits fixed and new file names.
Attachment #8743976 - Flags: review?(janx)
Attachment #8743378 - Attachment is obsolete: true
Missing a moz.build file.
Attachment #8743977 - Flags: review?(janx)
Attachment #8743976 - Attachment is obsolete: true
Attachment #8743976 - Flags: review?(janx)
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"?
Attachment #8743947 - Flags: review?(janx) → review+
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"?
Attachment #8743977 - Flags: review?(janx) → feedback+
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.
Attachment #8743963 - Flags: review?(jryans) → feedback+
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.)
Attached image Screenshot (obsolete) —
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.
Attachment #8745356 - Flags: feedback?(hholmes)
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?
Attachment #8746030 - Flags: review+
Attachment #8743947 - Attachment is obsolete: true
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.
Attachment #8746032 - Flags: review?(jryans)
Attachment #8743963 - Attachment is obsolete: true
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.
Attachment #8746033 - Flags: review?(janx)
Attachment #8743977 - Attachment is obsolete: true
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?
Attachment #8746032 - Flags: review?(jryans) → feedback+
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`.
Attachment #8746033 - Flags: review?(janx) → feedback+
(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!
Attachment #8746602 - Flags: review?(jryans)
Attachment #8746032 - Attachment is obsolete: true
Attached patch Show tabs in about:debugging. (obsolete) — Splinter Review
(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.
Attachment #8746604 - Flags: review?(janx)
Attachment #8746033 - Attachment is obsolete: true
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.
Attachment #8746604 - Flags: review?(janx) → review+
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!
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(hholmes)
(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/
Flags: needinfo?(ntim.bugs)
Thanks Tim!
Flags: needinfo?(hholmes)
Attached patch Tabs icon (obsolete) — Splinter Review
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.
Attachment #8747741 - Flags: review?(janx)
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?
Attachment #8747741 - Flags: review?(janx) → feedback+
Attached image tabs-icon.png (obsolete) —
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?
Attachment #8747922 - Flags: feedback?(ntim.bugs)
Comment on attachment 8745356 [details]
Screenshot

(This screenshot is now obsolete.)
Attachment #8745356 - Attachment is obsolete: true
Attachment #8745356 - Flags: feedback?(hholmes)
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.
Attachment #8747922 - Flags: feedback?(ntim.bugs)
Attached image screenshot
Are you ok with these icons?
Attachment #8747741 - Attachment is obsolete: true
Attachment #8747922 - Attachment is obsolete: true
Attachment #8748090 - Flags: feedback?(ntim.bugs)
Attachment #8748090 - Flags: feedback?(janx)
Attached patch icon patch (obsolete) — Splinter Review
Is yes, here is the related patch.
Attachment #8748091 - Flags: review?(janx)
Comment on attachment 8748090 [details]
screenshot

Looks great! Thank you Alex
Attachment #8748090 - Flags: feedback?(janx) → feedback+
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).
Attachment #8748091 - Flags: review?(janx) → review+
Blocks: 1269752
Blocks: 1269757
Attachment #8746030 - Attachment is obsolete: true
Rebased against eslint fixes.
Attachment #8748177 - Flags: review+
Attachment #8746602 - Attachment is obsolete: true
Merged the icon.
Attachment #8748178 - Flags: review+
Attachment #8746604 - Attachment is obsolete: true
Attachment #8748091 - Attachment is obsolete: true
Comment on attachment 8748090 [details]
screenshot

LGTM
Attachment #8748090 - Flags: feedback?(ntim.bugs) → feedback+
Depends on: 1272774
Blocks: 1212802
No longer depends on: 1212802
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!
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
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.