Closed Bug 546942 Opened 10 years ago Closed 10 years ago

Move places import-export-service to toolkit

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: kairo, Assigned: mak)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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.
Blocks: 549736
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
WIP
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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #432963 - Attachment is obsolete: true
note to self: remember to fix smart bookmarks.
Attached patch patch v1.2 (obsolete) — Splinter Review
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.
Attachment #433193 - Attachment is obsolete: true
Attachment #433352 - Flags: feedback?(adw)
self-review: there is a semicolon in nsPlacesModule that should not be there.
Attached patch patch v1.3 (obsolete) — Splinter Review
fixes the semicolon.
Attachment #433352 - Attachment is obsolete: true
Attachment #433495 - Flags: feedback?(adw)
Attachment #433352 - Flags: feedback?(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.)
Attachment #433495 - Flags: feedback?(adw)
(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.
(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.
Attached patch patch v1.4Splinter Review
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.
Attachment #433495 - Attachment is obsolete: true
Attachment #434227 - Flags: review?(dietrich)
Comment on attachment 434227 [details] [diff] [review]
patch v1.4

r=sdwilsh
Attachment #434227 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/mozilla-central/rev/4bcd8b3ae5a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
blocking2.0: ? → ---
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.