Closed Bug 988070 Opened 10 years ago Closed 10 years ago

Make getBookmarksTree use WITH RECURSIVE to allow it to return only subtrees

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32

People

(Reporter: mak, Assigned: asaf)

References

Details

(Keywords: dev-doc-needed, Whiteboard: p=3 s=it-32c-31a-30b.2 [qa!])

Attachments

(2 files, 9 obsolete files)

we can use WITH RECURSIVE to make recursive queries who will return subtrees of bookmarks starting from a given one. This would allow a lot of code paths to get bookmarks information without having to go crazy with old json methods.

Moreover we may then reuse the same trick in future to get all ancestors/descendants of a given bookmark with a single query...
for example, this gets all of the children of a given id (used 3, bookmarks toolbar, as an example), containers first, then their contents, sorted by position.
We can tweak the ORDER BY to visit the tree differently.

WITH RECURSIVE
descendants(id, title, level, parent, position) AS (
  SELECT id, title, 0, parent, position FROM moz_bookmarks where id = 3
  UNION ALL
  SELECT b.id, b.title, level+1, b.parent, b.position
    FROM moz_bookmarks b JOIN descendants ON b.parent =descendants.id
    ORDER BY 3, b.parent, b.position
) SELECT * FROM descendants;

while this gets the ancestors of a given id

WITH RECURSIVE
  ancestors(id, title, parent) AS (
    SELECT id, title, parent FROM moz_bookmarks where id = 3
    UNION ALL
    SELECT b.id, b.title, b.parent
      FROM moz_bookmarks b JOIN ancestors ON ancestors.parent =b.id
  )
