Last Comment Bug 714594 - Don't list app tabs in the all tabs menu
: Don't list app tabs in the all tabs menu
Status: RESOLVED FIXED
: ux-minimalism
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on: 714281
Blocks: 607226 776669 626854
  Show dependency treegraph
 
Reported: 2012-01-02 02:04 PST by Dão Gottwald [:dao]
Modified: 2012-07-23 13:10 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.45 KB, patch)
2012-01-04 02:42 PST, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (6.68 KB, patch)
2012-01-04 02:56 PST, Dão Gottwald [:dao]
mak77: review+
jboriss: ui‑review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-01-02 02:04:01 PST
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] 2012-01-04 02:42:37 PST
Created attachment 585690 [details] [diff] [review]
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] 2012-01-04 02:56:44 PST
Created attachment 585692 [details] [diff] [review]
patch v2

* removed the nasty tab property hack in browser-syncui.js :/
Comment 3 Marco Bonardo [::mak] 2012-01-04 08:06:18 PST
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] 2012-01-04 08:17:13 PST
> 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] 2012-01-04 08:40:32 PST
(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] 2012-01-04 08:50:13 PST
(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] 2012-01-04 09:04:44 PST
(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] 2012-01-04 12:05:12 PST
(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] 2012-01-05 06:00:43 PST
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] 2012-01-23 16:42:54 PST
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] 2012-01-24 05:07:30 PST
https://hg.mozilla.org/mozilla-central/rev/95a78ca23d9b

Note You need to log in before you can comment on or make changes to this bug.