Make suite glue initialize and migrate places bookmarks

RESOLVED FIXED in seamonkey2.1a3

Status

SeaMonkey
Bookmarks & History
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

Trunk
seamonkey2.1a3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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).
(Assignee)

Updated

7 years ago
Blocks: 580660
(Assignee)

Updated

7 years ago
OS: Linux → All
Hardware: x86 → All
(Assignee)

Updated

7 years ago
Blocks: 580662
(Assignee)

Updated

7 years ago
Blocks: 580663
(Assignee)

Comment 1

7 years ago
Created attachment 459041 [details] [diff] [review]
v2: glue changes

This is the current version of the glue code changes.
Attachment #459041 - Flags: review?(neil)
(Assignee)

Comment 2

7 years ago
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.
Attachment #459041 - Flags: review?(iann_bugzilla)

Comment 3

7 years ago
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.
Attachment #459041 - Flags: review?(iann_bugzilla) → review+
(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

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

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
(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.
(Assignee)

Comment 8

7 years ago
Created attachment 462463 [details] [diff] [review]
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.
Attachment #459041 - Attachment is obsolete: true
Attachment #462463 - Flags: review+
Attachment #459041 - Flags: review?(neil)
(Assignee)

Comment 9

7 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/50c4aad57952
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
Version: unspecified → Trunk
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 on attachment 462463 [details] [diff] [review]
v2.1: glue, fixed for Ian's comments

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

Comment 12

7 years ago
(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?
Depends on: 741068
You need to log in before you can comment on or make changes to this bug.