Comment on attachment 8529953 [details] [diff] [review] patch Review of attachment 8529953 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsINavHistoryService.idl @@ +288,5 @@ > */ > readonly attribute long long folderItemId; > + > + /** > + * For both simple folder nodes and simple-folder-query nodes, this is set out of curiosity, I wonder what's the difference between simple folder nodes and simple folder-query nodes, since folders are not nsINavHistoryQueryResultNode... do we really have such different things? ::: toolkit/components/places/nsNavHistory.cpp @@ +3924,5 @@ > NS_ENSURE_SUCCESS(rv,rv); > > + if (resultNode->IsFolder()) { > + // For folder shortcuts, mBookmarkGuid (like mItemId) points to the target > + // folder, and that's already set at this point by QueryRowToResult. Ok, here I'm lost. the only point I can find where we set mBookmarkGuid is RowToResult... and it's here. I cannot find a point in QueryRowToResult where we set mBookmarkGuid. I'm also not sure why we are adding mQueryGuid instead of using mBookmarkGuid. I was assuming we were going to use mBookmarkGuid for the query guid, and mTargetGuid for the concrete guid. That would be more coherent imo. honestly I don't care what we did for targetFolderId and queryItemId, it sounds much more confusing and I'd be happy if we'd also make that consistent, but I figure it may bring in regressions (in case we could land after the merge to get more testing).

Comment on attachment 8531808 [details] [diff] [review] patch.diff Review of attachment 8531808 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsINavHistoryService.idl @@ +282,5 @@ > */ > readonly attribute nsINavHistoryQueryOptions queryOptions; > > /** > + * For both folder nodes and simple-folder-query nodes, this is set to the what about s/simple-folder-query/folder shortcut/? @@ +288,5 @@ > */ > readonly attribute long long folderItemId; > + > + /** > + * For both folder nodes and simple-folder-query nodes, this is set to the ditto ::: toolkit/components/places/nsNavHistoryResult.cpp @@ +3077,5 @@ > + *aItemId = mTargetFolderItemId; > + return NS_OK; > +} > + > + trailing space @@ +3461,5 @@ > { > for (int32_t i = 0; i < mChildren.Count(); ++i) { > if (mChildren[i]->mItemId == aItemId || > (mChildren[i]->IsFolder() && > + mChildren[i]->GetAsFolder()->mTargetFolderItemId == aItemId)) { So, I think this is confusing (an yes, it was already) and could be worth to change it... in most cases we only care about the "external" id when using FindChildById... the only cases where we actually care about searching the concrete id is onItemChanged and onItemVisited. Having this code checking both external and concrete id may cause interesting bugs in all the other cases, for example, we remove a folder, in onItemRemoved the FindChildById call could find a shortcut to that folder instead (this would not actually cause a bug but makes us run useless code). While that may not be much better, probably we should distinguish somehow the 2 cases, either with a bool argument or a separate FindChildByIdOrConcreteId method. What do you think? @@ +3613,5 @@ > { > // We only care about notifications when a child changes. When the deleted > // item is us, our parent should also be registered and will remove us from > // its list. > + if (mTargetFolderItemId == aItemId) I feel like somehow the comment here is wrong. I think we are only registered for changes to the concrete id, I suppose, so the deleted item is not us, it's our target. we should never get a notification that the external id is being removed. Indeed I'd probably suggest to add such assertion aItemId != mItemId and improve the comment. ::: toolkit/components/places/tests/unit/test_1105866.js @@ +31,5 @@ > + Assert.strictEqual(PlacesUtils.asQuery(shortcutNode).targetFolderGuid, > + PlacesUtils.bookmarks.menuGuid); > + > + unfiledRoot.containerOpen = false; > +}); could you please also test adding a common folder? (not a shortcut)

I filed bug 1107691 for the FindChildById issue. I prefer not to change the current behavior because the chance for regression is very high and we don't have the resources for that at the moment.

forget a file there. https://tbpl.mozilla.org/?tree=Try&rev=67c6dd2d46b9 / https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=67c6dd2d46b9

Comment on attachment 8532231 [details] [diff] [review] patch Review of attachment 8532231 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/nsINavHistoryService.idl @@ +283,5 @@ > readonly attribute nsINavHistoryQueryOptions queryOptions; > > /** > + * For both simple folder queries and folder shortcut queries, this is set to > + * the concrete itemId of the folder. Otherwise, this is set to -1. I think we should try to clarify what concrete means here, something like "the concrete itemId (i.e. for folder shortcuts it's the target folder id) of the folder." @@ +289,5 @@ > readonly attribute long long folderItemId; > + > + /** > + * For both simple folder queries and folder shortcut queries, this is set to > + * the concrete guid of the folder. Otherwise, this is set to an empty string. ditto ::: toolkit/components/places/nsNavHistory.cpp @@ +4009,5 @@ > const nsACString& aTitle, > uint32_t aAccessCount, PRTime aTime, > const nsACString& aFavicon, > nsNavHistoryResultNode** aNode) > { We could MOZ_ASSERT here to check aBookmarkGuid is not an empty string ::: toolkit/components/places/nsNavHistoryResult.cpp @@ +3504,5 @@ > PRTime aDateAdded, > const nsACString& aGUID, > const nsACString& aParentGUID) > { > + NS_ASSERTION(aParentFolder == mTargetFolderItemId, "Got wrong bookmark update"); if possible, should convert to MOZ_ASSERT (be sure Try is happy though) @@ +3610,5 @@ > const nsACString& aGUID, > const nsACString& aParentGUID) > { > + // If this folder is a folder shortcut, we should never be notified for the > + // removal of the shortcut (the parent node would be). trailing space @@ +3619,3 @@ > return NS_OK; > > + NS_ASSERTION(aParentFolder == mTargetFolderItemId, "Got wrong bookmark update"); MOZ_ASSERT as well ::: toolkit/components/places/nsNavHistoryResult.h @@ +732,5 @@ > // after the container is closed until a notification comes in > bool mContentsValid; > > + // If the node is generated from a place:folder=X query, this is the target > + // folder item-id and itemGuid. For regular folder nodes, they are set to the "target folder id and GUID" (let's stop repeating item everywhere)

https://tbpl.mozilla.org/?tree=Try&rev=740a019d528b / https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=740a019d528b

I had to update the non-empty-guid assert to cover the itemId==-1 case: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ff70807ce847 / https://tbpl.mozilla.org/?tree=Try&rev=ff70807ce847