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

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

see bug 755593 comment 1

Comment 1

[Frank Yan (:fryn)]

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)]

I moved the review flag to Gavin, since he showed interest in getting this change in soon.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Also, why not just give the menu item an ID and access it directly?

Comment 4

[Dão Gottwald [:dao]]

We have three cmd_ToggleTabsOnTop menu items.

Comment 5

[Frank Yan (:fryn)]

(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]]

(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)]

(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]]

It's more relevant for any content rendered by Gecko.

Comment 9

[Frank Yan (:fryn)]

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]]

(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]]

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 12

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/89227104913a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/89227104913a

Comment 14

[Atlanx]

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.