Dão Gottwald [:dao] Assignee
	Description • 12 years ago

see bug 755593 comment 1

	Frank Yan (:fryn)
	Comment 1 • 12 years ago

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.

	Frank Yan (:fryn)
	Comment 2 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 12 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 4 • 12 years ago

We have three cmd_ToggleTabsOnTop menu items.

	Frank Yan (:fryn)
	Comment 5 • 12 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 6 • 12 years ago

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

	Frank Yan (:fryn)
	Comment 7 • 12 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 8 • 12 years ago

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

	Frank Yan (:fryn)
	Comment 9 • 12 years ago

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.

	Justin Dolske [:Dolske]
	Comment 10 • 12 years ago

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

	Justin Dolske [:Dolske]
	Comment 11 • 12 years ago

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* :)

	Dão Gottwald [:dao] Assignee
	Comment 12 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 13 • 12 years ago

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

	Atlanx
	Comment 14 • 11 years ago

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.