546942 - Move places import-export-service to toolkit

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Robert Kaiser]

IMHO, any places user should have some import/export code available, so that code should be in toolkit. Also the HTML bookmarks format has been in use for more than a decade by major browsers so it should probably be available in more than just browser/ - if mobile doesn't want it, we can see to have a build flag disabling it.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

WIP

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The above patch has some error i fixed locally.
an actual problem is that import export depends on browserGlue to create the smart bookmarks. but i guess we can remove this thing and fix browserGlue instead.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

note to self: remember to fix smart bookmarks.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

This un-hooks importExport service from browserGlue

Drew, can you do a first-review pass please? So later i can just get approval from Dietrich that already has some review in queue.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

self-review: there is a semicolon in nsPlacesModule that should not be there.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.3

fixes the semicolon.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 433495 [details] [diff] [review]
patch v1.3

Since I don't know much about browser glue and these makefiles and such, I'll leave those to Dietrich.

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -678,8 +678,9 @@ BrowserGlue.prototype = {
>     }
> 
>     if (!importBookmarks) {
>-      // Call it here for Fx3 profiles created before the Places folder
>-      // has been added, otherwise it's called during import.
>+      // In case we are importing bookmarks, smart bookmarks should be created
>+      // at the end of the import operation, otherwise smart bookmarks would be
>+      // overwritten and lost.
>       this.ensurePlacesDefaultQueriesInitialized();

This comment describes not the !importBookmarks case but the importBookmarks case, and it confused me until I looked at the next part of the patch.  You might want to move this comment to the code it actually describes, where you add the import observer.  Actually I would like to see a comment above this if statement describing why it's needed at all.  The else branch does a lot more than the if branch, and it's not clear to me what's going on at a higher level.

>@@ -719,6 +734,8 @@ BrowserGlue.prototype = {
>         } catch (err) {
>           // Report the error, but ignore it.
>           Cu.reportError("Bookmarks.html file could be corrupt. " + err);
>+          osvr.removeObserver(importObserver, "bookmarks-restore-success");
>+          osvr.removeObserver(importObserver, "bookmarks-restore-failed");

Where is osvr defined?  Did you mean this._observerService?

>diff --git a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js
>--- a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js
>+++ b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup.js
>@@ -81,7 +81,8 @@ function run_test() {
>       os.removeObserver(observer, "bookmarks-restore-failed");
>       do_check_eq(aTopic, "bookmarks-restore-success");
>       do_check_eq(aData, "html-initial");
>-      continue_test();
>+      // Smart bookmarks are created at HTML import notification, so enqueue.
>+      do_timeout(0, continue_test);

Hmm, seems kind of hacky.  I'm imagining oranges due to some unimagineable, dumb reason that causes continue_test to run before the browser glue stuff that creates the smart bookmarks.  Could you use a bookmark observer instead?

>diff --git a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js
>--- a/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js
>+++ b/browser/components/places/tests/unit/test_browserGlue_corrupt_nobackup_default.js
>@@ -81,7 +81,8 @@ function run_test() {
>       os.removeObserver(observer, "bookmarks-restore-failed");
>       do_check_eq(aTopic, "bookmarks-restore-success");
>       do_check_eq(aData, "html-initial");
>-      continue_test();
>+      // Smart bookmarks are created at HTML import notification, so enqueue.
>+      do_timeout(0, continue_test);

Here too.

>diff --git a/toolkit/components/build/nsToolkitCompsCID.h b/toolkit/components/build/nsToolkitCompsCID.h
>--- a/toolkit/components/build/nsToolkitCompsCID.h
>+++ b/toolkit/components/build/nsToolkitCompsCID.h
>@@ -113,6 +113,9 @@
> #define NS_FAVICONSERVICE_CONTRACTID \
>   "@mozilla.org/browser/favicon-service;1"
> 
>+#define NS_PLACESIMPORTEXPORTSERVICE_CONTRACTID \
>+  "@mozilla.org/browser/places/import-export-service;1"
>+

It's weird that this service is now in toolkit but its name implies it's still in browser.  Would it be difficult to change?

>diff --git a/browser/components/places/src/nsPlacesImportExportService.cpp b/toolkit/components/places/src/nsPlacesImportExportService.cpp
>rename from browser/components/places/src/nsPlacesImportExportService.cpp
>rename to toolkit/components/places/src/nsPlacesImportExportService.cpp
>--- a/browser/components/places/src/nsPlacesImportExportService.cpp
>+++ b/toolkit/components/places/src/nsPlacesImportExportService.cpp
>@@ -95,7 +95,7 @@
> #include "nsPlacesImportExportService.h"
> #include "nsNetUtil.h"
> #include "nsParserCIID.h"
>-#include "nsStringAPI.h"
>+//#include "nsStringAPI.h"

Hmm?

>@@ -1330,7 +1343,7 @@ BookmarkContentSink::ConvertImportedDate
>   PRTime convertedDate = 0;
>   if (!aDate.IsEmpty()) {
>     nsresult rv;
>-    convertedDate = aDate.ToInteger(&rv);
>+    convertedDate = PromiseFlatCString(aDate).ToInteger(&rv);

I'm trying to understand the need for PromiseFlatCString.  Is it because nsACString::ToInteger boils down to scanf (PR_sscanf), and sscanf needs a null-terminated string?  (If that's the case, seems like ToInteger should be smart enough to handle it, but looking at the code, it doesn't.)

Comment 9

[Robert Kaiser]

(In reply to comment #8)
> It's weird that this service is now in toolkit but its name implies it's still
> in browser.  Would it be difficult to change?

We have a number of other services in toolkit for which the same would be true. If any add-ons or any other pieces of code are using this service, they instantiate the service with this name, with it unchanged they don't even notice that it's now served from toolkit, which is a good thing as they don't actually need to change anything.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #8)
> >diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
> >-      // Call it here for Fx3 profiles created before the Places folder
> >-      // has been added, otherwise it's called during import.
> >+      // In case we are importing bookmarks, smart bookmarks should be created
> >+      // at the end of the import operation, otherwise smart bookmarks would be
> >+      // overwritten and lost.
> >       this.ensurePlacesDefaultQueriesInitialized();
> 
> This comment describes not the !importBookmarks case but the importBookmarks
> case, and it confused me until I looked at the next part of the patch.

will do

> >@@ -719,6 +734,8 @@ BrowserGlue.prototype = {
> >         } catch (err) {
> >           // Report the error, but ignore it.
> >           Cu.reportError("Bookmarks.html file could be corrupt. " + err);
> >+          osvr.removeObserver(importObserver, "bookmarks-restore-success");
> >+          osvr.removeObserver(importObserver, "bookmarks-restore-failed");
> 
> Where is osvr defined?  Did you mean this._observerService?

yep, ops.

> >+      do_timeout(0, continue_test);
> 
> Hmm, seems kind of hacky.  I'm imagining oranges due to some unimagineable,
> dumb reason that causes continue_test to run before the browser glue stuff that
> creates the smart bookmarks.  Could you use a bookmark observer instead?

will do

> >diff --git a/toolkit/components/build/nsToolkitCompsCID.h b/toolkit/components/build/nsToolkitCompsCID.h
> >+#define NS_PLACESIMPORTEXPORTSERVICE_CONTRACTID \
> >+  "@mozilla.org/browser/places/import-export-service;1"
> >+
> 
> It's weird that this service is now in toolkit but its name implies it's still
> in browser.  Would it be difficult to change?

This is something i don't want to change, for compat reasons.

> >diff --git a/browser/components/places/src/nsPlacesImportExportService.cpp b/toolkit/components/places/src/nsPlacesImportExportService.cpp
> >-#include "nsStringAPI.h"
> >+//#include "nsStringAPI.h"
> 
> Hmm?

forgot to remove

> >@@ -1330,7 +1343,7 @@ BookmarkContentSink::ConvertImportedDate
> >   PRTime convertedDate = 0;
> >   if (!aDate.IsEmpty()) {
> >     nsresult rv;
> >-    convertedDate = aDate.ToInteger(&rv);
> >+    convertedDate = PromiseFlatCString(aDate).ToInteger(&rv);
> 
> I'm trying to understand the need for PromiseFlatCString.  Is it because
> nsACString::ToInteger boils down to scanf (PR_sscanf), and sscanf needs a
> null-terminated string?  (If that's the case, seems like ToInteger should be
> smart enough to handle it, but looking at the code, it doesn't.)

yep, it's barely to avoid internal string API that are not accessible from here, ToInteger wants a null terminated string, as you said.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.4

Addressed comments, changed relevant tests to wait for the smart bookmarks endUpdateBatch. I'm also now using the browserGlue nsIObserver implementation, it looks cleaner and has less code and temp objects.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 434227 [details] [diff] [review]
patch v1.4

r=sdwilsh

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/4bcd8b3ae5a9