Marco Bonardo [:mak] Reporter
	Description • 10 years ago

BookmarkHTMLUtils mostly, there could be some related code around
should be fake-async already.

	Marco Bonardo [:mak] Reporter
	Comment 1 • 10 years ago

Here we must replace nsINavBookmarks API calls with Bookmarks.jsm calls

	Mitch L
	Comment 2 • 8 years ago

This looks like an interesting bug and I'd like to work on it, could I be assigned please?

	Marco Bonardo [:mak] Reporter
	Comment 3 • 8 years ago

Sure, feel free to needinfo me or drop me a mail for questions.

The scope here is to replace calls to the old synchronous bookmarks API (insertBookmark, removeItem, createFolder, removeFolderChildren, moveItem, insertSeparator, getIdForItemAt, setItemTitle, getItemTitle, setItemDateAdded, getItemDateAdded, setItemLastModified, getItemLastModified, getBookmarkURI, getItemIndex, setItemIndex, isBookmarked) with the new asynchronous API (PlacesUtils.bookmarks.insert/update/remove/fetch/eraseEverything... as documented in Bookmarks.jsm module).

Additional tooling useful here is promises and Task.jsm, that should make the changes closer to the synchronous version.

additionally should change this to use GUIDs (toolbarGuid, menuGuid, ...) instead of ids (bookmarksMenuFolderId, toolbarFolderId ...)

In some cases the operations that currently happen in multiple steps (for example createFolder then setItemDateAdded) can be done in one step with the new API.

	Mitch L
	Comment 4 • 8 years ago

Thought I'd update you so you know I'm still looking at this.

I've had to research a lot of new things but I am working on it just have limited time :)

	Josh Aas
	Comment 5 • 7 years ago

Been working on converting the HTML importer code to using insertTree. Posting what I have so far.

	Mark Banner (:standard8) Assignee
	Comment 6 • 7 years ago

Comment on attachment 8869023 [details] [diff] [review]
HTML Importer insertTree Patch v0.2

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

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +416,4 @@
>     *       We also don't import ADD_DATE or LAST_MODIFIED for separators because
>     *       pre-Places bookmarks did not support them.
>     */
> +// I don't think the insertTree API we use to insert bookmarks supports separators.

PlacesUtils.bookmarks.insertTree does handle separators - it needs something like:

node.type = PlacesUtils.bookmarks.TYPE_SEPARATOR

@@ +626,5 @@
>  
> +// The following code depends on having a previousID for the inserted bookmark.
> +// Since we don't actually insert and get an ID now, we don't have one.
> +// As far as I know, the insertTree API we use to insert bookmarks does not
> +// support the LOAD_IN_SIDEBAR_ANNO annotiation.

It doesn't currently, but as you'll see in the patch on bug 1095426, I've been adding various annotations for bookmarks that I've been hitting in tests and the existing json code. So adding this on top, might not be too much work.

For the IDs, I'm having to do something similar to fix up the folder queries, so you might be able to base something on that (I'm still working on getting that 100% correct atm).

