Created attachment 323093 [details] distribution.ini for testcase When any link items are defined in the [BookmarksMenu] section of <appdir>/distribution/distribution.ini, the following error is thrown on profile creation, and no bookmark customizations appear in either the Bookmarks Toolbar or Bookmarks menu. Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.insertBookmark]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/modules/distribution.js :: DIST_parseBookmarksSection :: line 189" data: no] Source File: file:///C:/Program%20Files/Mozilla%20Firefox/modules/distribution.js Line: 189 Removing all link items from the [BookmarksMenu] section permits the customizations made in the [BookmarksToolbar] section to be applied, but any other customizations in the [BookmarksMenu] section (i.e. folders) will not appear in the Bookmarks Menu. Steps to reproduce: - Create a "distribution" directory off the application directory - Copy the "distribution.ini for testcase" attachment to the distribution directory - Start Firefox3 with a new profile Expected results: Firefox 3 with a test link and folder in the Bookmarks Toolbar, and a test link and folder in the Bookmarks Menu Actual results: Firefox 3 with default Bookmarks Toolbar and Menu (removing the [BookmarksMenu] section and restarting with a new profile will result in the BookmarksToolbar being populated as expected with one link item and one folder item)
the call to insertBookmark call in distribution.js looks correct, and that API has not changed in a very long time. i'm not familiar with how the ini is being parsed etc, so cc'ing thunder, author of this code.
The distribution module is using 'bmSvc.bookmarksRoot' to get the menu root (where 'bmSvc' is nsINavBookmarksService), but that property was renamed to 'bookmarksMenuFolder' a while back: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsINavBookmarksService.idl&branch=&root=/cvsroot&subdir=/mozilla/toolkit/components/places/public&command=DIFF_FRAMESET&rev1=1.49&rev2=1.50
Created attachment 328045 [details] [diff] [review] [checked-in] distribution menu folder fix it's easy when someone tells you how
we'll likely also need approval flags on this, leaving that up to the module owner.
Comment on attachment 328045 [details] [diff] [review] [checked-in] distribution menu folder fix Thanks! Looks good.
Need review from benjamin, unless mconnor/bsmedberg want to update the reviewer page to make thunder an appropriate reviewer.
I'd ask that Benjamin review, please.
Does browser/modules/Distribution.jsm need patching too? Which one is actually used? (Why do we have both, when they appear to be identical?)
Looks like all of /modules is unused, disregard my comment, I guess! (Should probably fix the other references to this no longer existing property, right? http://lxr.mozilla.org/seamonkey/search?string=.bookmarksRoot )
(In reply to comment #9) > (Should probably fix the other references to this no longer existing property, > right? http://lxr.mozilla.org/seamonkey/search?string=.bookmarksRoot ) that sounds like a good idea.
Should we remove the .jsm file if it's not used? I didn't add it, I'm not sure why it's there.
(In reply to comment #11) > Should we remove the .jsm file if it's not used? I didn't add it, I'm not sure > why it's there. I'm all for removing unnecessary files. Moving the review request from bsmedberg to gavin per Benjamin's discussion in IRC.
Comment on attachment 328045 [details] [diff] [review] [checked-in] distribution menu folder fix landed on cvs trunk Checking in distribution.js; /cvsroot/mozilla/browser/components/distribution.js,v <-- distribution.js new revision: 1.3; previous revision: 1.2 done
and pushed to mozilla-central.
Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:184.108.40.206pre) Gecko/2008080705 GranParadiso/3.0.2pre. Testcase in the attachment yields the expected results from comment 1.