580658 - Make suite glue initialize and migrate places bookmarks

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Robert Kaiser]

The suite glue code should be made to initialize and migrate places bookmarks (importing from JOSN backups or HTML files as/if needed).

This is a split-off of part 3 of bug 498596, for easier review (as now that
part is isolated here).

Comment 1

[Robert Kaiser]

Attachment: v2: glue changes

This is the current version of the glue code changes.

Comment 2

[Robert Kaiser]

Comment on attachment 459041 [details] [diff] [review]
v2: glue changes

Requesting additional review from Ian on all patches but the actual core - I'll take reviews from whoever gets around to it first.

Comment 3

[Ian Neal]

Comment on attachment 459041 [details] [diff] [review]
v2: glue changes

This is just on code inspections to start with, I will do more general testing and review with all patches applied.

Some general nits, need to try and be consistent with } and the line that follows (e.g. for catch, finally and else)

>diff --git a/suite/common/pref/pref-content.xul b/suite/common/pref/pref-content.xul
>-      <description>&tbIconsDescription.label;</description>
>-      <radiogroup id="loadToolbarIcons"
>-                  class="indent"
>-                  preference="browser.chrome.load_toolbar_icons">
>-        <radio value="0"
>-               label="&tbIconsNever.label;"
>-               accesskey="&tbIconsNever.accesskey;"/>
>-        <radio value="1"
>-               label="&tbIconsCache.label;"
>-               accesskey="&tbIconsCache.accesskey;"/>
>-        <radio value="2"
>-               label="&tbIconsAlways.label;"
>-               accesskey="&tbIconsAlways.accesskey;"/>
>-      </radiogroup>
You've removed this but not the associated entities in pref-content.dtd

