Hi Mano, can you provide a point value.

Comment on attachment 8490008 [details] [diff] [review] patch Review of attachment 8490008 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/PlacesUIUtils.jsm @@ +376,5 @@ > + return PlacesTransactions.Move(info); > + } > + default: > + if (aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) > + throw new Error("Copying containers from old versions isn't supported"); "old version" as defined in this method is unclear, could you please expand it a little bit, something like "coming from the legacy transactions code" or "generated by deprecated transactions code" would already be clearer. I think I don't like the switch falling back to the default case, it is confusing and adds too much indentation. what about going for a plain if? if ("itemGuid" in aData) { if (aDatatype != PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER && aDatatype != PlacesUtils.TYPE_X_MOZ_PLACE && aDatatype != PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR) { throw new Error... } return copy or move } all the rest... note that controller has placesFlavors and GENERIC_VIEW_DROP_TYPES, you may copy them to PlacesUIUtils, then here use this.PLACES_FLAVORS.indexOf(aDataType) (probably also rename GENERIC_VIEW_DROP_TYPES to SUPPORTED_FLAVORS). you might also split GENERIC_VIEW_DROP_TYPES to URL_FLAVORS and make SUPPORTED_FLAVORS a merge of PLACES_FLAVORS and URL_FLAVORS (concat should be fine, even with the dupes) feel free to find better names :) ::: browser/components/places/content/controller.js @@ +72,5 @@ > + // TODO mano: callers can pass the title. > + get tagName() { > + if (!this.isTag) > + throw new Error("tagName getter called for non-tag insertion point"); > + return PlacesUtils.getItemTitle(this.itemId); This doesn't seem very async-ready... I'm not even sure we have a PlacesUtils.getItemTitle... is this missing a .bookmarks? I see that the problem is you didn't support tag by id, that's fine, but we need a way to allow this info to be fetched from the result or asynchronously. Since the callpoints are setting isTag, at this point let's make them set a tag name instead of a boolean, we always have such information at hand, should be a simple change to do here. @@ +139,1 @@ > case "placesCmd_new:folder": why did you remove placesCmd_cut and placesCmd_paste? are those handled by a different controller, or is this (http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/placesOverlay.xul#82) no more valid? @@ +1365,2 @@ > > // Cut/past operations are not repeatable, so clear the clipboard. please while here fix the typo, should be Cut&Paste

Comment on attachment 8491612 [details] [diff] [review] fix treeView.js caller Review of attachment 8491612 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I have only one doubt, we have some b-c tests testing paste(), but they are not waiting for it to complete (now that it's async) Did you try to run places b-c tests with async transactions yet? for example browser_416459_cut.js, browser_410196_paste_into_tags.js and browser_435851_copy_query.js It might be worth a bug, or we'll look into that in the final bug that will flip the pref. ::: browser/components/places/PlacesUIUtils.jsm @@ +351,5 @@ > + * @param aData > + * The unwrapped data blob of dropped or pasted data. > + * @param aType > + * The content type of the data. > + * @param aNewParentGUID Guid @@ +361,5 @@ > + * > + * @returns a Places Transaction that can be passed to > + * PlacesTranactions.transact for performing the move/insert command. > + */ > + getTransactionForData: function(aData, aType, aNewParentGUID, aIndex, aCopy) { aNewParentGuid ::: browser/components/places/content/controller.js @@ +1350,1 @@ > while here please remove this trailing space ::: browser/components/places/content/menu.xml @@ +104,5 @@ > return dropPoint; > } > + > + let tagName = PlacesUtils.nodeIsTagQuery(elt._placesNode) ? > + elt._placesNode.title : null; indent 2 more ::: browser/components/places/content/tree.xml @@ +529,5 @@ > if (PlacesControllerDragHelper.disallowInsertion(container)) > return null; > > + let tagName = PlacesUtils.nodeIsTagQuery(container) ? > + container.title : null; indent 2 more.

https://hg.mozilla.org/mozilla-central/rev/6ef192784187 https://hg.mozilla.org/mozilla-central/rev/014f231ac173