Closed Bug 763295 Opened 12 years ago Closed 11 years ago

Port the bookmarks export service to JavaScript

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

A functionally identical port of the "bookmarks.html" export code from C++ to
JavaScript paves the way for:

- Making the favicons queries asynchronous
- Making the bookmark queries asynchronous
- Making the file writes asynchronous
- Supporting JSON formats
Attached patch Work in progress (obsolete) — Splinter Review
The port of the C++ code is complete, and the generated file is identical. I've
only updated one test for now, that passes as expected.
Attachment #631738 - Flags: feedback?(mak77)
Comment on attachment 631738 [details] [diff] [review]
Work in progress

Review of attachment 631738 [details] [diff] [review]:
-----------------------------------------------------------------

So I'm happy this is happening, something needs to be changed though

Also, this doesn't seem to remove the old cpp code.

::: browser/components/nsBrowserGlue.js
@@ +1113,5 @@
> +              Cu.reportError("Bookmarks.html file could not be saved.");
> +            }
> +          }).bind(this));
> +      } catch (err) {
> +        Cu.reportError("Bookmarks.html file could not be saved. " + err);

Actually these reportError won't ever be read by anyone, since this happens, unfortunately, at shutdown. So we may just avoid handling errors here, try/catch and forget.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +27,5 @@
>  
>  const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
>  const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +
> +const US_PER_S = 1000000;

in the past we refused using the US abbreviation cause it's confusing with "us"
so expand to MICROSEC_PER_SEC

@@ +60,5 @@
> +  /**
> +   * Saves the current bookmarks hierarchy to a bookmarks.html file.
> +   *
> +   * This function can be called on shutdown, thus it is actually synchronous,
> +   * despite implementing a success or failure callback.

This is actually problematic, we need to figure out a better solution to make this async... or spin in the single autoExportHTML case, that sucks but it's a non-default option that is rarely used so it may be acceptable as a temporary solution. But really we should not make this exposed method synchronous.
(the alternative would be to temporarily expose a _synchronouExportToFile "private" method, clearly being private may go away at any time so add-ons should not rely on it, we took a similar hack in migration iirc).

@@ +61,5 @@
> +   * Saves the current bookmarks hierarchy to a bookmarks.html file.
> +   *
> +   * This function can be called on shutdown, thus it is actually synchronous,
> +   * despite implementing a success or failure callback.
> +   */

the javadoc lacks @params

@@ +832,5 @@
> +    return (aText || "").replace("&", "&")
> +                        .replace("<", "&lt;")
> +                        .replace(">", "&gt;")
> +                        .replace("\"", "&quot;")
> +                        .replace("'", "&#39;");

These don't work as expected, cause replace is missing the "g" option, so will just replace a single entity

@@ +840,5 @@
> +   * Provides URL escaping for use in HTML attributes of the bookmarks file,
> +   * compatible with the old bookmarks system.
> +   */
> +  escapeUrl: function escapeUrl(aText) {
> +    return (aText || "").replace("\"", "%22");

ditto!

@@ +851,5 @@
> +    } catch (ex) {
> +      aCallback(false, ex);
> +      return;
> +    }
> +    aCallback(true);

so, this should actually be async, we will special case at the caller level.
I'm fine with fake-async for now, we should file a bug to make this actually async (OS.File?)

@@ +915,5 @@
> +
> +    // Write the Bookmarks Menu as the outer container.
> +    query.setFolders([PlacesUtils.bookmarksMenuFolderId], 1);
> +    let root = PlacesUtils.history.executeQuery(query, options).root;
> +    root.containerOpen = true;

I think you may use PlacesUtils.getFolderContents(PlacesUtils.bookmarksMenuFolderId).root here, saving 6 lines of code :)

@@ +928,5 @@
> +
> +    // Write the Bookmarks Toolbar as a child item for backwards compatibility.
> +    query.setFolders([PlacesUtils.toolbarFolderId], 1);
> +    root = PlacesUtils.history.executeQuery(query, options).root;
> +    root.containerOpen = true;

ditto, and so on...

@@ +970,5 @@
> +    this._writeLine(">" + this.escapeHtml(aItem.title) + "</H3>");
> +    this._writeDescription(aItem);
> +    this._writeLine(aIndent + "<DL><p>");
> +    this._writeContainerContents(aItem, aIndent);
> +    this._writeLine(aIndent + "</DL><p>");

I wonder if we should make this readable as a sort of 
"<DT><H3 %ATTRIBUTES%>%TITLE%<H3>\n%DESC%<DL>"... and then go with .replace, a sort of template replacement. Not a suggestion to implement, just a loud thought.

