397826 - Resetting bookmarks in Safemode is broken

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Shayne Jewers]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre

attempting to reset bookmarks via safemode doesn't appear to do anything.

Reproducible: Always

Steps to Reproduce:
1. Open minefield in safemode
2. Select "Reset Bookmarks" and click "Make changes and Restart"
3. Observe bookmarks
Actual Results:  
bookmarks are not rest to default bookmarks, and your bookmarks stay

Expected Results:  
bookmarks should be reset to default bookmarks

Comment 1

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007092714 Minefield/3.0a9pre ID:2007092714

I see this too. Selecting "reset bookmarks to minefield defaults" from firefox's safemode doesn't reset my bookmarks... they remain the same as they were before. Maybe a regression from places bookmarks landing; i'll investigate a little more tomorrow.

Comment 2

[(not reading, please use [e-mail] instead)]

them problem is that in the places code we dont have support for the browser.bookmarks.restore_default_bookmarks pref.

safeMode.js sets it and the old (rdf based) bookmarks service restore the default bookmarks.

see:

http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4676
http://lxr.mozilla.org/seamonkey/search?string=restore_default_bookmarks

Comment 3

[Mike Connor [:mconnor]]

So, import the default like an HTML backup restore?  Should be a simple enough bit of code to do this, no?

Comment 4

[(not reading, please use [e-mail] instead)]

> So, import the default like an HTML backup restore?  Should be a simple enough
> bit of code to do this, no?

yes, I would think so, too.

Note, here's a better link to how it worked in fx 2:

http://lxr.mozilla.org/mozilla1.8/source/browser/components/bookmarks/src/nsBookmarksService.cpp#4745

Comment 5

[Kadir Topal [:atopal]]

In Fx 2 this pref would delet all user created bookmarks, but I'm having a hard time imagining a situation, where the user really want's his bookmarks to be erased with one unintentional click. Instead, when fixed, this pref should restore the original bookmarks aditionally to the user created ones or at least a strong warning should be displayed, that the user is about to delete all his bookmarks.

Comment 6

[Marco Bonardo [:mak]]

see also Bug 332857, probably a dupe or depends on

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)
> In Fx 2 this pref would delet all user created bookmarks, but I'm having a hard
> time imagining a situation, where the user really want's his bookmarks to be
> erased with one unintentional click. Instead, when fixed, this pref should
> restore the original bookmarks aditionally to the user created ones or at least
> a strong warning should be displayed, that the user is about to delete all his
> bookmarks.
> 

the attached patch backs up the user-created bookmarks before restoring defaults.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 312759 [details] [diff] [review]
patch

>Index: browser/components/nsBrowserGlue.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/nsBrowserGlue.js,v
>retrieving revision 1.84
>diff -u -8 -p -r1.84 nsBrowserGlue.js
>--- browser/components/nsBrowserGlue.js	28 Mar 2008 16:43:17 -0000	1.84
>+++ browser/components/nsBrowserGlue.js	31 Mar 2008 17:58:39 -0000
>@@ -385,56 +385,85 @@ BrowserGlue.prototype = {
>    * - imports the bookmarks html file if bookmarks datastore is empty
>    *
>    * These prefs are set by the backend services upon creation (or recreation)
>    * of the Places db:
>    * - browser.places.importBookmarksHTML
>    *   Set to false by the history service to indicate we need to re-import.
>    * - browser.places.createdSmartBookmarks
>    *   Set during HTML import to indicate that the queries were created.
>+   *
>+   * These prefs are setup by the frontend:

nit: s/setup/set up/

>+   * - browser.bookmarks.restore_default_bookmarks
>+   *   Set to true by safe-mode dialog to indicate we must restore default
>+   *   bookmarks.
>    */
>   _initPlaces: function bg__initPlaces() {
>-    // we need to instantiate the history service before we check the 
>-    // the browser.places.importBookmarksHTML pref, as 
>+    // we need to instantiate the history service before checking
>+    // the browser.places.importBookmarksHTML pref, as
>     // nsNavHistory::ForceMigrateBookmarksDB() will set that pref
>     // if we need to force a migration (due to a schema change)
>     var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>                   getService(Ci.nsINavHistoryService);
> 
>     var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>                      getService(Ci.nsIPrefBranch);
> 
>     var importBookmarks = false;
>+    var restoreDefaultBookmarks = false;
>     try {
>-      importBookmarks = prefBranch.getBoolPref("browser.places.importBookmarksHTML");
>+      restoreDefaultBookmarks = prefBranch.getBoolPref("browser.bookmarks.restore_default_bookmarks");
>     } catch(ex) {}
> 
>+    if (restoreDefaultBookmarks) {
>+      prefBranch.setBoolPref("browser.bookmarks.restore_default_bookmarks",
>+                             false);

i think you should set the pref to false after successfully importing the defaults.

r=me with that fixed.

Comment 11

[Marco Bonardo [:mak]]

FX2 was resetting it early, so that's why i've done that immediately.

putting it in the finally clause (as importBookmarksHTML) will not change much from the actual behaviour, will be set in both cases (success or not)

Comment 12

[Marco Bonardo [:mak]]

Attachment: for check-in

fixed comments

Comment 13

[Dietrich Ayala (:dietrich)]

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.85; previous revision: 1.84
done

Comment 14

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre)
Gecko/2008042408 Minefield/3.0pre

Comment 15

[Tracy Walker [:tracy]]

added to litmus

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv