470314 - Send a bookmarks-restore notification

Status: RESOLVED FIXED

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

Description

[Dan Mills [:thunder]]

After a bookmarks restore, all items will have changed (even if they are potentially the same items).  For Weave this means all items will be processed as changes and uploaded to the server.  Adding a bookmarks-restored notification would allow Weave to do an "initial sync", meaning it will download the server data and merge, instead of uploading all items as changes.

Comment 1

[Marco Bonardo [:mak]]

mh you could get databaseStatus from the history service and see if it's DATABASE_STATUS_CORRUPT, won't that be good because you will need to look at it instead of simply waiting on a notification?

Comment 2

[Marco Bonardo [:mak]]

or better, maybe you need to know if the restore has been successful

Comment 3

[Dan Mills [:thunder]]

I was not assuming that the restore was related to a corrupt database.  Simply that the user went to the bookmarks manager and selected the option to restore from a json/html file.

Comment 5

[Drew Willcoxon :adw]

I think I can do this.  Would an nsIObserverService notification be OK?  (The bookmarks service doesn't handle restores, so I don't see how it could be a nsINavBookmarkObserver-related notification, which would make more sense).  If so, would different topics for restoring from JSON vs. restoring from HTML be helpful?  Any extra data that could be passed to the observer that might be useful?

Comment 6

[Dan Mills [:thunder]]

Yeah, it would be great to have a notification, even if it doesn't come via nsINavBookmarkObserver.

It would also be good to know if it's a json or html restore.  I can't think of anything else that would be relevant.

Thanks!

Comment 7

[Marco Bonardo [:mak]]

well, html import can act in 2 ways, based on how aInitialImport is set.
it acts as initial import only when profile is first created (first migration), while will simply append when executed manually. Has this has relevance for you?

Comment 8

[Drew Willcoxon :adw]

What about this: the nsIObserver topic is "bookmarks-restore", and the data passed to the observer is one of "json", "html", or "html-initial"?  Potentially this would be useful to other people, too.

Comment 9

[Dan Mills [:thunder]]

I don't think Weave would distinguish between them now, but it seems potentially useful.  Your suggestion in comment #8 is good imo.

Comment 10

[Drew Willcoxon :adw]

Attachment: v1

How's this?  I guess we need a test.

Comment 11

[Drew Willcoxon :adw]

Attachment: v2

Changed "bookmarks-restore" to "bookmarks-restored" (past tense) to indicate that the restore has completed by the time you're notified.  Marco, comes with a test, yay! :)

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 372202 [details] [diff] [review]
v2

Looks goot to me, thanks.
asking review from Dan, to be sure is what he needs.

Comment 13

[Marco Bonardo [:mak]]

my only concern are that importExport is a browser service, while PlacesUtils is part of toolkit. Also the notification happens inside the batch, so observer code could run during it (and we still don't know if batch won't be rollback).
What if we would notify when the import/restore starts, and try to do that only in browser code?

Comment 14

[Marco Bonardo [:mak]]

but at that point we would lose the ability to know if that has finished correctly.

Comment 15

[Drew Willcoxon :adw]

