422420 - Revert home button move and related migration code

Status: VERIFIED FIXED

Description

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

Beltzner thinks it would be best to revert the changes we made to move the home button to the personal toolbar by default.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

We should probably get this done for beta. With apologies to Dao and Gavin and Mano for having to go through the hassle of moving it in the first place.

I don't think we need to move it back. Just need to remove the code that migrates it for existing profiles, and return it to the toolbar in new profile creation.

Comment 2

[Dão Gottwald [::dao]]

For the sake of a reasonably slim navigation toolbar, are we going to tackle bug 343396 instead?

Comment 3

[Mike Connor [:mconnor]]

No, we're not revisiting that this cycle.

Comment 4

[Mike Connor [:mconnor]]

Created attachment 309719 [details] [diff] [review]
remove migration code and revert defaultset changes

Comment 5

[Dão Gottwald [::dao]]

Even though the startup code will do that, the default class name should be updated too:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rv=1.445&mark=294#293

Comment 6

[Mike Connor [:mconnor]]

Ah, right, will fix on checkin.

Comment 7

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

Comment on attachment 309719 [details] [diff] [review]
remove migration code and revert defaultset changes

>Index: browser/components/nsBrowserGlue.js

>       var currentSet = this._rdf.GetResource("currentset");
>       var collapsed = this._rdf.GetResource("collapsed");

"collapsed" is now unused.

>       var navBar = this._rdf.GetResource("chrome://browser/content/browser.xul#nav-bar");
>       target = this._getPersist(navBar, currentSet);
>       if (target) {
>         let originalTarget = target;
> 
>         // add the new combined back and forward button
>         if (!/(?:^|,)unified-back-forward-button(?:$|,)/.test(target))
>           target = "unified-back-forward-button," + target;
> 
>         if (target != originalTarget)
>           this._setPersist(navBar, currentSet, target);

You can simplify this logic now that there's only one change to "target" (combine declaration/assignment to "target", call _setPersist directly rather than assigning to "target", get rid of "originalTarget").

Do we want to continue to support the special personal toolbar styling for the home button? If we go back to just having it be a "normal" toolbar button (i.e. no text label when on the personal toolbar) we could get rid of a bunch of code (updatePersonalToolbarStyle) and CSS (the home-button-specific .bookmark-item rules).

r=me with those and Dao's comment addressed.

Comment 8

[Mike Connor [:mconnor]]

Created attachment 310041 [details] [diff] [review]
for checkin, comments addressed

will land this tonight.

Comment 9

[Michael Lefevre]

Apologies if I'm adding noise to this bug, but I don't have time to test this thoroughly and thought someone else might...

I thought I would revert my home button to be in the default position (having decided I'd try and get used to the new position on the bookmarks bar)...

1. Have home button on bookmarks bar
2. Right click, customise, restore default set
3. Observe lack of home button on either bar

If you then repeat step 2, the home button will then appear next to stop on the navigation bar.

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031801 Minefield/3.0b5pre, on a profile created under 3.0b4

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Confirmed comment 9 - something's wonky here.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Actually, I think comment 9 is bug 423770. Reclosing.

Comment 12

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.   new profile and old profiles work as expected.