Closed Bug 755646 Opened 8 years ago Closed 8 years ago

Hide UI for toggling tabs on top unless the user disabled tabs on top

Categories

(Firefox :: Menus, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 15

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
see bug 755593 comment 1
Attachment #624318 - Flags: review?(dolske)
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.
Attachment #624318 - Flags: review?(dolske) → review?(gavin.sharp)
I moved the review flag to Gavin, since he showed interest in getting this change in soon.
Also, why not just give the menu item an ID and access it directly?
We have three cmd_ToggleTabsOnTop menu items.
(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]?
(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.
(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.
It's more relevant for any content rendered by Gecko.
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.
Attachment #624318 - Flags: feedback+
(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 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* :)
Attachment #624318 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/89227104913a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 763294
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.
You need to log in before you can comment on or make changes to this bug.