@@ +692,4 @@
>  
>      try {
>        if (frame.previousFeed) {
> +//XXXJOSH not sure how to handle live bookmarks with insertTree API

Bug 1095426 should just make this work if you add it like a normal bookmark "node" with livemark annotations.

	Josh Aas
	Comment 7 • 7 years ago

Adds support for separators and fixes an issue with modification dates.

There is some more profiling work to be done as well because a large import still takes a long time or just never completes, not sure which yet.

I'm going to leave live bookmark support and annotation work until the JSON patch lands.

	Josh Aas
	Comment 8 • 7 years ago

Now that the JSON version of this has landed I'll finish this up.

	Marco Bonardo [:mak] Reporter
	Comment 9 • 7 years ago

Mark offered to do a first pass review on this. Regardless, feel free to ask any questions. As we discovered in the JSON patch, there are still things to fix in the new API.

	Josh Aas
	Comment 10 • 7 years ago

This patch is a lot further along than the last one. There are two known issues remaining:

1. When importing a live bookmark there is an odd delay in updating the bookmarks UI and then the inserted bookmark appears as a normal non-live bookmark. This odd delay happens even when just importing a single bookmark. I think I'm inserting live bookmarks correctly from my code, seems like insertTree API isn't handling it well.

2. I don't know what to do about importing favicons. I've just commented that code out for now. Could use advice on a strategy for handling favicons.

	Mark Banner (:standard8) Assignee
	Comment 11 • 7 years ago

Comment on attachment 8883173 [details] [diff] [review]
HTML Importer insertTree Patch v0.5

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

(In reply to Josh Aas from comment #10)
> 1. When importing a live bookmark there is an odd delay in updating the
> bookmarks UI and then the inserted bookmark appears as a normal non-live
> bookmark. This odd delay happens even when just importing a single bookmark.
> I think I'm inserting live bookmarks correctly from my code, seems like
> insertTree API isn't handling it well.

With how insertTree currently works that is entirely possible - since we insert placeholders first then update the bookmarks. This was mainly so that we could add everything in one big lump, and then manage the livemarks via their normal service.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +600,5 @@
> +    // Add bookmark to the tree.
> +    frame.folder.children.push(bookmark);
> +    frame.previousBookmark = bookmark;
> +
> +//XXXJOSH I am not sure what to do about favicons when we're using insertTree

The way the JSON bookmarks handle this is to save them up until the insertTree is completed, then use the favicon service to insert any that are found.

	Josh Aas
	Comment 12 • 7 years ago

Includes some more fixes, live bookmarks work as expected now. It was my fault, not the insertTree API.

Icons are still not supported in this patch but before I go any further I'm hoping for a review to make sure I'm on the right track. I have no prior experience with this code and I rarely write JS.

	Mark Banner (:standard8) Assignee
	Comment 13 • 7 years ago

Comment on attachment 8883452 [details] [diff] [review]
HTML Importer insertTree Patch v0.6

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

Generally the js is looking reasonable, there's a few stylistic issues I commented on. I noticed that some of the xpcshell-unit tests are failing, you might want to start with getting test_bookmarks_html.js to pass.

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +318,4 @@
>    // counter.
>    this._source = aInitialImport ? PlacesUtils.bookmarks.SOURCE_IMPORT_REPLACE :
>                                    PlacesUtils.bookmarks.SOURCE_IMPORT;
> +  

note: we now have ESLint tests run in automation, so you'll want to run ESLint before heading to final review. More info here: https://developer.mozilla.org/docs/ESLint

@@ +320,5 @@
>                                    PlacesUtils.bookmarks.SOURCE_IMPORT;
> +  
> +  let bookmarkRoot = {type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +                      guid: PlacesUtils.bookmarks.menuGuid,
> +                  children: []};

nit: alignment. Generally I think we prefer this format:

let bookmarkRoot = {
  type: ...,
  guid: ...,
  children: ...
};

@@ +358,3 @@
>      switch (containerType) {
>        case Container_Normal:
> +        folder.title = containerTitle;

Can you add a note that this is a sub-folder of an existing folder, so we shouldn't need to set a guid here, as it'll be the child of whichever parent it is in the folder tree that we create?

@@ +384,2 @@
>      }
> +    if (folder.dateAdded && frame.previousLastModifiedDate != null) {

From bookmarks.preplaces.html it appears we can hit cases where last modified is specified but dateAdded is not.

I think we need to support that case, so we probably have to set dateAdded to lastModified in this specific case.

@@ +549,5 @@
>      }
>  
> +    let bookmark = {annos: []};
> +
> +    if (frame.previousLink != null) {

nit (several places): We normally just use `if (!frame.previousLink) {`

@@ +593,5 @@
> +
> +    // Remove the annotations array if we didn't add any annotations.
> +    // Validator doesn't like an empty annotations array.
> +    if (bookmark.annos.length == 0) {
> +      delete bookmark.annos;

From what I can see, we're only setting annos within one if statement, so maybe we should just add it in there, rather than deleting it later?

@@ +600,5 @@
> +    // Add bookmark to the tree.
> +    frame.folder.children.push(bookmark);
> +    frame.previousBookmark = bookmark;
> +
> +//XXXJOSH I am not sure what to do about favicons when we're using insertTree

The way I handled these in the json case was to form the tree nodes with the favicon data, then handle it in a separate iteration of the tree later (insertFaviconsForTree) - so we might actually want to share the function between the two files (maybe via PlacesBackups.jsm).

@@ +637,4 @@
>        // we also need to re-set the imported last-modified date here. Otherwise
>        // the addition of items will override the imported field.
>        let prevFrame = this._previousFrame;
> +      if (frame.folder.dateAdded && prevFrame.previousLastModifiedDate != null) {

Again, we might just need to handle the no dateAdded but lastModified case.

	Josh Aas
	Comment 14 • 7 years ago

Just storing this here so I have it between different machines, it's sometimes useful.

	Josh Aas
	Comment 15 • 7 years ago

(In reply to Mark Banner (:standard8) (afk 21 - 30 July) from comment #13)
> Comment on attachment 8883452 [details] [diff] [review]
> HTML Importer insertTree Patch v0.6

Thanks for the review. Been fixing things up, just about have a new patch ready.

> @@ +549,5 @@
> >      }
> >  
> > +    let bookmark = {annos: []};
> > +
> > +    if (frame.previousLink != null) {
> 
> nit (several places): We normally just use `if (!frame.previousLink) {`

What I wrote and what you suggested are not logically equivalent (you're reversing the intended logic). The reason I wrote it this way is that JS has confusing boolean value rules (confusing to me anyway) and I specifically want to know if that value is null, because null is the default value.

	Josh Aas
	Comment 16 • 7 years ago

Fixes a number of issues. More work to do on passing tests.

	Josh Aas
	Comment 18 • 7 years ago

Moves to current trunk and fixes an issue. Still doesn't pass tests, though it's closer.

	Marco Bonardo [:mak] Reporter
	Comment 19 • 7 years ago

This is unlikely to make 57 at this point.

	Mark Banner (:standard8) Assignee
	Comment 20 • 7 years ago

Josh has been busy and has agreed for me to take this over.

	Mark Banner (:standard8) Assignee
	Comment 22 • 7 years ago

I updated the patch slightly - it starts to make importing work, but the description annotation doesn't seem to get imported correctly. I'll take a deeper look tomorrow and see where I can get to.

	Mark Banner (:standard8) Assignee
	Comment 23 • 7 years ago

I've still got some cleanup to do, but the one test failure that remains would benefit from bug 1404631 or bug 1373610 being fixed.

test_bookmarks_html_corrupt.js
Unexpected exception Error: Bookmarks.jsm: insertTree: Invalid value for property 'url': "b0rked" at resource://gre/modules/PlacesUtils.jsm:581

	Mark Banner (:standard8) Assignee
	Comment 26 • 7 years ago

I've discovered we don't actually need bug 1373610 nor bug 1404631 to get the tests passing. In pre-reviewing the changes, there was a check for a valid url that had been accidentally dropped. Adding that back in has fixed the test.

	Marco Bonardo [:mak] Reporter
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8914434 [details]
Bug 1095427 - Convert HTML bookmarks import code to the new async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/185734/#review193658

::: commit-message-97efd:1
(Diff revision 4)
> +Bug 1095427 - Convert JSON backups code to the new async Bookmarks.jsm API. r?mak

s/JSON/HTML/

::: toolkit/components/places/BookmarkHTMLUtils.jsm:85
(Diff revision 4)
>  const Container_Unfiled = 3;
>  const Container_Places = 4;
>  
>  const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
>  const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +const CHARSET_ANNO = "URIProperties/characterSet";

CHARSET_ANNO is in PlacesUtils already

::: toolkit/components/places/BookmarkHTMLUtils.jsm:146
(Diff revision 4)
> -  importFromURL: function BHU_importFromURL(aSpec, aInitialImport) {
> +  async importFromURL(aSpec, aInitialImport) {
> -    return (async function() {
> -      notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN, aInitialImport);
> +    notifyObservers(PlacesUtils.TOPIC_BOOKMARKS_RESTORE_BEGIN, aInitialImport);
> -      try {
> +    try {
> -        let importer = new BookmarkImporter(aInitialImport);
> +      let importer = new BookmarkImporter(aInitialImport);
> +

leftover newline?

::: toolkit/components/places/BookmarkHTMLUtils.jsm:184
(Diff revision 4)
> -      try {
> +    try {
> -        if (!(await OS.File.exists(aFilePath))) {
> +      if (!(await OS.File.exists(aFilePath))) {
> -          throw new Error("Cannot import from nonexisting html file: " + aFilePath);
> +        throw new Error("Cannot import from nonexisting html file: " + aFilePath);
> -        }
> +      }
> -        let importer = new BookmarkImporter(aInitialImport);
> +      let importer = new BookmarkImporter(aInitialImport);
> +

leftover newline?

::: toolkit/components/places/BookmarkHTMLUtils.jsm:321
(Diff revision 4)
>    // counter.
>    this._source = aInitialImport ? PlacesUtils.bookmarks.SOURCE_IMPORT_REPLACE :
>                                    PlacesUtils.bookmarks.SOURCE_IMPORT;
> +
> +  // This is the root where bookmarks get imported into by default, if we're not
> +  // doing the initial import.

The comment should also explain the other case.
If I understood this correctly, in one case we just use this root, no the other case we call insertTree on each of the roots that are under this?
Some comment with an overview of the process would be welcome.

::: toolkit/components/places/BookmarkHTMLUtils.jsm:326
(Diff revision 4)
> +  // doing the initial import.
> +  let bookmarkRoot = {
> +    type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +    guid: PlacesUtils.bookmarks.menuGuid,
> +    children: []
> +  };

IIRC, the source should only be set in the root, since we reuse the root source for all the nodes

::: toolkit/components/places/BookmarkHTMLUtils.jsm:328
(Diff revision 4)
> +    type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +    guid: PlacesUtils.bookmarks.menuGuid,
> +    children: []
> +  };
> +
> +  this._bookmarkTree = bookmarkRoot;

nit: why the temp var rather than just assigning to this._bookmarkTree

::: toolkit/components/places/BookmarkHTMLUtils.jsm:419
(Diff revision 4)
> -                                              this._source);
> -    } catch (e) {}
> -  },
>  
> -  /**
> -   * Handles <H1>. We check for the attribute PLACES_ROOT and reset the
> +    let separator = {
> +      source: this._source,

we only need source in the tree root (or better, the one passed to insertTree).

::: toolkit/components/places/BookmarkHTMLUtils.jsm:550
(Diff revision 4)
>        }
>      }
>  
> -    // Save bookmark's last modified date.
> -    if (lastModified) {
> -      frame.previousLastModifiedDate =
> +    let bookmark = {
> +      source: this._source
> +    };

ditto

::: toolkit/components/places/BookmarkHTMLUtils.jsm:663
(Diff revision 4)
> -        PlacesUtils.bookmarks.setItemLastModified(frame.previousId,
> -                                                  frame.previousLastModifiedDate,
> -                                                  this._source);
> -      } catch (e) {
> +        });
> +        frame.previousItem.annos.push({
> +          "name": PlacesUtils.LMANNO_SITEURI,
> +          "flags": 0,
> +          "expires": 4,
> +          "value": frame.previousLink

the site uri is optional. indeed the previous code was checking previousLink.
Probably this case is lacking in tests.

::: toolkit/components/places/BookmarkHTMLUtils.jsm:679
(Diff revision 4)
>        return;
>      }
>      switch (aElt.localName) {
>        case "h1":
> -        this._handleHead1Begin(aElt);
> +        // Nothing to do.
>          break;

we can completely remove "h1" handling, since there's no error in case of default.

::: toolkit/components/places/BookmarkHTMLUtils.jsm:920
(Diff revision 4)
> +
> +      insertFaviconsForTree(tree);
> +    }
>    },
>  
> -  async importFromURL(href) {
> +  _fetchData(href) {

looks like this could be moved to an helper function in the global

::: toolkit/components/places/BookmarkHTMLUtils.jsm:942
(Diff revision 4)
> +  async importFromURL(href) {
> +    let data = await this._fetchData(href);
> +
> +    this._walkTreeForImport(data);
> +
> +    await this._importBookmarks();

nit: the code looks a bit sparse, too many newlines?

::: toolkit/components/places/tests/unit/test_bookmarks_html.js:86
(Diff revision 4)
> -  gBookmarksFileOld = do_get_file("bookmarks.preplaces.html");
> +  gBookmarksFileOld = OS.Path.join(do_get_cwd().path, "bookmarks.preplaces.html");
>  
>    // File pointer to a new Places-exported bookmarks file.
> -  gBookmarksFileNew = Services.dirsvc.get("ProfD", Ci.nsIFile);
> -  gBookmarksFileNew.append("bookmarks.exported.html");
> -  if (gBookmarksFileNew.exists()) {
> +  gBookmarksFileNew = OS.Path.join(OS.Constants.Path.profileDir, "bookmarks.exported.html");
> +  if (OS.File.exists(gBookmarksFileNew)) {
> +    OS.File.remove(gBookmarksFileNew);

shouldn't you await for both exists and remove calls?

	Marco Bonardo [:mak] Reporter
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8914434 [details]
Bug 1095427 - Convert HTML bookmarks import code to the new async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/185734/#review194080

::: toolkit/components/places/BookmarkHTMLUtils.jsm:604
(Diff revision 5)
>  
> -    // Import last charset.
> -    if (lastCharset) {
> +    if (iconUri) {
> +      bookmark.iconUri = iconUri;
> -      let chPromise = PlacesUtils.setCharsetForURI(frame.previousLink, lastCharset, this._source);
> -      this._importPromises.push(chPromise);
>      }

nit: all of the properties like tags, webpanel, charset, keyword, postdata, icon and iconURI, only make sense if bookmark.url is defined, so we could probably just early return for livemarks (so, repeat .push and .previousItem and early return).

::: toolkit/components/places/BookmarkHTMLUtils.jsm:882
(Diff revision 5)
>  
> -  async importFromURL(href) {
> -    this._importPromises = [];
> -    await new Promise((resolve, reject) => {
> -      let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> -                  .createInstance(Ci.nsIXMLHttpRequest);
> +    // If we are importing defaults, we need to separate out the top-level
> +    // default folders into separate items, for the caller to pass into insertTree.
> +    let bookmarkTrees = [this._bookmarkTree];
> +
> +    this._bookmarkTree.children = this._bookmarkTree.children.filter(child => {

what's the point of reassigning to this._bookmarkTree.children? couldn't you just assign to bookmarkTrees?

::: toolkit/components/places/BookmarkHTMLUtils.jsm:905
(Diff revision 5)
> +    if (this._isImportDefaults) {
> +      await PlacesUtils.bookmarks.eraseEverything();
> +    }
> +
> +    let bookmarksTrees = this._getBookmarkTrees();
> +

nit: remove this empty line

::: toolkit/components/places/BookmarkHTMLUtils.jsm:916
(Diff revision 5)
> +      // Give the tree the source.
> +      tree.source = this._source;
> +
> +      await PlacesUtils.bookmarks.insertTree(tree);
> +
> +      insertFaviconsForTree(tree);

nit: remove the empty lines between these 3 code lines

	Mark Banner (:standard8) Assignee
	Comment 31 • 7 years ago
mozreview-review-reply

Comment on attachment 8914434 [details]
Bug 1095427 - Convert HTML bookmarks import code to the new async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/185734/#review194080

> what's the point of reassigning to this._bookmarkTree.children? couldn't you just assign to bookmarkTrees?

The children of this "root" element will contain normal children of the bookmark menu as well as the places roots. Hence, we need to filter out the separate roots, but keep the children that are relevant to the bookmark menu.

I'll add a comment.

	Pulsebot
	Comment 34 • 7 years ago

Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6925cf5fb0c2
Convert HTML bookmarks import code to the new async Bookmarks.jsm API. r=mak

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6925cf5fb0c2

	Marco Bonardo [:mak] Reporter
	Comment 36 • 7 years ago

Thank you Josh and Mark!