714594 - Don't list app tabs in the all tabs menu

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

The all tabs menu was originally added along with tab overflow. App tabs aren't part of this and are always easily accessible by design.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

noteworthy changes:

* replaced the per-menuitem command event handlers with a single command event handler

* removed the TabOpen handler, since we don't support opening tabs while the popup is open. We also don't handle TabPinned, TabUnpinned, TabMove, TabHide and TabShow.

* removed the aEvent.isTrusted check since we cannot get content events here

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v2

* removed the nasty tab property hack in browser-syncui.js :/

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

some questions:
1. can we receive TabClose commands while the popup is open? how does that differ from TabOpen?
2. Rather than relying on external callers to use _removeOnPopupHidden, maybe could be inverted, and add an attribute to static popup content in browser.xul.
3. in _updateTabsVisibilityStatus there is a special check for "tabs from other computers", but since now they don't have anymore a fake .tab property, the first check will already filter them out, so this additional check is useless.

Comment 4

[Dão Gottwald [:dao]]

> 1. can we receive TabClose commands while the popup is open?

It would be hard for a user to actively close a tab, but a page could close itself or an add-on could do strange things.

> how does that differ from TabOpen?

It's easier to support, and the failure case is worse.

> 2. Rather than relying on external callers to use _removeOnPopupHidden,
> maybe could be inverted, and add an attribute to static popup content in
> browser.xul.

I don't really care which way we do this, except that I'd need to find a reasonable attribute name. Suggestions?

> 3. in _updateTabsVisibilityStatus there is a special check for "tabs from
> other computers", but since now they don't have anymore a fake .tab
> property, the first check will already filter them out, so this additional
> check is useless.

Will fix.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #4)
> > how does that differ from TabOpen?
> 
> It's easier to support, and the failure case is worse.

Worse in the sense that we'd have a menuitem pointing to a nonexistent tab? While not adding one nothing bad happens, it is just missing?

> > 2. Rather than relying on external callers to use _removeOnPopupHidden,
> > maybe could be inverted, and add an attribute to static popup content in
> > browser.xul.
> 
> I don't really care which way we do this, except that I'd need to find a
> reasonable attribute name. Suggestions?

Heh, a "static" or "persistent" attribute name sucks. "persistonpopuphidden" is clearer but a bit verbose.
Another alternative to not add sucky named attributes, may be to add a constructor that adds a _static property to existing items.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > > how does that differ from TabOpen?
> > 
> > It's easier to support, and the failure case is worse.
> 
> Worse in the sense that we'd have a menuitem pointing to a nonexistent tab?
> While not adding one nothing bad happens, it is just missing?

Yep.

> > > 2. Rather than relying on external callers to use _removeOnPopupHidden,
> > > maybe could be inverted, and add an attribute to static popup content in
> > > browser.xul.
> > 
> > I don't really care which way we do this, except that I'd need to find a
> > reasonable attribute name. Suggestions?
> 
> Heh, a "static" or "persistent" attribute name sucks. "persistonpopuphidden"
> is clearer but a bit verbose.
> Another alternative to not add sucky named attributes, may be to add a
> constructor that adds a _static property to existing items.

This seems more fragile, as external code would need to be careful about when to inject custom items. At that point, I'd just keep _removeOnPopuphidden.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dão Gottwald [:dao] from comment #6)
> This seems more fragile, as external code would need to be careful about
> when to inject custom items. At that point, I'd just keep
> _removeOnPopuphidden.

The most common behavior is to add items on capture popupshowing, but may be I'm too optimist! So, if we need defense against adding items before the constructor, the only ways are the attribute or _removeOnPopuphidden. I was preferring the attribute because the most common operation is to add content that has to be removed, so having to set the property on each item is more error-prone.
What about "staticcontent"?

Comment 8

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > This seems more fragile, as external code would need to be careful about
> > when to inject custom items. At that point, I'd just keep
> > _removeOnPopuphidden.
> 
> The most common behavior is to add items on capture popupshowing, but may be
> I'm too optimist!

We only have one external data point. Realistically, people may want to add items that should or shouldn't be removed at arbitrary points in time.

> So, if we need defense against adding items before the
> constructor, the only ways are the attribute or _removeOnPopuphidden. I was
> preferring the attribute because the most common operation is to add content
> that has to be removed, so having to set the property on each item is more
> error-prone.

It's not hugely more common, unless you count the tab menu items added in the loop as single instances...

> What about "staticcontent"?

Still doesn't sounds specific enough to me. The advantage of using a property is that we can use the underscore to indicate it's something private rather than a general XUL feature.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

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

(In reply to Dão Gottwald [:dao] from comment #8)
> The advantage of using a
> property is that we can use the underscore to indicate it's something
> private rather than a general XUL feature.

yep, that's why I thought of the constructor approach adding a property to the less frequent case of static entries.

Ok, please just fix point 3, and eventually add a comment about the fact we support TabClose but not TabOpen to avoid leaving zombie menuitems around.

Eventually we'll evaluate the property change in future, if we get an healthier idea.

Comment 10

[Dão Gottwald [:dao]]

I used the first patch, since "Tabs from other computers" is gone.

> 3. in _updateTabsVisibilityStatus there is a special check for "tabs from
> other computers", but since now they don't have anymore a fake .tab
> property, the first check will already filter them out, so this additional
> check is useless.

Fixed.

http://hg.mozilla.org/integration/mozilla-inbound/rev/95a78ca23d9b

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/95a78ca23d9b