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)
Toolkit
Places
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)
9.96 KB,
patch
|
Details | Diff | Splinter Review | |
44.51 KB,
patch
|
Gavin
:
superreview+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•10 years ago
|
||
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;
Updated•10 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: p=0
Updated•10 years ago
|
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog?
Updated•10 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Comment 2•10 years ago
|
||
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]
Reporter | ||
Comment 3•10 years ago
|
||
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)
Reporter | ||
Comment 4•10 years ago
|
||
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.
Updated•10 years ago
|
Whiteboard: p=0 [diamond] → [diamond] p=3
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mano
Updated•10 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [diamond] p=3 → [diamond] p=3 s=it-31c-30a-29b.3 [qa?]
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8413692 -
Flags: feedback?
Assignee | ||
Updated•10 years ago
|
Attachment #8413692 -
Flags: feedback? → feedback?(mak77)
Comment 6•10 years ago
|
||
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?]
Assignee | ||
Comment 7•10 years ago
|
||
It'll - QA should verify there are no regressions in bookmark backups.
Assignee | ||
Updated•10 years ago
|
Whiteboard: [diamond] p=3 s=it-32c-31a-30b.1 [qa?] → p=3 s=it-32c-31a-30b.1 [qa?]
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
BookmarksJSONUtils is probably a better candidate than PlacesUtils.
Updated•10 years ago
|
QA Contact: florin.mezei
Whiteboard: p=3 s=it-32c-31a-30b.1 [qa?] → p=3 s=it-32c-31a-30b.1 [qa+]
Updated•10 years ago
|
QA Contact: florin.mezei → andrei.vaida
Reporter | ||
Comment 10•10 years ago
|
||
(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.
Reporter | ||
Comment 11•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: p=3 s=it-32c-31a-30b.1 [qa+] → p=3 s=it-32c-31a-30b.2 [qa+]
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8413692 -
Attachment is obsolete: true
Attachment #8421715 -
Flags: feedback?
Assignee | ||
Updated•10 years ago
|
Attachment #8421715 -
Flags: feedback? → feedback?(mak77)
Reporter | ||
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Reporter | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
PlacesBackups consumer fixed. test_bookmarks_json passes.
Attachment #8425401 -
Attachment is obsolete: true
Attachment #8425401 -
Flags: review?(mak77)
Attachment #8425495 -
Flags: review?(mak77)
Reporter | ||
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Reporter | ||
Comment 21•10 years ago
|
||
(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.
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8425495 -
Attachment is obsolete: true
Attachment #8426404 -
Flags: review?(mak77)
Updated•10 years ago
|
Flags: needinfo?(lhenry)
Assignee | ||
Comment 23•10 years ago
|
||
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).
Assignee | ||
Comment 24•10 years ago
|
||
Add the warning; take care of void-titles.
Attachment #8426404 -
Attachment is obsolete: true
Attachment #8426404 -
Flags: review?(mak77)
Attachment #8426509 -
Flags: review?(mak77)
Reporter | ||
Comment 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
(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" ;)
Assignee | ||
Comment 27•10 years ago
|
||
(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).
Assignee | ||
Comment 28•10 years ago
|
||
* item/bookmark - untouched for now. * hasOwnProperty - done. * I'm not sure I like gAsyncDBConnPromised revised version.
Attachment #8426509 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
removed the comment in PlacesBackups.
Attachment #8426800 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
I'll either take this to a new bug or include it in the copy&paste/d&d patch.
Reporter | ||
Comment 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
Attachment #8426801 -
Attachment is obsolete: true
Attachment #8426865 -
Flags: superreview?(gavin.sharp)
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
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
Assignee | ||
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d791ac083c16
https://hg.mozilla.org/mozilla-central/rev/d791ac083c16
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 37•10 years ago
|
||
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!]
You need to log in
before you can comment on or make changes to this bug.
Description
•