415099 - browser.xul ids changed

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Doron Rosenberg (IBM)]

Bug 414836 changed some ids in browser.xul that will break extensions.  This either needs to be documented or the old ids added back.

Comment 1

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

We changed the IDs intentionally, because we need to reset toolbar customization state.

Comment 2

[Doron Rosenberg (IBM)]

(In reply to comment #1)
> We changed the IDs intentionally, because we need to reset toolbar
> customization state.
> 

I assumed so - this probably might have to be documented.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

This is a bad way to reset localstore.rdf stuff. But I assume there is no other way to do that?

Comment 4

[Philip Chee]

> But I assume there is no other way to do that?

You can do it with JS but then how do you make sure it's done only once? (Yes, yes, I know stick a flag in localstore.rdf)

Comment 6

[Jochen]

OK. I have only a few questions and I will stop shouting after those questions.

How does this change improve compatibility and maintainability for 
extension or theme developers?
The only thing I can see are broken extensions and/or themes. What will
happen next? Is the reset-toolbar-thing a moving target with "id changes" when needed?

Isn't it possible to do changes like this before late betas? 

"navigator-toolbox" was one of the really few constants you can rely on...

Comment 7

[Sylvain Pasche]

I updated the devmo page and added a step 3 (could be reverted if this doesn't stay).

Comment 8

[Jochen]

ID changes of this significance should be announced before checkeing them in.
Isn't there really another way to reset the customizations?

Comment 9

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

(In reply to comment #6)
> How does this change improve compatibility and maintainability for 
> extension or theme developers?

It doesn't, obviously. The cost of breaking addons needs to be weighed against the benefits of the new toolbar layout for all users. It's hard to make a judgment call without knowing how many add-ons are broken, how broken they are, and how hard the problem is to work around.

Comment 10

[Jochen]

(In reply to comment #9)
> It doesn't, obviously. The cost of breaking addons needs to be weighed against
> the benefits of the new toolbar layout for all users. It's hard to make a
> judgment call without knowing how many add-ons are broken, how broken they are,
> and how hard the problem is to work around.
> 
Got it.
But the 20080130-nightly used different IDs for the personal bookmarks folder
and the navigation toolbar. Right?
This was not working at all, so you decided to make it with the "navigator-toolbox" 
Please correct me if I'm wrong.

Could it be that there a some wishes which can not be realized without breaking
a lot of extensions and themes and that the devs are in a "hurry state"?

Comment 11

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

(In reply to comment #10)
> But the 20080130-nightly used different IDs for the personal bookmarks folder
> and the navigation toolbar. Right?
> This was not working at all, so you decided to make it with the
> "navigator-toolbox" 
> Please correct me if I'm wrong.

I'm not sure I understand what you're saying. We changed the navigation toolbar and personal bookmarks toolbar IDs because we're adding buttons to them and want them to appear for all users, even those with toolbars that are already customized. We needed to change the toolbox ID in bug 414836, because the icon size and type is persisted on the toolbox in addition to the toolbars, so resetting one and not the other leads to inconsistent indications in the customize toolbar panel on the first restart after picking up the changes.

Comment 12

[Jochen]

All I want to say is that you are currently doing things in a hurry,
as far as I can see. If I'm wrong just give a link to the site where
those id changes were announced.

Comment 13

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

(In reply to comment #12)
> All I want to say is that you are currently doing things in a hurry,
> as far as I can see. If I'm wrong just give a link to the site where
> those id changes were announced.

They weren't announced before the fact, you're right. Doron and mfinkle have since blogged about them, and mfinkle is working on an alternative solution that might allow us to revert the change.

Comment 14

[Ian McKellar]

perhaps I'm missing something, but couldn't we have done something like this to reset localstore state?
// get the rdf service
var rdfSvc = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
// get the localstore datasource
var localstore = rdfSvc.GetDataSource('rdf:local-store');
// get an nsIRDFResource for the navigator-toolbox item
var navigator_toolbox = rdfSvc.GetResource('chrome://browser/content/browser.xul#navigator-toolbox');
// enumerate its properties
var properties = localstore.ArLabelsOut(navigator_toolbox);
while(properties.hasMoreElements()) {
  var property = arc_labels.getNext();
  // enumerate the values for this property
  var targets = localstore.GetTargets(navigator_toolbox, property, true);
  while(targets.hasMoreElements()) {
    // clear the value
    localstore.Unassert(localstore, arc_label, targets.getNext());
  }
}

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #14)
> perhaps I'm missing something, but couldn't we have done something like this to
> reset localstore state?

I am trying to get a solution based on code like that.

Comment 16

[Dão Gottwald [:dao]]

I just had the chance to take a look at Mark's proof-of-concept migration code, and it did look very promising.

Comment 17

[Shawn Wilsher :sdwilsh]

When would that be done?  Would it impact Ts?

Comment 18

[Alfred Kayser]

So, will these ID's be reverted to the original values?

This change only to move the 'home' button caused problems with extensions and themes.

Comment 19

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

I imagine that it would be a one-time Ts hit, yes, but we're already doing some migration stuff for Places, aiui - can we hook into that so that we're not running two seperate migration paths?

Connor: I think we need a call here, one way or another, to determine if the cost of migration is worth the compatibility benefit. Blocking on that decision for Beta 3.

Comment 20

[Wladimir Palant]

Note that resetting toolbars is generally not a nice thing to do - users might have their customizations there, this will make the whole Firefox upgrade experience awkward. Also, some extensions (e.g. Adblock Plus) put their icon into the toolbar - that happens only once, when the extension is installed the first time. These extensions will need some code to recognize that toolbars were reset programmatically rather than manually - and run the icon insertion code again. The users will for sure not understand why the icon that they never had to add manually suddenly disappeared.

Mark, when you design a better solution, can you maybe consider this? One option would be replacing "back-button" by "unified-back-forward-button,back-button" in the stored property - this will both keep user's chosen toolbar layout and prevent the toolbar from becoming unusable if the user downgrades to Firefox 2 again.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #17)
> When would that be done?

What I've seen was more or less ready for prime time in that it would have the same effect as the id changes (without changing the ids). However, Mark said he'd like to make the migration smarter; e.g., instead of throwing away the currentsets, it would inject the back-forward item and move the home button explicitly.

> Would it impact Ts?

Besides the one-time migration, there would be a presumably cheap preference read on every startup.

Comment 22

[Ian McKellar]

I'd be happy to contribute RDF-fu if anyone needs help. I'll be on #developers (ianloic) all day.

Comment 23

[Eric Shepherd [:sheppy]]

I've applied additional documentation changes.  They'll need to be rolled back if we undo this change.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: reverts id's and uses RDF service to try to migrate the persisted values

WIP v1: This patch sets the toolbox, nav toolbar and personal bookmark toolbar id's back to their previous values. It also adds some code to nsBrowserGlue.js to attempt a programmatic migration of the persisted data.

This patch is a brute force, first attempt to get it right. It removes all persisted data for the given XUL elements.

The next patch will only update the required data and *not* remove all persisted data.

I tested this by creating a FF2 profile with an extra button in the navigation toolbar and an extra button in the personal bookmark toolbar. When running FF3 (with patch) on the profile, the toolbars are correctly shown with the unified forward-back in the nav toolbar and the home button in the personal toolbar. As expected, the user customized buttons were dropped. The next patch should keep any user customized buttons when upgrading.

The migration is only attempted once using a pref check.

Comment 25

[Philip Chee]

> -    gNavToolbox = document.getElementById("browser-toolbox");
> +    gNavToolbox = document.getElementById("navigator-toolbox");
.....
> -    InitWithToolbox(window.parent.document.getElementById("browser-toolbox"));
> +    InitWithToolbox(window.parent.document.getElementById("navigator-toolbox"));

Hmm. How about something like:
InitWithToolbox(window.parent.gNavtoolbox);

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #25)

> 
> Hmm. How about something like:
> InitWithToolbox(window.parent.gNavtoolbox);
> 

Maybe, but I'd like to keep this patch as minimally simple as possible, rolling back whatever I can.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #25)
> > -    InitWithToolbox(window.parent.document.getElementById("browser-toolbox"));
> > +    InitWithToolbox(window.parent.document.getElementById("navigator-toolbox"));
> 
> Hmm. How about something like:
> InitWithToolbox(window.parent.gNavtoolbox);

getNavToolbox() would be the save way, i.e.:
InitWithToolbox(parent.getNavToolbox());

Comment 28

[Philip Chee]

(In reply to comment #27)
> getNavToolbox() would be the save way, i.e.:
> InitWithToolbox(parent.getNavToolbox());

s/save/safe/

We should discuss this in a separate polish bug once all the urgent stuff has been resolved, but just to get in a point or two:

gToolbox or getToolbox() would in future make it easier to unfork Thunderbird which (currently uses getMailToolbox) and would reduce the amount of forking I would have to do in SeaMonkey. And it would help isolate the customize toolbar code from specific apps that call into it.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v2 - same as first patch, but it only changes what it needs

This patch is almost the same as the first patch, except the code only modifies the RDF persist values required to get the new UI working:
* removes "mode" and "iconsize" from the navigator-toolbox, nav-bar and PersonalToolbar
* replaces the "back-button" and "forward-button" on the nav-bar, but only if the buttons are side-by-side
* prepends the "home-button" to the PersonalToolbar

Everything else is left alone, so any user toolbar customizations are kept.

NOTE: feedback on the pref name is welcome.

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #20)

> Mark, when you design a better solution, can you maybe consider this? One
> option would be replacing "back-button" by
> "unified-back-forward-button,back-button" in the stored property - this will
> both keep user's chosen toolbar layout and prevent the toolbar from becoming
> unusable if the user downgrades to Firefox 2 again.
> 

I am hoping to get this quick fix in for beta 3, without any behavior changes from the original fix. If this is something that the UI guys can be convinced to do later, we can patch it in a separate bug.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v3 - with some new tweaks and checks

This patch only changes what it needs to, but it changes less than the v2 patch.
* iconsize and mode are *not* altered in any way. this is better for the user and not needed to get the functionality to work.
* the home button is only added to the PersonalToolbar if it was found in the nav-bar. If found it is removed from the nav-bar and prepended to the PersonalToolbar.
* the unified-back-forward-button is always prepended to the nav-bar. The old back-button and forward-button are *not* removed but will be ignored by FF3 and clean up by any future customization by the user.

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 301097 [details] [diff] [review]
v3 - with some new tweaks and checks

Dave - can you check out the RDF API usage for correctness?

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 301097 [details] [diff] [review]
v3 - with some new tweaks and checks

I'll trust gavin to check the id changes elsewhere and just touch nsBrowserGlue.js

>Index: browser/components/nsBrowserGlue.js
>===================================================================
>@@ -401,16 +404,90 @@ BrowserGlue.prototype = {
>   _shutdownPlaces: function bg__shutdownPlaces() {
>     // backup bookmarks to bookmarks.html
>     var importer =
>       Cc["@mozilla.org/browser/places/import-export-service;1"].
>       getService(Ci.nsIPlacesImportExportService);
>     importer.backupBookmarksFile();
>   },
> 
>+  _migrateUI: function bg__migrateUI() {
>+    var migration = 0;
>+    try {
>+      var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                       getService(Ci.nsIPrefBranch);
>+      migration = prefBranch.getIntPref("browser.migration.version");
>+    } catch(ex) {}
>+
>+    if (migration == 0) {

This will attempt to migrate even for new profiles which seems wrong. It won't cause any issues, but just slow down the startup on first run. If you added this preference to the prefs.js default file then that would solve it, there may be a better way though.

>+      // grab the localstore.rdf and make changes needed for new UI
>+      var rdfSvc = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
>+      var localStore = rdfSvc.GetDataSource("rdf:local-store");
>+
>+      var foundHome = false;
>+
>+      // get an nsIRDFResource for the nav-bar item
>+      var navBar = rdfSvc.GetResource("chrome://browser/content/browser.xul#nav-bar");
>+      if (navBar) {

No need for this test I think (or the equivalent below). GetResource always returns a resource, unless it fails in which case it will throw.

>+        var currentSet = rdfSvc.GetResource("currentset");
>+        var target = this._getPersist(localStore, navBar, currentSet);
>+        if (target) {
>+          foundHome = (target.indexOf("home-button") != -1);
>+          if (foundHome)
>+            target = target.replace("home-button", "");
>+          target = "unified-back-forward-button," + target;
>+          this._setPersist(localStore, navBar, currentSet, rdfSvc.GetLiteral(target));
>+        }
>+      }
>+
>+      // get an nsIRDFResource for the PersonalToolbar item
>+      var personalBar = rdfSvc.GetResource("chrome://browser/content/browser.xul#PersonalToolbar");
>+      if (personalBar) {
>+        var currentSet = rdfSvc.GetResource("currentset");
>+        var target = this._getPersist(localStore, personalBar, currentSet);
>+        if (target && foundHome && target.indexOf("home-button") == -1)

Do we need the indexOf("home-button")? Can the home button really appear on both the navbar and the bookmarks bar?

>+          this._setPersist(localStore, personalBar, currentSet,
>+                           rdfSvc.GetLiteral("home-button," + target));
>+      }
>+
>+      // force the RDF to be saved
>+      localStore.QueryInterface(Ci.nsIRDFRemoteDataSource).Flush();
>+    }
>+
>+    // update the migration version
>+    prefBranch.setIntPref("browser.migration.version", 1);
>+  },
>+
>+  _getPersist: function bg__getPersist(aDataSource, aSource, aProperty) {
>+    var target = aDataSource.GetTarget(aSource, aProperty, true);
>+    if (!target)
>+      return null;
>+
>+    if (target instanceof Ci.nsIRDFLiteral)
>+      return target.Value;
>+
>+    return null;
>+  },
>+
>+  _setPersist: function bg__setPersist(aDataSource, aSource, aProperty, aTarget) {
>+    try {
>+      if (aDataSource.hasArcOut(aSource, aProperty)) {
>+       var oldTarget = aDataSource.GetTarget(aSource, aProperty, true);

The hasArcOut is a wasted operation really. Just use GetTarget and compare the result to null.
Alternatively it would be more efficient to just pass in the old target to save us a further operation, but that complicates the migrateUI function a little, get gavin's opinion on how much we want to save time here.
Nit: indentation is wrong.

>+        if (aTarget)
>+          aDataSource.Change(aSource, aProperty, oldTarget, aTarget);
>+        else
>+          aDataSource.Unassert(aSource, aProperty, oldTarget);
>+      }
>+      else {
>+        aDataSource.Assert(aSource, aProperty, aTarget, true);
>+      }
>+    }
>+    catch(ex) {}
>+  },
>+
>   // ------------------------------
>   // public nsIBrowserGlue members
>   // ------------------------------
>   
>   sanitize: function(aParentWindow) 
>   {
>     this.Sanitizer.sanitize(aParentWindow);
>   },

This is fine as is really, just a few minor changes to make. I would like to see whether we can avoid hitting the migration path completely for new profiles though.

Comment 34

[Wladimir Palant]

Comment on attachment 301097 [details] [diff] [review]
v3 - with some new tweaks and checks

Additional comments:

> +        var currentSet = rdfSvc.GetResource("currentset");

No need to do this twice. Just get the resource at the beginning and use it twice.

> +    if (target instanceof Ci.nsIRDFLiteral)

I don't think you can do it like this, it isn't guaranteed to work. You have to QI the RDF node - and catch the exception that might happen.

> +      if (aDataSource.hasArcOut(aSource, aProperty)) {

I would rather check whether oldTarget is not null and do Unassert then. After than I would do Assert if we have a new value and drop Change entirely.

> +  _setPersist: function bg__setPersist(aDataSource, aSource, aProperty, aTarget) {

I would make aTarget a string - for consistency, since _getPersist returns a string.

Generally: looks fine, the points above are only nits.

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> I don't think you can do it like this, it isn't guaranteed to work. You have to
> QI the RDF node - and catch the exception that might happen.
That's incorrect.  XPConnect QI's it for you when you use instanceof, and adds the appropriate methods and properties to the object if it succeeded.  instanceof is also faster last I knew.

Comment 36

[Dave Townsend [:mossop]]

(In reply to comment #34)
> > +      if (aDataSource.hasArcOut(aSource, aProperty)) {
> 
> I would rather check whether oldTarget is not null and do Unassert then. After
> than I would do Assert if we have a new value and drop Change entirely.

While it is true that Change is only going to be marginally faster than Unassert followed by Assert, I think the use of Change is good and explains what is going on.

> > +  _setPersist: function bg__setPersist(aDataSource, aSource, aProperty, aTarget) {
> 
> I would make aTarget a string - for consistency, since _getPersist returns a
> string.

Forgot to mention this, should do it, unless you are switching to passing in the old value in which case you are probably better off sticking with literals all round.

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #33)
> Do we need the indexOf("home-button")? Can the home button really appear on
> both the navbar and the bookmarks bar?

It's not needed on trunk (as of bug 414864), but it's needed for going back to branch.

Comment 38

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v4 - updated patch based on reviews

updated this patch based on the reviews. carrying over dtownsend's r+

Comment 39

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #33)
> 
> >Index: browser/components/nsBrowserGlue.js
> >===================================================================
> >@@ -401,16 +404,90 @@ BrowserGlue.prototype = {
> >   _shutdownPlaces: function bg__shutdownPlaces() {
> >     // backup bookmarks to bookmarks.html
> >     var importer =
> >       Cc["@mozilla.org/browser/places/import-export-service;1"].
> >       getService(Ci.nsIPlacesImportExportService);
> >     importer.backupBookmarksFile();
> >   },
> > 
> >+  _migrateUI: function bg__migrateUI() {
> >+    var migration = 0;
> >+    try {
> >+      var prefBranch = Cc["@mozilla.org/preferences-service;1"].
> >+                       getService(Ci.nsIPrefBranch);
> >+      migration = prefBranch.getIntPref("browser.migration.version");
> >+    } catch(ex) {}
> >+
> >+    if (migration == 0) {
> 
> This will attempt to migrate even for new profiles which seems wrong. It won't
> cause any issues, but just slow down the startup on first run. If you added
> this preference to the prefs.js default file then that would solve it, there
> may be a better way though.

I added a pref to firefox.js which defaults to a migration version of 1, which will skip the migration attempt on new profiles.

> 
> >+      // grab the localstore.rdf and make changes needed for new UI
> >+      var rdfSvc = Cc["@mozilla.org/rdf/rdf-service;1"].getService(Ci.nsIRDFService);
> >+      var localStore = rdfSvc.GetDataSource("rdf:local-store");
> >+
> >+      var foundHome = false;
> >+
> >+      // get an nsIRDFResource for the nav-bar item
> >+      var navBar = rdfSvc.GetResource("chrome://browser/content/browser.xul#nav-bar");
> >+      if (navBar) {
> 
> No need for this test I think (or the equivalent below). GetResource always
> returns a resource, unless it fails in which case it will throw.

removed the tests

> 
> >+        var currentSet = rdfSvc.GetResource("currentset");
> >+        var target = this._getPersist(localStore, navBar, currentSet);
> >+        if (target) {
> >+          foundHome = (target.indexOf("home-button") != -1);
> >+          if (foundHome)
> >+            target = target.replace("home-button", "");
> >+          target = "unified-back-forward-button," + target;
> >+          this._setPersist(localStore, navBar, currentSet, rdfSvc.GetLiteral(target));
> >+        }
> >+      }
> >+
> >+      // get an nsIRDFResource for the PersonalToolbar item
> >+      var personalBar = rdfSvc.GetResource("chrome://browser/content/browser.xul#PersonalToolbar");
> >+      if (personalBar) {
> >+        var currentSet = rdfSvc.GetResource("currentset");
> >+        var target = this._getPersist(localStore, personalBar, currentSet);
> >+        if (target && foundHome && target.indexOf("home-button") == -1)
> 
> Do we need the indexOf("home-button")? Can the home button really appear on
> both the navbar and the bookmarks bar?

just making safe for going back to branch

> 
> >+          this._setPersist(localStore, personalBar, currentSet,
> >+                           rdfSvc.GetLiteral("home-button," + target));
> >+      }
> >+
> >+      // force the RDF to be saved
> >+      localStore.QueryInterface(Ci.nsIRDFRemoteDataSource).Flush();
> >+    }
> >+
> >+    // update the migration version
> >+    prefBranch.setIntPref("browser.migration.version", 1);
> >+  },
> >+
> >+  _getPersist: function bg__getPersist(aDataSource, aSource, aProperty) {
> >+    var target = aDataSource.GetTarget(aSource, aProperty, true);
> >+    if (!target)
> >+      return null;
> >+
> >+    if (target instanceof Ci.nsIRDFLiteral)
> >+      return target.Value;
> >+
> >+    return null;
> >+  },
> >+
> >+  _setPersist: function bg__setPersist(aDataSource, aSource, aProperty, aTarget) {
> >+    try {
> >+      if (aDataSource.hasArcOut(aSource, aProperty)) {
> >+       var oldTarget = aDataSource.GetTarget(aSource, aProperty, true);
> 
> The hasArcOut is a wasted operation really. Just use GetTarget and compare the
> result to null.
> Alternatively it would be more efficient to just pass in the old target to save
> us a further operation, but that complicates the migrateUI function a little,
> get gavin's opinion on how much we want to save time here.
> Nit: indentation is wrong.

dropped hasArcOut and just use GetTarget as suggested. Fix indentation.

> > +  _setPersist: function bg__setPersist(aDataSource, aSource, aProperty, aTarget) {
> 
> I would make aTarget a string - for consistency, since _getPersist returns a
> string.

done - _setPersist now takes a string and converts to a literal

> > +        var currentSet = rdfSvc.GetResource("currentset");
>
> No need to do this twice. Just get the resource at the beginning and use it twice.

done

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #39)
> I added a pref to firefox.js which defaults to a migration version of 1, which
> will skip the migration attempt on new profiles.

I'm not sure I understand this. Won't the pref be there after updating from Firefox 2 to Firefox 3, before you get the chance to migrate the UI?

Comment 41

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #40)
> (In reply to comment #39)
> > I added a pref to firefox.js which defaults to a migration version of 1, which
> > will skip the migration attempt on new profiles.
> 
> I'm not sure I understand this. Won't the pref be there after updating from
> Firefox 2 to Firefox 3, before you get the chance to migrate the UI?
> 

Yeah, Mossop caught this too. I need to add the pref to prefs.js

New patch coming up

Comment 42

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v5 - same patch with no default pref, but a dirty flag used to flush datasource

I was not happy with any default pref solution, used to *not* execute the migration code in a new profile. I added a flag to skip flushing the datasource in a new profile (where the targets would be null).

Any Ts hit will only be a one time hit and hopefully it will be small. Let's get this reviewed and try it out.

Comment 43

[Dave Townsend [:mossop]]

(In reply to comment #42)
> I was not happy with any default pref solution, used to *not* execute the
> migration code in a new profile. I added a flag to skip flushing the datasource
> in a new profile (where the targets would be null).

The problem we have is that we will hit the path for migration == 0 for two distinct cases. For profiles pre-Firefox 3, and for new profiles. Right now that is not really an issue, the code deals well. Come Firefox 5, who knows.

If it isn't going to be changed please at least add a comment explaining that we go through migration for both of those cases.

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #42)
> I was not happy with any default pref solution, used to *not* execute the
> migration code in a new profile.

Why? What's the use case for migrating a new profile? Is there a bug in our default toolbar setup that we should fix?

Comment 45

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

To be clear, the current patch doesn't do any actual "migration" for new profiles. It does check to see whether it needs to, but since neither of the checks will return true, dirty will be false and it will effectively be a no-op.

I suppose we could potentially add this pref to prefs.js? I don't really like that approach because it relies on us copying over that pref file for all new profiles (which I'm not sure is guaranteed, and IIRC bsmedberg wants to get rid of it).

Comment 46

[Eric Shepherd [:sheppy]]

Just to be sure of what I'm doing before I re-twiddle the docs -- we've rolled
out the changes to the element IDs and that's a done deal?

Comment 47

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

(In reply to comment #46)
> Just to be sure of what I'm doing before I re-twiddle the docs -- we've rolled
> out the changes to the element IDs and that's a done deal?

No done deals. We want to take mfinkle's patch which will remove the need to change the IDs, after which we will revert the ID changes (which would leave them the same as in Firefox 2).

Comment 48

[Eric Shepherd [:sheppy]]

OK, I'll temporarily leave in the doc changes on the element IDs then and revisit those once this patch is in, if it goes.  Thanks.

Comment 49

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

Comment on attachment 301219 [details] [diff] [review]
v5 - same patch with no default pref, but a dirty flag used to flush datasource

Missing one personaltoolbar ID change in browser.js's updatePersonalToolbarStyle.

>Index: browser/components/nsBrowserGlue.js

>+  _migrateUI: function bg__migrateUI() {

>+    if (migration == 0) {

Add a comment explaining that this codepath should always migrate pre-Fx3 profiles to whatever the current state is? If we change the default toolbar layout and still care about users updating directly to that version from Firefox 2, we'll need to change this code.

>+      // get an nsIRDFResource for the PersonalToolbar item
>+      var personalBar = this._rdf.GetResource("chrome://browser/content/browser.xul#PersonalToolbar");
>+      target = this._getPersist(localStore, personalBar, currentSet);
>+      if (target && foundHome && target.indexOf("home-button") == -1) {
>+        this._setPersist(localStore, personalBar, currentSet, "home-button," + target);
>+        dirty = true;
>+      }

You can move up the foundHome check to avoid the GetResource and _getPersist calls if it's false, right?

>+  _getPersist: function bg__getPersist(aDataSource, aSource, aProperty) {
>+    var target = aDataSource.GetTarget(aSource, aProperty, true);
>+    if (!target)
>+      return null;
>+
>+    if (target instanceof Ci.nsIRDFLiteral)
>+      return target.Value;

nit: these checks are redundant, null will never be instanceof nsIRDFLiteral.

Comment 50

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

Comment on attachment 301219 [details] [diff] [review]
v5 - same patch with no default pref, but a dirty flag used to flush datasource

a=beltzner

Should we clobber nightlies and see if this works out for people?

Comment 51

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

(that a was with nits, natch)

Comment 52

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: v6 - patch for checkin, with nits fixed

Here is the patch for checkin. It has gavin's review nits addressed.

Comment 53

[Reed Loden [:reed]]

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.51; previous revision: 1.50
done
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.57; previous revision: 1.56
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.958; previous revision: 1.957
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.425; previous revision: 1.424
done
Checking in browser/base/content/customizeToolbarSheet.js;
/cvsroot/mozilla/browser/base/content/customizeToolbarSheet.js,v  <--  customizeToolbarSheet.js
new revision: 1.4; previous revision: 1.3
done
Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.173; previous revision: 1.172
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.121; previous revision: 1.120
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.166; previous revision: 1.165
done