>diff --git a/suite/common/src/nsSuiteGlue.js b/suite/common/src/nsSuiteGlue.js
>@@ -102,16 +118,51 @@ SuiteGlue.prototype = {
>       case "session-save":
>         this._setPrefToSaveSession();
>         subject.QueryInterface(Components.interfaces.nsISupportsPRBool);
>         subject.data = true;
>         break;
>       case "dl-done":
>         this._playDownloadSound();
>         break;
>+      case "places-init-complete":
>+        this._initPlaces();
>+        Services.obs.removeObserver(this, "places-init-complete");
>+        this._hasPlacesInitObserver = false;
>+        // no longer needed, since history was initialized completely.
Nit: as comment ends with a full stop, should start with capital letter.
Similar issues with other comments throughout this patch, so if starts with a capital letter then end with a full stop, also make sure there is a space between // and the first character.

>+        } catch (err) {
>+          // Report the error, but ignore it.
>+          Components.utils.reportError("Bookmarks.html file could be corrupt. " + err);
>+          osvr.removeObserver(importObserver, "bookmarks-restore-success");
>+          osvr.removeObserver(importObserver, "bookmarks-restore-failed");
>+        }
This should be Services.obs.removeObserver shouldn't it?
Nit: space between catch and (, elsewhere use ex rather than err.

>+  _showPlacesLockedNotificationBox: function(aSubject) {
>+    // Stick the notification onto the selected tab of the active browser window.
>+    var brandBundle  = Services.strings.createBundle("chrome://branding/locale/brand.properties");
>+    var applicationName = brandBundle.GetStringFromName("brandShortName");
>+    var placesBundle = Services.strings.createBundle("chrome://communicator/locale/places/places.properties");
>+    var title = placesBundle.GetStringFromName("lockPrompt.title");
>+    var text = placesBundle.formatStringFromName("lockPrompt.text", [applicationName], 1);
>+    var buttonText = placesBundle.GetStringFromName("lockPromptInfoButton.label");
>+    var accessKey = placesBundle.GetStringFromName("lockPromptInfoButton.accessKey");
places/places.properties seems to be missing from this patch (is part of one of the core patches it seems).

>+
>+    var helpTopic = "places-locked";
>+    var helpRDF = "chrome://communicator/locale/help/suitehelp.rdf";
>+
>+    var buttons = [
>+                    {
>+                      label:     buttonText,
>+                      accessKey: accessKey,
>+                      popup:     null,
>+                      callback:  function(aNotificationBar, aButton) {
>+                        aSubject.openHelp(helpTopic, helpRDF);
>+                      }
>+                    }
>+                  ];
Nit: Match style in http://mxr.mozilla.org/comm-central/source/suite/browser/navigator.js#2659

r=me with those points addressed.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> Comment on attachment 459041 [details] [diff] [review]
> v2: glue changes
> 
> >+        } catch (err) {
> >+          // Report the error, but ignore it.
> >+          Components.utils.reportError("Bookmarks.html file could be corrupt. " + err);

Should be bookmarks.html, no? This will appear in the Error Console (thus user-facing, though not highly visible) and file names are case-sensitive on Linux. Fixing it now comes at no cost.

Comment 5

[Ian Neal]

Just as note, this patch has been bitrotted by patch from bug 505311 (oops by KaiRo)

Comment 6

[Ian Neal]

(In reply to comment #5)
> Just as note, this patch has been bitrotted by patch from bug 505311 (oops by
> KaiRo)
In browser-prefs.js that is.

Comment 7

[Robert Kaiser]

(In reply to comment #3)
> Some general nits, need to try and be consistent with } and the line that
> follows (e.g. for catch, finally and else)

I *think* we're (almost) consistent in this patch (and its context) with those with the exception of empty catches that already seem to share a line with the } in the context of this file (which I generally don't like much, though, but I copied it here).
I corrected the cases where this was done with non-empty catch in my patch (one in this file does exist outside my patch), as I don't think it's good to do this when there's actual code in the catch.

> You've removed this but not the associated entities in pref-content.dtd

Good catch!

> Nit: as comment ends with a full stop, should start with capital letter.

Thanks, I should have corrected all comments I'm adding here, I'll not touch the others in the context, or this patch will grow larger than it needs to be (I don't want to artificially grow this patchset and do as much as possible in followups).

> This should be Services.obs.removeObserver shouldn't it?

OOps. Good catch!

> places/places.properties seems to be missing from this patch (is part of one of
> the core patches it seems).

Yes, everything needed up to here is in core, it felt too tedious to split up that file between patches just for the sake of reviews.
BTW, I have done a run through all the L10n files to check for unused strings, and even written up a script to check that and a bug against Firefox for those they had as well (resolved since).

Rest of the comments are also fixed locally.

(In reply to comment #4)
> > >+          Components.utils.reportError("Bookmarks.html file could be corrupt. " + err);
> 
> Should be bookmarks.html, no?

Probably a good idea, yes. Thanks. Fixed.

Comment 8

[Robert Kaiser]

Attachment: v2.1: glue, fixed for Ian's comments

Here's the updated patch that's fixed for Ian's comments. This should be ready for checkin as it is, given that core gets reviews.

Comment 9

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/50c4aad57952

Comment 10

[[e-mail]]

Comment on attachment 462463 [details] [diff] [review]
v2.1: glue, fixed for Ian's comments

>-pref("browser.chrome.image_icons.max_size", 1024);
This is a tabbrowser preference, not a bookmarks preference, and it should not have been removed.

Comment 11

[[e-mail]]

Comment on attachment 462463 [details] [diff] [review]
v2.1: glue, fixed for Ian's comments

>+    var notifyBox = aSubject.gBrowser.getNotificationBox();
Nit: getBrowser()

Comment 12

[Robert Kaiser]

(In reply to comment #10)
> >-pref("browser.chrome.image_icons.max_size", 1024);
> This is a tabbrowser preference, not a bookmarks preference, and it should not
> have been removed.

Defined in http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/init/all.js#159 anyhow, so I just removed a double-definition.

(In reply to comment #11)
> >+    var notifyBox = aSubject.gBrowser.getNotificationBox();
> Nit: getBrowser()

I'll fold that into the bug 581319 patch, OK?