SELECT * FROM ancestors;
Whiteboard: p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Following the triage meetings, this remains unassigned. Mak, would you be willing to mentor a community member through this bug?
Flags: needinfo?(mak77)
Whiteboard: p=0 → p=0 [diamond]
I could mentor it, sure, but keep in mind this is not trivial. It involves managing an adjacency tree in SQL and hooking it up with the current js function (+ writing tests for it)
Flags: needinfo?(mak77)
ok, so I got clarifications about [diamond] bugs (I wasn't aware of).
I'm not sure this classifies as a diamond cause it's blocking one of the goals (Places async transactions) and thus it would be better to fix this sooner than later. If PMs disagree with this, I can mentor it, we'll have to workaround its miss though.
Whiteboard: p=0 [diamond] → [diamond] p=3
Assignee: nobody → mano
Status: NEW → ASSIGNED
Whiteboard: [diamond] p=3 → [diamond] p=3 s=it-31c-30a-29b.3 [qa?]
Attached patch wip (obsolete) — Splinter Review
Attachment #8413692 - Flags: feedback?
Attachment #8413692 - Flags: feedback? → feedback?(mak77)
Hi Liz, can you review to determine if this bug will require QA verification.
Flags: needinfo?(lhenry)
Whiteboard: [diamond] p=3 s=it-31c-30a-29b.3 [qa?] → [diamond] p=3 s=it-32c-31a-30b.1 [qa?]
It'll - QA should verify there are no regressions in bookmark backups.
Whiteboard: [diamond] p=3 s=it-32c-31a-30b.1 [qa?] → p=3 s=it-32c-31a-30b.1 [qa?]
Marco, I'm asking for feedback rather for few reasons:
* Lack of documentation
* The method should be moved to PlacesUtils
* There's some backup-specific stuff in there (EXCLUDE_FROM_BACKUP_ANNO)
* I'm not happy with the way I handled the "IN" operator.
* The object created by sqliteRowToBookmarkObject should be generalized somehow, so that we can also use it as input elsewhere.

All of this is quite trivial though.
BookmarksJSONUtils is probably a better candidate than PlacesUtils.
QA Contact: florin.mezei
Whiteboard: p=3 s=it-32c-31a-30b.1 [qa?] → p=3 s=it-32c-31a-30b.1 [qa+]
QA Contact: florin.mezei → andrei.vaida
(In reply to Mano from comment #9)
> BookmarksJSONUtils is probably a better candidate than PlacesUtils.

As discussed over IRC, I think BookmarkJsonUtils and BookmarkHTMLUtils should stick to be modules handling whatever is needed to export/import to the specified files. This is a more general util that is not part of that process, it just happens to be used. As we said, either PlacesUtils, or if it's growing it may deserve its own module.

The Bookmark object may be adding overhead (that overhead is multiplied by #of bookmarks in many cases, thousands of bookmarks is not unlikely), we should evaluate very well if it's worth it or not.
Comment on attachment 8413692 [details] [diff] [review]
wip

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +515,5 @@
> +      "UNION ALL " +
> +      "SELECT b.fk, level + 1, b.type, b.id, b.guid, b.parent, descendants.guid, " +
> +      "b.position, IFNULL(b.title, ''), b.dateAdded, b.lastModified " +
> +      "FROM moz_bookmarks b JOIN descendants ON b.parent =descendants.id " +
> +      ") " + 

the first part of the query needs better indentation... I wonder if you could move the parentGuid to the second part to simplify this one.

@@ +534,5 @@
> +      "FROM descendants d " +
> +      "LEFT JOIN moz_bookmarks p ON p.id = d.parent " +
> +      "LEFT JOIN moz_places h ON h.id = d.fk " +
> +      "LEFT JOIN moz_favicons f ON f.id = h.favicon_id " +
> +      "ORDER BY d.parent, d.position";

Are you sure about this ordering? The scope of doing the order by in the UNION ALL subquery was to enforce visiting the tree Depth-first, by doing so we could remove the possibility to add children before parents (thus simplifying the code), but looks like you are not using that option this way.
Is there a specific reason for that?
Attachment #8413692 - Flags: feedback?(mak77)
Whiteboard: p=3 s=it-32c-31a-30b.1 [qa+] → p=3 s=it-32c-31a-30b.2 [qa+]
Attached patch checkpoint (obsolete) — Splinter Review
Attachment #8413692 - Attachment is obsolete: true
Attachment #8421715 - Flags: feedback?
Attachment #8421715 - Flags: feedback? → feedback?(mak77)
Comment on attachment 8421715 [details] [diff] [review]
checkpoint

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

it seems ok, you should try to replace the current getBookmarksTree usage with this and run tests against it.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1603,5 @@
> +   *   having the same set of properties as above.
> +   *
> +   * @note although the tags "root" is technically a child of the places root,
> +   * it's not treated as such by this API, meaning that when the places root
> +   * is queried, the tags root is excluded.

I think we should omit this, and start thinking tags as not folders, everywhere. We should not even hint consumers they can think them as folders.

@@ +1634,5 @@
> +      }
> +
> +      switch (type) {
> +        case Ci.nsINavBookmarksService.TYPE_BOOKMARK:
> +          // TODO: What about shortcuts?

likely nothing, just handle as bookmarks as we do.

@@ +1637,5 @@
> +        case Ci.nsINavBookmarksService.TYPE_BOOKMARK:
> +          // TODO: What about shortcuts?
> +          item.type = PlacesUtils.TYPE_X_MOZ_PLACE;
> +          // This will throw if we try to serialize an invalid url and the node will
> +          // just be skipped.

where does this happen? I can't find a try/catch handling this

@@ +1698,5 @@
> +      "FROM descendants d " +
> +      "LEFT JOIN moz_bookmarks b3 ON b3.id = d.parent " +
> +      "LEFT JOIN moz_places h ON h.id = d.fk " +
> +      "LEFT JOIN moz_favicons f ON f.id = h.favicon_id " +
> +      "ORDER BY d.level, d.parent, d.position";

my question from comment 11 is still unanswered, are you sure this ORDER BY causes visiting the tree in a depth-first order if put in this position rather than into the recursive query? (see comment 1) I didn't verify this.
Attachment #8421715 - Flags: feedback?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #13)

> my question from comment 11 is still unanswered, are you sure this ORDER BY
> causes visiting the tree in a depth-first order if put in this position
> rather than into the recursive query? (see comment 1) I didn't verify this.

I don't think it makes any difference. Either you sort in the "inside" query, and then the items are "naturally" sorted, or you don't sort internally and then you've to sort in the "outside query". Same same, unless there's a performance difference I'm unaware of.
Attached patch Complete API + test (obsolete) — Splinter Review
The only thing missing here, afaict, is the conversion of the backup consumer. It's quite trivial, but I need to figure out if the dual return value is necessary).
Attachment #8421715 - Attachment is obsolete: true
Attachment #8425400 - Flags: review?(mak77)
Attached patch Complete API + test (obsolete) — Splinter Review
I just had to break something in the very last minute.

Note to self: I still need to test title-less bookmark and fix PlacesTransactions to support them properly.
Attachment #8425400 - Attachment is obsolete: true
Attachment #8425400 - Flags: review?(mak77)
Attachment #8425401 - Flags: review?(mak77)
(In reply to Mano from comment #14)
> I don't think it makes any difference. Either you sort in the "inside"
> query, and then the items are "naturally" sorted, or you don't sort
> internally and then you've to sort in the "outside query". Same same, unless
> there's a performance difference I'm unaware of.

I find it strange since this is recursive, ordering inside or outside should make a difference. Btw, as far as we don't reintroduce bug 992901 that's ok.
Attached patch patch (obsolete) — Splinter Review
PlacesBackups consumer fixed. test_bookmarks_json passes.
Attachment #8425401 - Attachment is obsolete: true
Attachment #8425401 - Flags: review?(mak77)
Attachment #8425495 - Flags: review?(mak77)
Comment on attachment 8425495 [details] [diff] [review]
patch

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

::: toolkit/components/places/PlacesBackups.jsm
@@ +505,5 @@
> +    let rootGUID = yield PlacesUtils.promiseItemGUID(PlacesUtils.placesRootId);
> +    let root = yield PlacesUtils.promiseBookmarksTree(rootGUID, {
> +      excludeItemsCallback: aItem => {
> +        return aItem.annos &&
> +          aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);

So, this means we have to go through each single entry to check if it has the annotation. I'm a little bit worried for performances, cause excluded bookmarks are an handful of ids in the worst case, but to remove this tiny set we spend a non-trivial amount of time: even if it's 1ms, it may still be for thousands of bookmarks.

Couldn't we rather pass an array/Set of ids/guids and filter with a much simpler check? or a filterItemsWithAnno option... it's some more queries but should be quite faster when there are many bookmarks. If we'd have a getItemGuidsWithAnnotation would be even better (we wouldn't need id->guid for each entry) :(

::: toolkit/components/places/PlacesUtils.jsm
@@ +1558,5 @@
> +   * Asynchronously retrieve a JS-object representation of a places bookmarks
> +   * item (a bookmark, a folder, or a separator) along with it all of its
> +   * descendants.
> +   *
> +   * @param [optioanl aItemGUID

typo and missing ]

@@ +1575,5 @@
> +   *
> +   * @resolves to a JS object that represent either a single item or a
> +   * bookmarks tree.  Each node in the tree has the following properties set:
> +   * - guid (string): the item's unique-id (same as aItemGUID for the top item).
> +   *   [deprecated] id (number): the item's id. Only use it if you must. It'll

sounds like it's missing a - at the beginning?

@@ +1581,5 @@
> +   * - type (number):  the item's type.  @see PlacesUtils.TYPE_X_*
> +   * - title (string): the item's title. If it has no title, this property
> +   *   isn't set.
> +   * - dateAdded (number, microseconds from the epoch): the date-added value of
> +   *   the item (microseconds from the epoch).

"microseconds from the epoch" is repeated twice

@@ +1585,5 @@
> +   *   the item (microseconds from the epoch).
> +   * - lastModified (number, microseconds from the epoch): the last-modified
> +   *   value of the item.
> +   * - annos (see getAnnotationsForItem): the item's annotations (unset if there
> +   *   are no annotations set for the item).

period after "the item's annotations" and remove parenthesis (it's the only one using them)?

@@ +1588,5 @@
> +   * - annos (see getAnnotationsForItem): the item's annotations (unset if there
> +   *   are no annotations set for the item).
> +   *
> +   * The root object (i.e. the one for aItemGUID) also has the following:
> +   * properties:

typo: following: properties:

@@ +1591,5 @@
> +   * The root object (i.e. the one for aItemGUID) also has the following:
> +   * properties:
> +   * - parentGUID (string): the guid of the root's parent.  This isn't set if
> +   *   the root item is the places root.
> +   * - itemsCount (number, not enumerable): the number of items, including the

if you want to simplify the code, I'd be fine making it enumerable and short its name to just "count".  even if we are going to write it into the json it's just set on the root so it's an ignorable overhead.

@@ +1595,5 @@
> +   * - itemsCount (number, not enumerable): the number of items, including the
> +   *   root item itself, which are represented in the resolved object.
> +   *
> +   * property, referencing the item's parent.  This property isn't set if the
> +   * entire places root was queried.

typo? I'm not sure where this phrase comes from

@@ +1611,5 @@
> +   *   having the same set of properties as above.
> +   *
> +   * @note although the tags "root" is technically a child of the places root,
> +   * it's not treated as such by this API, meaning that when the places root
> +   * is queried, the tags root is excluded.

please annotate that this is because tags will change to not be folders anymore (bug 424160)

@@ +1643,5 @@
> +
> +      switch (type) {
> +        case Ci.nsINavBookmarksService.TYPE_BOOKMARK:
> +          item.type = PlacesUtils.TYPE_X_MOZ_PLACE;
> +          // This will throw if we try to serialize an invalid url and the node will

nit: probably should change the word "serialize", it made sense when saving to json, but this API is more generic.

@@ +1710,5 @@
> +
> +    if (!aItemGUID)
> +      aItemGUID = yield this.promiseItemGUID(PlacesUtils.placesRootId);
> +
> +    let dbFilePath = OS.Path.join(OS.Constants.Path.profileDir, "places.sqlite");

please annotate here the bug number to provide a shared Sqlite.jsm places connection in PlacesUtils and cache the statements.

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +112,5 @@
>  NS_IMETHODIMP
>  nsNavHistoryResultNode::GetIcon(nsACString& aIcon)
>  {
>    if (mFaviconURI.IsEmpty()) {
> +    aIcon.SetIsVoid(true);

does this work properly in our views?
For example PTV_getImageSrc with this change will return null, does treeview support that?
in the other views we set the "src" attribute to .icon, that means are are doing a setAttribute("src", null) that doesn't sound too nice...

::: toolkit/components/places/tests/unit/test_promiseBookmarksTree.js
@@ +185,5 @@
> +    aInfo.title = "Test Item (folder) " + itemsCount;
> +  return yield PlacesTransactions.transact(PlacesTransactions.NewFolder(aInfo));
> +}
> +
> +function* promiseSetIcon(aPageURI, aIconURI) {

may be useful in head_common.js, as promiseSetIconForPage (make arguments coherent with naming in such a case: icon, page)

@@ +210,5 @@
> +  }
> +  return item;
> +}
> +
> +function* test_item(aItemGUID = "", aOptions, aExcludedGUIDs) {

this may have a better name, test_item is really generic...

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +140,5 @@
>  skip-if = os == "android"
>  [test_utils_backups_create.js]
>  [test_utils_getURLsForContainerNode.js]
>  [test_utils_setAnnotationsFor.js]
> +[test_promiseBookmarksTree.js]

please keep alphabetic ordering
Attachment #8425495 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #19)

> ::: toolkit/components/places/PlacesBackups.jsm
> @@ +505,5 @@
> > +    let rootGUID = yield PlacesUtils.promiseItemGUID(PlacesUtils.placesRootId);
> > +    let root = yield PlacesUtils.promiseBookmarksTree(rootGUID, {
> > +      excludeItemsCallback: aItem => {
> > +        return aItem.annos &&
> > +          aItem.annos.find(a => a.name == PlacesUtils.EXCLUDE_FROM_BACKUP_ANNO);
> 
> So, this means we have to go through each single entry to check if it has
> the annotation. I'm a little bit worried for performances, cause excluded
> bookmarks are an handful of ids in the worst case, but to remove this tiny
> set we spend a non-trivial amount of time: even if it's 1ms, it may still be
> for thousands of bookmarks.
> 
> Couldn't we rather pass an array/Set of ids/guids and filter with a much
> simpler check? or a filterItemsWithAnno option... it's some more queries but
> should be quite faster when there are many bookmarks. If we'd have a
> getItemGuidsWithAnnotation would be even better (we wouldn't need id->guid
> for each entry) :(

I for one find the callback quite useful, and the guid/id filtering options somewhat limited.  However, I agree that my current solution is not well optimized. Let's discuss this over #places.

By the way, I think I'm going to make the id property an opt-in (to be used by PlacesBackups and sync, I guess).


> 
> @@ +1585,5 @@
> > +   *   the item (microseconds from the epoch).
> > +   * - lastModified (number, microseconds from the epoch): the last-modified
> > +   *   value of the item.
> > +   * - annos (see getAnnotationsForItem): the item's annotations (unset if there
> > +   *   are no annotations set for the item).
> 
> period after "the item's annotations" and remove parenthesis (it's the only
> one using them)?

There were quite a few later on. Fixed.

> @@ +1591,5 @@
> > +   * The root object (i.e. the one for aItemGUID) also has the following:
> > +   * properties:
> > +   * - parentGUID (string): the guid of the root's parent.  This isn't set if
> > +   *   the root item is the places root.
> > +   * - itemsCount (number, not enumerable): the number of items, including the
> 
> if you want to simplify the code, I'd be fine making it enumerable and short
> its name to just "count".  even if we are going to write it into the json
> it's just set on the root so it's an ignorable overhead.
>

Personally, I don't like this property at all, but since I don't want to change the backup file name format I kept it in. Do you think there are other use cases for this? If there are, it makes sense to make it a normal property. Otherwise I'm fine with the current setup.

On a side note, my current variant of bookmarks wrapping in PlacesTransations uses a flat array rather than a tree. If we break the current json format sometime in the future, it's worth considering.

> @@ +1611,5 @@
> > +   *   having the same set of properties as above.
> > +   *
> > +   * @note although the tags "root" is technically a child of the places root,
> > +   * it's not treated as such by this API, meaning that when the places root
> > +   * is queried, the tags root is excluded.
> 
> please annotate that this is because tags will change to not be folders
> anymore (bug 424160)

Oooops... I forgot to remove the comment per your first suggestion. I sure don't mind doing so. Do you still prefer that?


> @@ +1710,5 @@
> > +
> > +    if (!aItemGUID)
> > +      aItemGUID = yield this.promiseItemGUID(PlacesUtils.placesRootId);
> > +
> > +    let dbFilePath = OS.Path.join(OS.Constants.Path.profileDir, "places.sqlite");
> 
> please annotate here the bug number to provide a shared Sqlite.jsm places
> connection in PlacesUtils and cache the statements.

Or maybe I'll just do that either. We shall see.
 
> ::: toolkit/components/places/nsNavHistoryResult.cpp
> @@ +112,5 @@
> >  NS_IMETHODIMP
> >  nsNavHistoryResultNode::GetIcon(nsACString& aIcon)
> >  {
> >    if (mFaviconURI.IsEmpty()) {
> > +    aIcon.SetIsVoid(true);
> 
> does this work properly in our views?
> For example PTV_getImageSrc with this change will return null, does treeview
> support that?
> in the other views we set the "src" attribute to .icon, that means are are
> doing a setAttribute("src", null) that doesn't sound too nice...

I'll take it out then.
(In reply to Mano from comment #20)
> > please annotate that this is because tags will change to not be folders
> > anymore (bug 424160)
> 
> Oooops... I forgot to remove the comment per your first suggestion. I sure
> don't mind doing so. Do you still prefer that?

Oh right! let's just remove the comment then and make not-tags the common behavior.
Attached patch patch (obsolete) — Splinter Review
Attachment #8425495 - Attachment is obsolete: true
Attachment #8426404 - Flags: review?(mak77)
Flags: needinfo?(lhenry)
Things I forgot in the last patch:
1) Add a warning about the usage of excludeItemsCallback
2) Take care of the void-titles mess
3) make itemIds opt-in for this method (I may save that for a follow up, or not do it at all).
Attached patch patch (obsolete) — Splinter Review
Add the warning; take care of void-titles.
Attachment #8426404 - Attachment is obsolete: true
Attachment #8426404 - Flags: review?(mak77)
Attachment #8426509 - Flags: review?(mak77)
Comment on attachment 8426509 [details] [diff] [review]
patch

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

would be an r+, but the PlacesUtils changes to id->guid and guid->id are missing pieces.

::: toolkit/components/places/PlacesBackups.jsm
@@ +513,3 @@
>  
> +    // Report the time taken to build the tree. This doesn't take into
> +    // account the time spent in the query since that's off the main-thread.

now this time takes into account the query time, since that is done in a different code piece and we yield.
And actually we could have taken into account that even before... Dunno, probably I was drunk or just dumb.
Btw, please just remove the second paragraph.

::: toolkit/components/places/PlacesUtils.jsm
@@ +1305,5 @@
>  
>    /**
> +   * Gets the shared Sqlite.jsm readonly connection to the Places database.
> +   * This is intended to be used mostly internally, and by other Places modules.
> +   * Outside the Places component, it should be used only as a last resort. 

trailing space

@@ +1307,5 @@
> +   * Gets the shared Sqlite.jsm readonly connection to the Places database.
> +   * This is intended to be used mostly internally, and by other Places modules.
> +   * Outside the Places component, it should be used only as a last resort. 
> +   * Keep in mind the Places DB schema is by no means frozen or even stable.
> +   * Your custom queries can - and will - break overtime.

<3

@@ +1564,5 @@
> +  promiseItemId: function (aGUID) GUIDHelper.getItemId(aGUID),
> +
> +  /**
> +   * Asynchronously retrieve a JS-object representation of a places bookmarks
> +   * item (a bookmark, a folder, or a separator) along with it all of its

"along with all of"

@@ +1585,5 @@
> +   *          this option can slow down the process significantly if the
> +   *          callback does anything that's not relatively trivial. It's highly
> +   *          recommended to avoid any synchronous I/O or DB queries.
> +   *
> +   * @resolves to a JS object that represent either a single item or a

"represents"

@@ +1598,5 @@
> +   *   the item.
> +   * - lastModified (number, microseconds from the epoch): the last-modified
> +   *   value of the item.
> +   * - annos (see getAnnotationsForItem): the item's annotations.  This is not
> +   *   set if there are no annotations set for the item).

please indent these one space more

@@ +1605,5 @@
> +   * properties set:
> +   * - parentGUID (string): the guid of the root's parent.  This isn't set if
> +   *   the root item is the places root.
> +   * - itemsCount (number, not enumerable): the number of items, including the
> +   *   root item itself, which are represented in the resolved object.

ditto

@@ +1612,5 @@
> +   * - uri (string): the item's url.
> +   * - tags (string): csv string of the bookmark's tags.
> +   * - charset (string): the last known charset of the bookmark.
> +   * - keyword (string): the bookmark's url (unset if none).
> +   * - iconuri (string): the bookmark's favicon url.

ditto

@@ +1616,5 @@
> +   * - iconuri (string): the bookmark's favicon url.
> +   *
> +   * The last four properties are not set at all if they're irrelevant (e.g.
> +   * |charset| is not set if no charset was previously set for the bookmark
> +   * url).

I'd probably remove the newline above this paragraph to emphasize it's related to the previous one, with more indentation for properties it should stay readable

@@ +1723,5 @@
> +    let conn = yield this.promiseDBConnection();
> +
> +    let excludedParents = new Set();
> +    let shouldExcludeItem = (aItem, aParentGUID) => {
> +      if ("excludeItemsCallback" in aOptions) {

I wonder if wouldn't be better to call shouldExcludeItem only if excludeItemsCallback is defined, from a perf point of view, so basically move the check outside the method and in the method assume we have a callback.

@@ +1734,5 @@
> +        return exclude;
> +      }
> +    };
> +
> +    let rootItem = null, rootItemCreateionEx = null;

CreateionEx sounds like a typo (copyPasted below...) I assume it was rootItemCreationEx (I don't like much this name, but I don't care that much)

apart that, may we use rootObj and stop abusing the item word here?

@@ +1735,5 @@
> +      }
> +    };
> +
> +    let rootItem = null, rootItemCreateionEx = null;
> +    let parents = new Map();

what about parentsMap?

@@ +1741,5 @@
> +      yield conn.executeCached(QUERY_STR,
> +          { tags_folder: PlacesUtils.tagsFolderId,
> +            charset_anno: PlacesUtils.CHARSET_ANNO,
> +            item_guid: aItemGUID }, (aRow) => {
> +        let item;

and the same along the whole method, I really would like to avoid confusing item as we defined until today with this js object that represents a bookmark.
I suggest item => bookmarkObj,
rootItem => rootObj,
createItemInfoObject => createBookmarkObject

@@ +1756,5 @@
> +          }
> +
> +          // not enumerable
> +          Object.defineProperty(rootItem, "itemsCount",
> +                                { value: 1, writable: true });

instead of commenting like that, please specify all properties in options, also enumerable and configurable, better be explicit.

@@ +1904,5 @@
> +  let connPromise = Sqlite.cloneStorageConnection({
> +    connection: PlacesUtils.history.DBConnection,
> +    readOnly: true
> +  });
> +  connPromise.then((conn) => {

nit: I think you could even:
return Sqlite.clone()
             .then()

or even avoid the return since it's an arrow function...

@@ +1956,5 @@
> +    if (rows.length == 0)
> +      throw new Error("no item found for the given itemId");
> +
> +    return rows[0].getResultByName("guid");
> +  }),

I like how Sqlite.jsm usage has shrinked this code...
Though, shouldn't you add the found values to the idsForGUIDs and GUIDsForIds caches?
You also forgot the this.ensureObservingRemovedItems(); part

@@ +2000,5 @@
>   * Inactive windows commands will be updated on focus.
>   */
>  function updateCommandsOnActiveWindow()
>  {
> +  let win = Services.focus.activeWindow;

while here could you please also remove focus manager from PlacesUIUtils (http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/PlacesUIUtils.jsm#1052) ?
Attachment #8426509 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #25)

> @@ +1734,5 @@
> > +        return exclude;
> > +      }
> > +    };
> > +
> > +    let rootItem = null, rootItemCreateionEx = null;
> 
> CreateionEx sounds like a typo (copyPasted below...) I assume it was
> rootItemCreationEx (I don't like much this name, but I don't care that much)
> 
> apart that, may we use rootObj and stop abusing the item word here?
...
> @@ +1741,5 @@
> > +      yield conn.executeCached(QUERY_STR,
> > +          { tags_folder: PlacesUtils.tagsFolderId,
> > +            charset_anno: PlacesUtils.CHARSET_ANNO,
> > +            item_guid: aItemGUID }, (aRow) => {
> > +        let item;
> 
> and the same along the whole method, I really would like to avoid confusing
> item as we defined until today with this js object that represents a
> bookmark.
> I suggest item => bookmarkObj,
> rootItem => rootObj,
> createItemInfoObject => createBookmarkObject

Problem is, it's just as confusing, maybe even more, to refer to a folder or a separator as a bookmark.  And here it's even worse because we distinguish between bookmarks from folders in the documentation ("Bookmarks items also have the following...").  Of course, the method name itself (promiseBookmarksTree) suffers from this problem, but for some reason I'm not as disturbed when it's used in plural form.  Go figure.

Since this is a tree, I could go with "node" ;)
(In reply to Marco Bonardo [:mak] from comment #25)

> @@ +1723,5 @@
> > +    let conn = yield this.promiseDBConnection();
> > +
> > +    let excludedParents = new Set();
> > +    let shouldExcludeItem = (aItem, aParentGUID) => {
> > +      if ("excludeItemsCallback" in aOptions) {
> 
> I wonder if wouldn't be better to call shouldExcludeItem only if
> excludeItemsCallback is defined, from a perf point of view, so basically
> move the check outside the method and in the method assume we have a
> callback.

Hrm, we'd need to have a boolean for that. I could switch to hasOwnProperty though, which should be nearly as fast as reading a local variable holding the "in" result (however, given that how flat the prototype chain of the incoming options object is - usually it'd be just an object literal - I doubt it can make any difference).
Attached patch patch (obsolete) — Splinter Review
* item/bookmark - untouched for now.
* hasOwnProperty - done.
* I'm not sure I like gAsyncDBConnPromised revised version.
Attachment #8426509 - Attachment is obsolete: true
Attached patch patch.diff (obsolete) — Splinter Review
removed the comment in PlacesBackups.
Attachment #8426800 - Attachment is obsolete: true
I'll either take this to a new bug or include it in the copy&paste/d&d patch.
Comment on attachment 8426801 [details] [diff] [review]
patch.diff

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

::: toolkit/components/places/PlacesUtils.jsm
@@ +1722,5 @@
> +    let conn = yield this.promiseDBConnection();
> +
> +    let excludedParents = new Set();
> +    let shouldExcludeItem = (aItem, aParentGUID) => {
> +      if (aOptions.hasOwnProperty("excludeItemsCallback")) {

As discussed on IRC, it's not about using "in" or hasOwnProperty, it's about not calling shouldEcludeItem at all of a callback is not provided,
something like
if (hasExcludeCallback && shouldExcludeItem)

@@ +1901,5 @@
>           createBundle(PLACES_STRING_BUNDLE_URI);
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised", () => {
> +  return Sqlite.cloneStorageConnection({

nit: I think you may omit braces and return and just
XPCOMUtils.defineLazyGetter(this, "gAsyncDBConnPromised",
  () => Sqlite.cloneStorageConnection({
Attachment #8426801 - Flags: review+
Attached patch patchSplinter Review
Attachment #8426801 - Attachment is obsolete: true
Attachment #8426865 - Flags: superreview?(gavin.sharp)
Comment on attachment 8426865 [details] [diff] [review]
patch

>diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm

>+   * Bookmark items also have the following properties:

>+   *  - keyword (string): the bookmark's url (unset if none).

This looks incorrect?

Looks good otherwise. Are there MDN docs for this? If so make sure they get updated.
Attachment #8426865 - Flags: superreview?(gavin.sharp) → superreview+
Let's see if the old magic works (https://developer.mozilla.org/en/docs/Places_utilities_for_JavaScript wasn't updated for quite a while).

Thanks!
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/d791ac083c16
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Regression testing around bookmark backups, exports and imports using Nightly 32.0a1 (2014-05-22) [1] found no new bugs. Mano, if there's anything else I should look at here, please let me know.

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.2 [qa!]
Depends on: 1016831
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: