Last Comment Bug 739093 - Remove the fullscreenflex
: Remove the fullscreenflex
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on:
Blocks: 739160 782043
  Show dependency treegraph
 
Reported: 2012-03-25 12:56 PDT by Dão Gottwald [:dao]
Modified: 2012-08-11 07:54 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (5.43 KB, patch)
2012-03-25 12:56 PDT, Dão Gottwald [:dao]
felipc: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-03-25 12:56:16 PDT
Created attachment 609147 [details] [diff] [review]
patch

It's useless.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-25 12:58:53 PDT
Was it ever useful? Some more context here would be nice.
Comment 2 Dão Gottwald [:dao] 2012-03-25 13:33:52 PDT
It wasn't ever useful. Its only purpose is to push the fullscreen window controls to the end in case there are no other flexible elements on the toolbar.
Comment 3 Dão Gottwald [:dao] 2012-03-26 01:11:47 PDT
http://forums.mozillazine.org/viewtopic.php?p=11855321#p11855321 motivated me to do this. As it happens, I've seen the back/forward buttons fail to connect with the location bar for no apparent reason on my aunt's computer two days ago. Resetting the toolbars fixed it.
Comment 4 Dão Gottwald [:dao] 2012-03-28 00:24:46 PDT
I should probably remove "#TabsToolbar > " here: http://hg.mozilla.org/mozilla-central/annotate/3e4735893504/browser/themes/winstripe/browser.css#l1151
Comment 5 :Felipe Gomes (needinfo me!) 2012-03-28 00:41:27 PDT
Comment on attachment 609147 [details] [diff] [review]
patch

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

::: browser/components/nsBrowserGlue.js
@@ -1268,5 @@
> -                                          "$1bookmarks-menu-button-container,fullscreenflex$2")
> -        }
> -        else {
> -          currentset += ",bookmarks-menu-button-container";
> -        }

Is removing this from the code correct? Even though fullscreenflex is going away, the purpose of this is to remove "fullscreenflex" from currentset when going from UI version 2 to 3, so if someone is on that upgrade path the code should still remove it.

Additionally, should we upgrade the UI version with this change to remove it again? FTR I have fullscreenflex in my currentset
Comment 6 Dão Gottwald [:dao] 2012-03-28 00:54:42 PDT
(In reply to Felipe Gomes (:felipe) from comment #5)
> Comment on attachment 609147 [details] [diff] [review]
> patch
> 
> Review of attachment 609147 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserGlue.js
> @@ -1268,5 @@
> > -                                          "$1bookmarks-menu-button-container,fullscreenflex$2")
> > -        }
> > -        else {
> > -          currentset += ",bookmarks-menu-button-container";
> > -        }
> 
> Is removing this from the code correct? Even though fullscreenflex is going
> away, the purpose of this is to remove "fullscreenflex" from currentset when
> going from UI version 2 to 3, so if someone is on that upgrade path the code
> should still remove it.

The purpose of this code isn't to remove fullscreenflex but to add bookmarks-menu-button-container.

> Additionally, should we upgrade the UI version with this change to remove it
> again? FTR I have fullscreenflex in my currentset

It doesn't hurt to have it in the currentset, toolbars handle non-existing ids just fine.
Comment 7 :Felipe Gomes (needinfo me!) 2012-03-28 01:27:11 PDT
Comment on attachment 609147 [details] [diff] [review]
patch

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

Oh that's true, that was not meant to remove fullscreenflex but to add the bookmarks menu. currentset will also get cleared up after any customization so that shouldn't be a worry.

And yeah we can remove #TabsToolbar > #window-controls
Comment 8 :Felipe Gomes (needinfo me!) 2012-03-28 01:28:41 PDT
There's only this addon on AMO that relies on this element: https://addons.mozilla.org/en-US/firefox/addon/bug582139/?src=ss
So giving a heads up to Alice.
Comment 9 Dão Gottwald [:dao] 2012-03-28 02:45:32 PDT
https://hg.mozilla.org/mozilla-central/rev/4f8b9ff09a8a
Comment 10 Dão Gottwald [:dao] 2012-03-29 05:59:26 PDT
Comment on attachment 609147 [details] [diff] [review]
patch

I'd like to land this fairly simple patch (mostly code removal) on Aurora for bug 739160. The feature being broken was new in Firefox 10.
Comment 11 rob64rock 2012-03-31 13:38:45 PDT
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 609147 [details] [diff] [review]
> patch
> 
> I'd like to land this fairly simple patch (mostly code removal) on Aurora
> for bug 739160. The feature being broken was new in Firefox 10.

Could they not land this fix on Fx 12 beta also? If not, What would be the ramifications if they did?
Comment 12 Dão Gottwald [:dao] 2012-03-31 14:02:19 PDT
> Could they not land this fix on Fx 12 beta also?

Theoretically, yes. Don't know what the chances are to get it approved.
Comment 13 Alex Keybl [:akeybl] 2012-04-02 10:49:19 PDT
Comment on attachment 609147 [details] [diff] [review]
patch

[Triage Comment]
Regression in Firefox 10 - approving for Aurora 13. I'm concerned that there's too little time left in Beta 12 to confidently determine that there is no add-on compatibility fallout (esp. outside of AMO). For that reason, let's let this land in FF13 first.

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