Last Comment Bug 755646 - Hide UI for toggling tabs on top unless the user disabled tabs on top
: Hide UI for toggling tabs on top unless the user disabled tabs on top
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: 755593 763294
  Show dependency treegraph
 
Reported: 2012-05-16 00:44 PDT by Dão Gottwald [:dao]
Modified: 2013-07-01 08:36 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.71 KB, patch)
2012-05-16 00:44 PDT, Dão Gottwald [:dao]
dolske: review+
fryn: feedback+
Details | Diff | Review

Description Dão Gottwald [:dao] 2012-05-16 00:44:45 PDT
Created attachment 624318 [details] [diff] [review]
patch

see bug 755593 comment 1
Comment 1 Frank Yan (:fryn) 2012-05-18 16:33:16 PDT
Comment on attachment 624318 [details] [diff] [review]
patch

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

::: browser/base/content/browser.js
@@ +5467,5 @@
>      Services.prefs.addObserver(this._prefName, this, false);
> +
> +    // Only show the toggle UI if the user disabled tabs on top.
> +    if (Services.prefs.getBoolPref(this._prefName)) {
> +      for (item of document.querySelectorAll("menuitem[command=cmd_ToggleTabsOnTop]"))

Use `let` here.

What is the benefit of using a `for` loop here that runs in the common case (of [tabsontop=true]) instead of having hidden=true on the <menuitem/>s in the markup by default.
Comment 2 Frank Yan (:fryn) 2012-05-18 16:34:57 PDT
I moved the review flag to Gavin, since he showed interest in getting this change in soon.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-18 16:57:24 PDT
Also, why not just give the menu item an ID and access it directly?
Comment 4 Dão Gottwald [:dao] 2012-05-18 18:43:14 PDT
We have three cmd_ToggleTabsOnTop menu items.
Comment 5 Frank Yan (:fryn) 2012-05-18 23:02:17 PDT
(In reply to Dão Gottwald [:dao] from comment #4)

That doesn't answer my question, which could be rephrased as: is it better to have a <menuitem/> removed from the document at load than have it be declared as [hidden=true]?
Comment 6 Dão Gottwald [:dao] 2012-05-19 00:07:05 PDT
(In reply to Frank Yan (:fryn) from comment #5)
> That doesn't answer my question,

I replied to comment 3.

> which could be rephrased as: is it better
> to have a <menuitem/> removed from the document at load than have it be
> declared as [hidden=true]?

I expect querySelectorAll to be cheap, so I don't think it matters.
Comment 7 Frank Yan (:fryn) 2012-05-19 00:12:31 PDT
(In reply to Dão Gottwald [:dao] from comment #6)

Okay, but given that the tabs-on-top is the default, we should initialize our UI in a state that reflects that, i.e. flipping the `if` condition and setting [hidden=true] in the markup. Dolske recommended this kind of thing when I first wrote the combined stop/go/reload code. See bug 544816 comment 54.
Comment 8 Dão Gottwald [:dao] 2012-05-19 00:22:32 PDT
It's more relevant for any content rendered by Gecko.
Comment 9 Frank Yan (:fryn) 2012-05-19 00:31:21 PDT
Comment on attachment 624318 [details] [diff] [review]
patch

(In reply to Dão Gottwald [:dao] from comment #8)
> It's more relevant for any content rendered by Gecko.

Good point.

feedback+ with the `let` added.
Comment 10 Justin Dolske [:Dolske] 2012-05-23 15:44:51 PDT
(In reply to Frank Yan (:fryn) from comment #5)

> That doesn't answer my question, which could be rephrased as: is it better
> to have a <menuitem/> removed from the document at load than have it be
> declared as [hidden=true]?

As a general rule I'd probably suggest using hidden=true, since that's what we do with other menus (most infamously the main context menu :). But since we're talking about actually _removing_ this at some point, we might as well just start doing so now. (EG, if an add-on makes use of it, it's gonna have to change sooner or later).
Comment 11 Justin Dolske [:Dolske] 2012-05-23 15:47:31 PDT
Comment on attachment 624318 [details] [diff] [review]
patch

(In reply to Frank Yan (:fryn) from comment #2)
> I moved the review flag to Gavin, since he showed interest in getting this
> change in soon.

*yoink* :)
Comment 13 Ed Morley [:emorley] 2012-05-24 09:29:40 PDT
https://hg.mozilla.org/mozilla-central/rev/89227104913a
Comment 14 Atlanx 2013-06-30 23:18:47 PDT
Why you doing this?

I don't like that the tabs are on top (over the address bar) because it makes the disctance you have to move the mouse longer and this makes it for me less efficient to use the browser.

So, please bring at least the option back to set the tabs under the address bar.

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