Closed Bug 422420 Opened 16 years ago Closed 16 years ago

Revert home button move and related migration code

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: Gavin, Assigned: mconnor)

Details

Attachments

(1 file, 1 obsolete file)

Beltzner thinks it would be best to revert the changes we made to move the home button to the personal toolbar by default.
Flags: blocking-firefox3?
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.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Target Milestone: --- → Firefox 3 beta5
For the sake of a reasonably slim navigation toolbar, are we going to tackle bug 343396 instead?
No, we're not revisiting that this cycle.
Assignee: nobody → mconnor
Attachment #309719 - Flags: review?(gavin.sharp)
Whiteboard: [has patch][needs review gavin]
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
Ah, right, will fix on checkin.
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.
Attachment #309719 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][needs review gavin] → [has patch][needs new patch]
will land this tonight.
Attachment #309719 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs new patch]
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
Confirmed comment 9 - something's wonky here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Actually, I think comment 9 is bug 423770. Reclosing.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.