1067954 - Async Places Transactions: Paste functionality

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Marco Mucci [:MarcoM]]

Hi Mano, can you provide a point value.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 4

[Asaf Romano (gone)]

Attachment: patch

Comment 5

[Asaf Romano (gone)]

Attachment: fix treeView.js caller

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 7

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/6ef192784187

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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