(In reply to comment #13)
> my only concern are that importExport is a browser service, while PlacesUtils
> is part of toolkit.

What exactly is the issue there?  Are you saying we really need two different notifications, one for toolkit and one for browser?

> What if we would notify when the import/restore starts, and try to do that only
> in browser code?

My understanding is that Dan needs to know when bookmarks have changed, not when they're going to change.  An additional "will restore" notification might be potentially useful to people though.  About notifying during the batch, would it be reasonable to use nsITimer/setTimeout() to push the notification outside?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> My understanding is that Dan needs to know when bookmarks have changed, not
> when they're going to change.  An additional "will restore" notification might
> be potentially useful to people though.  About notifying during the batch,
> would it be reasonable to use nsITimer/setTimeout() to push the notification
> outside?
Ideally, you'd use the thread manager, but yeah.

Comment 17

[Dan Mills [:thunder]]

Comment on attachment 372202 [details] [diff] [review]
v2

This would definitely work for me.  I can't speak for the questions about timing/specific placement of the notifications (comment #13), but I think I'll be happy wherever you fire them from.

Knowing that bookmarks are about to be restored would also be nice, though not quite as important.  It would allow Weave to ignore the impending barrage of add/delete notifications that will be caused by the import.

Thanks!

Comment 18

[Marco Bonardo [:mak]]

(In reply to comment #15)
> (In reply to comment #13)
> > my only concern are that importExport is a browser service, while PlacesUtils
> > is part of toolkit.
> 
> What exactly is the issue there?  Are you saying we really need two different
> notifications, one for toolkit and one for browser?

no but usually, if possible, we should try to have a better code separation between the two. in this case won't make a big difference and backup restore code is somehow splitted between the two.

in case of a restore you would probably see beginUpdateBatch, maybe a bunch of notifications (in 3.6 i suppose), restore notification, endUpdateBatch.
Notifying out of the batch would let you without a way to know if we are restoring or if it's a big change by the user...
Or maybe we could make a thing similar to begin/endUpdateBatch, so onBeginBookmarksRestore(type of restore), onEndBookmarksRestore(success/failed)?

Comment 19

[Drew Willcoxon :adw]

(In reply to comment #18)
> Or maybe we could make a thing similar to begin/endUpdateBatch, so
> onBeginBookmarksRestore(type of restore),
> onEndBookmarksRestore(success/failed)?

That would make the most sense I think (comment 5), but my question is why are the two different restores (HTML and JSON) not part of nsINavBookmarksService anyway, and why is one in toolkit (buried way down in utils.js) and the other in browser?  Since nsINavBookmarksService doesn't handle the restores, I don't see how it would even be feasible to add such methods to nsINavBookmarkObserver.

Comment 20

[Marco Bonardo [:mak]]

(In reply to comment #19)
> (In reply to comment #18)
> > Or maybe we could make a thing similar to begin/endUpdateBatch, so
> > onBeginBookmarksRestore(type of restore),
> > onEndBookmarksRestore(success/failed)?
> 
> That would make the most sense I think (comment 5), but my question is why are
> the two different restores (HTML and JSON) not part of nsINavBookmarksService
> anyway, and why is one in toolkit (buried way down in utils.js) and the other
> in browser?  Since nsINavBookmarksService doesn't handle the restores, I don't
> see how it would even be feasible to add such methods to
> nsINavBookmarkObserver.

oh i was not talking about new bookmarks observer methods, but 2 separate observer service notifications (sorry for confusion i created saying onSomething()), so first notification before starting that tells what import we are going to do, and a second notification after end that says restore has ended (successfully or not).

About why those have been splitted part in toolkit part in browser i don't have a real answer, was done ages before my arrival, i think all import/restore should be in browser, and toolkit should at last provide a hot-backup api to make a 1:1 copy of the db (new sqlite can do that).

Comment 21

[Drew Willcoxon :adw]

Wow, I'm dumb.  For some reason I've always assumed runInBatchMode() runs the callback asynchronously.  But it only wraps the callback in begin/endUpdateBatch().  So that was the only reason I put the notification inside the batch.  Now I agree with you (I think) that we should fire a will-restore notification before the batch and a did-restore notification after the batch that also indicates if the restore was successful.

Comment 22

[Marco Bonardo [:mak]]

updateBatch is also adding a transaction around the full thing (and it could be potentially rollback even if we don't do that explicitly)

Comment 23

[Drew Willcoxon :adw]

Any preferences on how we communicate whether the restore failed?  The current patch passes "json", etc. as data to nsIObserver.observe().  Maybe "json failed"?  A more correct way might be to create a new interface, like nsIBookmarksRestoreInfo that has attributes like "failed" and "type", and pass it as the subject to observe(), but that seems like overkill.

Comment 24

[Marco Bonardo [:mak]]

If we want to allow observers to only listen on a single topic, we could have topic "bookmarks-restore" with data like "type_of_restore|status", where status could be "begin, success, failed" (so "json|begin", "json|failed", "json|success").

Comment 25

[Drew Willcoxon :adw]

Attachment: v3

Some changes, but I think this is good.

There are 3 observer topics: "bookmarks-restore-begin", "bookmarks-restore-success", and "bookmarks-restore-failed".  3 separate topics means you have to register for each one you want, but it also means that the notifications are in line with the other topics, like "sessionstore-state-read" and "sessionstore-state-write", etc., and you don't have to parse the data string to find out what's going on, which I think is too hackish to ask people to do.

In addition, if you use importHTMLFromFileToFolder() of the HTML importer, the observer is passed the folder ID that was imported into through the subject parameter.  The subject parameter is an nsISupports, which means the folder ID is an nsISupportsPRInt64.

I guess there's little chance of this landing on 1.9.1 today, and if that's the case it might be nice to scale it back to just the bookmarks-restore-success notification if that really improves the chances of landing to help Weave.  Am I crazy?

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 373072 [details] [diff] [review]
v3

The only thing i don't like about this approach is the fact an observer has to listen on 2 notifications to know if a restore has finished, btw if it's not interested on the status it has finished it can use a switch and 2 case with fallback, so won't be a big blocker.

Please add specific comment to nsIPlacesImportExportService.idl about these notifications, even if we can't change interfaces there's no problem making interfaces comments better!

>diff --git a/browser/components/places/src/nsPlacesImportExportService.cpp b/browser/components/places/src/nsPlacesImportExportService.cpp

>+// NotifyImportObservers
>+//
>+//    Notifies bookmarks-restore observers using nsIObserverService.
>+
>+static nsresult
>+NotifyImportObservers(char* aTopic,
>+                      nsISupportsPRInt64* aFolderId,
>+                      PRBool aIsInitialImport)

shouldn't that be const char*?

I'd prefer if this function would receive a PRInt64 and then convert in place to nsISupportsPRInt64, would concentrate code changes here making this a better helper. To be consistent with other methods, when you don't have a folderId you could pass -1 instead of nsnull.

>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIObserverService> obs =
>+    do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);

if this fails any caller of the import methods will think import has failed, i think is better to ignore errors here and continue with an if (obs) { , to avoid mixing up notifications errors with import errors. Also because later you ignore this return value, so better directly making this a void helper.

> // nsIPlacesImportExportService::ImportHTMLFromFile
> //
> NS_IMETHODIMP
> nsPlacesImportExportService::ImportHTMLFromFile(nsILocalFile* aFile, PRBool aIsInitialImport)
> {
>+  NotifyImportObservers(RESTORE_BEGIN_NSIOBSERVER_TOPIC,
>+                        nsnull,
>+                        aIsInitialImport);
>+
>   // this version is exposed on the interface and disallows changing of roots
>-  return ImportHTMLFromFileInternal(aFile, PR_FALSE, 0, aIsInitialImport);
>+  nsresult rv = ImportHTMLFromFileInternal(aFile,
>+                                           PR_FALSE,
>+                                           0,
>+                                           aIsInitialImport);
>+
>+  if (NS_FAILED(rv)) {
>+    NotifyImportObservers(RESTORE_FAILED_NSIOBSERVER_TOPIC,
>+                          nsnull,
>+                          aIsInitialImport);
>+    return rv;
>+  }
>+
>+  NotifyImportObservers(RESTORE_SUCCESS_NSIOBSERVER_TOPIC,
>+                        nsnull,
>+                        aIsInitialImport);
>+  return NS_OK;

use an else and return rv

> // nsIPlacesImportExportService::ImportHTMLFromFileToFolder
> //
> NS_IMETHODIMP
> nsPlacesImportExportService::ImportHTMLFromFileToFolder(nsILocalFile* aFile, PRInt64 aFolderId, PRBool aIsInitialImport)
> {
>+  nsresult rv;
>+  nsCOMPtr<nsISupportsPRInt64> folderId =
>+    do_CreateInstance(NS_SUPPORTS_PRINT64_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = folderId->SetData(aFolderId);
>+  NS_ENSURE_SUCCESS(rv, rv);

as i said above i'd prefer seeing this inside the helper.

>diff --git a/browser/components/places/tests/unit/test_bookmarksRestoreNotification.js b/browser/components/places/tests/unit/test_bookmarksRestoreNotification.js

>+// nsIObserver that observes bookmarks-restore-begin.
>+var beginObserver = {
>+  observe: function _beginObserver(aSubject, aTopic, aData) {
>+    var test = tests[currTestIndex];
>+    if (test.currTopic !== aTopic) {
>+      do_throw("Unexpected topic " + aTopic + " for current test; was " +
>+               "expecting " + test.currTopic);

why not simply using do_check_eq in all the observers ch? that will also print the two values in case of failure.

>+    }
>+    if (test.data !== aData) {
>+      do_throw("Unexpected data " + aData + " for current test; was " +
>+               "expecting " + test.data);
>+    }
>+    test.currTopic = test.finalTopic;

add a comment saying you update currentTopic to the next expected one.

>+// nsIObserver that observes bookmarks-restore-success/failed.  This starts
>+// the next test.
>+var successAndFailedObserver = {
>+  observe: function _successAndFailedObserver(aSubject, aTopic, aData) {
>+    var test = tests[currTestIndex];
>+    if (test.currTopic !== aTopic) {
>+      do_throw("Unexpected topic " + aTopic + " for current test; was " +
>+               "expecting " + test.currTopic);
>+    }
>+    if (test.data !== aData) {
>+      do_throw("Unexpected data " + aData + " for current test; was " +
>+               "expecting " + test.data);
>+    }
>+    // On restore failed, file may not exist
>+    try {
>+      test.file.remove(false);
>+    }
>+    catch (exc) {} 

add a comment here saying you are going to check if importing to folder has setup the correct folderId

>+    if (test.folderId) {
>+      do_check_eq(aSubject.QueryInterface(Ci.nsISupportsPRInt64).data,
>+                  test.folderId);
>+    }

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js

>   restoreBookmarksFromJSONFile:
>   function PU_restoreBookmarksFromJSONFile(aFile) {

>+    }
>+    catch (exc) {
>+      failed = true;
>+      obsServ.notifyObservers(null,
>+                              RESTORE_FAILED_NSIOBSERVER_TOPIC,
>+                              RESTORE_NSIOBSERVER_DATA);

since you are catching the exception it's now hidden from external caller, please do a Components.utils.reportError("Bookmarks restore failed"); (so that it's human readable) and then rethrow the exception (clearly you'll have to catch it in the test then).

apart those, r=me
Freeze has been postponed, but getting this asap would be cool

Comment 27

[Drew Willcoxon :adw]

Thanks for the quick review.  I'll post a new patch soon.

What do you think about adding a fourth notification, "bookmarks-restore-completed" (or "bookmarks-restore-done" or "bookmarks-restore-finished") that's fired regardless of whether the restore failed or succeeded?  It would be in addition to the -failed -success notifications.

Comment 28

[Drew Willcoxon :adw]

Attachment: v4

It would be pretty simple for a follow-up to add the bookmarks-restore-completed notification.  Since I don't have Marco's approval for that and we'd like to get this checked in, not doing it now.

Comment 29

[Dan Mills [:thunder]]

FWIW, I think having 3 notifications is fine.  It's pretty easy to subscribe to two topics and have them run the same function, if that's what you want.  And it's definitely cleaner than having to parse the subject IMO.  Weave uses 3 notifications for many events (like sync :start, :success, :error).

Comment 30

[Marco Bonardo [:mak]]

no, i'm not for having a 4th notification. 3 are more than what we need, looks good as it is.

Comment 31

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d348393c7711

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 373165 [details] [diff] [review]
v4

Asking approval 1.9.1, this is really wanted for Weave support.
Has low risk, only adds some notification, as usual for Drew, comes with a bunch of tests.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 373165 [details] [diff] [review]
v4

a191=beltzner

Comment 34

[Drew Willcoxon :adw]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c4f2cb359857

Comment 35

[Dan Mills [:thunder]]

Thanks! Weave follow-up is bug 488922.

Comment 36

[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