@@ +983,5 @@
> +        // Since the livemarks service is asynchronous, use the annotations
> +        // service to get the information for now.
> +        if (PlacesUtils.annotations.itemHasAnnotation(
> +                                           child.itemId,
> +                                           PlacesUtils.LMANNO_FEEDURI)) {

nit: fancy indentation...

Actually this should use mozIAsyncLivemarks.getLivemark, that means this method can't be synchronous :(
Btw, we can handle that apart, let's keep this as a pure cpp -> js conversion

@@ +988,5 @@
> +          this._writeLivemark(child, localIndent);
> +        } else {
> +          // This is a normal folder, open it.
> +          child.QueryInterface(Ci.nsINavHistoryContainerResultNode)
> +               .containerOpen = true;

PlacesUtils.asContainer(child).containerOpen = true

@@ +1005,5 @@
> +  },
> +
> +  _writeSeparator: function writeSeparator(aItem, aIndent) {
> +    // We can't write the separator ID or anything else other than NAME because
> +    // it makes Firefox 2.x crash/hang.  See bug 381129.

Actually we don't care about Firefox 2.x anymore, this comment can go away

@@ +1008,5 @@
> +    // We can't write the separator ID or anything else other than NAME because
> +    // it makes Firefox 2.x crash/hang.  See bug 381129.
> +    this._write(aIndent + "<HR");
> +
> +    // Separator result nodes don't support the title getter yet.

this comment is wrong as well, we stopped supporting titles in separators, so the fact is that we keep importing and exporting titles for legacy reasons but don't directly support them

@@ +1025,5 @@
> +  _writeLivemark: function writeLivemark(aItem, aIndent) {
> +    this._write(aIndent + "<DT><A");
> +    let feedSpec = PlacesUtils.annotations.getItemAnnotation(
> +                                                  aItem.itemId,
> +                                                  PlacesUtils.LMANNO_FEEDURI);

nit: fancy indent (there are others, so won't comment further)

so, we need a follow-up bug to use the new async livemarks
Attachment #631738 - Flags: feedback?(mak77) → feedback+
Blocks: 809862
Attached patch The patch (obsolete) — Splinter Review
Addressed all the previous comments. While working on this, I realized that a
promise-based API really suits these BookmarkHTMLUtils helpers, let me know if
you agree with me. Even changing the signature of the existing functions has a
very low impact on existing add-ons (there is only one on MXR that uses the
import function, and it leaves out the callback argument in any case, thus no
change is needed there).

I also think we should go for an "add_task" function, that we can easily
uplift to the testing framework, rather than a wrapAsTask local helper. That
helper would only make sense in conjunction with add_test, so we might as well
make things easier and avoid conversion problems like:

add_test(wrapAsTask(function test_promise_resolves_to_true() {
  let result = yield promiseThatResolvesToTrue;
  do_check_true(result);
});

that is incorrect, while the following is correct and easier to read:

add_task(function test_promise_resolves_to_true() {
  let result = yield promiseThatResolvesToTrue;
  do_check_true(result);
});
Attachment #631738 - Attachment is obsolete: true
Attachment #680442 - Flags: review?(mak77)
(In reply to Paolo Amadini [:paolo] from comment #3)
> Created attachment 680442 [details] [diff] [review]
> The patch
> 
> Addressed all the previous comments. While working on this, I realized that a
> promise-based API really suits these BookmarkHTMLUtils helpers, let me know
> if
> you agree with me.

My problem is that promises are a proposal, a well accepted one, but still not a standard in ecmascript. Here we are making an API depend on something that may change. Another fact is that it's easy to wrap a callback based API in a promises-based ones, so whoever wants that can easily do that.
I tried discussing this a bit with Gavin but we don't have strong opinions on it, though we think it's sane to be a bit conservative on APIs for now (they are definitely fine for internal usage).
The only answer I may give is that we should contact someone that is actually working on ecmascript standardization and ask what's current status of promises standardization, then take a decision based on that. This would be useful also for all future cases.

So, I'm reviewing this patch modulo the promises approach.

> I also think we should go for an "add_task" function, that we can easily
> uplift to the testing framework, rather than a wrapAsTask local helper.

yes, this is good.
Comment on attachment 680442 [details] [diff] [review]
The patch

Review of attachment 680442 [details] [diff] [review]:
-----------------------------------------------------------------

almost-r+ :)

There is the synchronous/asynchronous fact below, and the discussion about promises in APIs above to be clarified.

::: browser/components/nsBrowserGlue.js
@@ +1178,5 @@
> +      // If this fails to get the preference value, we don't export.
> +      if (Services.prefs.getBoolPref("browser.bookmarks.autoExportHTML")) {
> +        // This call is actually synchronous.  Since we are shutting down, we
> +        // ignore the return value because we cannot even report errors.
> +        BookmarkHTMLUtils.exportToFile(FileUtils.getFile("BMarks", []));

The comment should state that it works as far as the internal implementation is synchronous, it must look like a really dangerous thing to rely on, not a common operation, should clearly state add-ons must not rely on this since may change at any time.

Considered how rarely this pref is used, I'd accept spinning for this single case.
This is likely not going to survive long, considered the async livemarks and async favicons problems, so maybe we should immediately take the hack, file a bug to figure out a solution to it, and move on.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +896,5 @@
> +  exportToFile: function exportToFile(aLocalFile, aCallback) {
> +    let deferred = Promise.defer();
> +    try {
> +      // The export operation is internally synchronous at present.  If this
> +      // changes, then the call in nsBrowserGlue must be updated as well.

I we have to retain this, move it to a huge @note in the missing javadoc
Attachment #680442 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #4)
> I tried discussing this a bit with Gavin but we don't have strong opinions
> on it, though we think it's sane to be a bit conservative on APIs for now
> (they are definitely fine for internal usage).

I think it depends on the definition of API. For functions exposed to web content,
that is definitely a good strategy. For functions exposed only to privileged
JavaScript (internal use and add-ons), I think it can be fine to return a promise.

I'm glad that OS.File set a precedent for the use of promises in add-ons and
internal code, it's a quite extensive API that will soon be widely used in our
code and in add-ons. I doubt we'll ever want to change the semantics or the
naming of the "then" function now that we're here.

In the BookmarkHTMLUtils case, where we're even more far from the definition of a
public API than OS.File is, an far less call points, I think we can return one of
our promise objects without concerns.
(In reply to Marco Bonardo [:mak] from comment #5)
> Considered how rarely this pref is used, I'd accept spinning for this single
> case.
> This is likely not going to survive long, considered the async livemarks and
> async favicons problems, so maybe we should immediately take the hack, file
> a bug to figure out a solution to it, and move on.

I'll do this.
I found two bugs related to asynchronous shutdown: bug 518683 and bug 435058.
Attached patch Final patch (obsolete) — Splinter Review
Attachment #680442 - Attachment is obsolete: true
Attachment #683477 - Flags: review?(mak77)
Attached patch Final patchSplinter Review
It seems a couple of minor fixes didn't get in the previous attachment.
Attachment #683477 - Attachment is obsolete: true
Attachment #683477 - Flags: review?(mak77)
Attachment #683479 - Flags: review?(mak77)
(In reply to Paolo Amadini [:paolo] from comment #10)
> It seems a couple of minor fixes didn't get in the previous attachment.

Never mind, the attachments are identical. Just, you shouldn't trust the
Bugzilla interdiff, you won't see all changes.
Comment on attachment 683479 [details] [diff] [review]
Final patch

Review of attachment 683479 [details] [diff] [review]:
-----------------------------------------------------------------

r-me with the following comments fixed.

I changed my mind regarding the use of promises here for a couple reasons:
1. They are already suggested to add-on authors by the add-ons sdk documentation
2. We are working on a Storage promises based module (bug 813833) and OS.File

In the end it's unlikely we can do more damage with this seldomly used module.

::: browser/components/migration/src/MigrationUtils.jsm
@@ +268,5 @@
>  
>          // Note doMigrate doesn't care about the success value of the
>          // callback.
>          BookmarkHTMLUtils.importFromURL(
> +          "resource:///defaults/profile/bookmarks.html", true).then(function() {

The previous code was ignoring success/failure result of the import, while looks like this one cares about that? We should continue regardless success/failure here.

::: browser/components/nsBrowserGlue.js
@@ +1122,5 @@
>        if (bookmarksURI) {
>          // Import from bookmarks.html file.
>          try {
> +          BookmarkHTMLUtils.importFromURL(bookmarksURI.spec, true)
> +                           .then(function onSuccess() {

what about indenting as
blabla.then(
  function onSuccess() {
    // bla
  },
  function onFailure() {
    // bla
  }
);

@@ +1130,5 @@
> +            // Ensure that smart bookmarks are created once the operation is
> +            // complete.
> +            this.ensurePlacesDefaultQueriesInitialized();
> +          }.bind(this), function onFailure() {
> +            Cu.reportError("Bookmarks.html file could be corrupt.");

I'm actually thinking there is a bug here, cause we are not applying distribution.ini bookmarks and initing smart queries if bookmarks.html is corrupt...
We should invoke those in both cases. Not your fault, was already broken.

@@ +1177,4 @@
>      try {
> +      // If this fails to get the preference value, we don't export.
> +      if (Services.prefs.getBoolPref("browser.bookmarks.autoExportHTML")) {
> +        // Exceptionally, since this is a non-default setting, we spin the event

I'd also add "since this is a non-default settings and html format is discouraged in favor of the json backups"

@@ +1190,5 @@
> +          function onFailure() {
> +            // There is no point in reporting errors since we are shutting down.
> +            shutdownComplete = true;
> +          }
> +        );

hey you used the right indentation here ;)

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +912,5 @@
> +    safeFileOut.init(aLocalFile,
> +                     FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE
> +                                           | FileUtils.MODE_TRUNCATE,
> +                     parseInt("0600", 8),
> +                     0);

nit: just move the zero to the line above, it's uninteresting
Attachment #683479 - Flags: review?(mak77) → review+
Landed with the changes above, and one more converted test file:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5273eab26e50
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/5273eab26e50
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 820308
You need to log in before you can comment on or make changes to this bug.