1095411 - PlacesUtils.wrapNode is using synchronous getKeywordForBookmark

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

We could leave this issue to a later time, the problem is that we likely cannot make wrapNode async...

Comment 1

[Asaf Romano (gone)]

Once PT is on we can do away with that. The new Move and Copy transactions rely merely on the itemGuid property, ignoring everything else.

Comment 2

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

I'm honestly uncertain this will be SO easy, cause D&D is involved that means manual testing of a bunch of cases. let's say 3, even I'd be more for 5.

Comment 3

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

picking up this

Comment 4

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

Attachment: patch v1

I'm sorry, this one may not be easy to review, cause I preferred refactoring this piece of code. There's a reason though.

Once upon a time, wrapNode was shared code used by copy&paste, drag&drop, sync, html exports, json backups.
Slowly we moved to better solutions and today the code is only used for copy&paste and drag&drop.
When I converted the export/json code, I didn't bother touching this code cause the patches were large enough.

But that means the code is doing far more than we need for just copy&paste and drag&drop
1. it doesn't need to mark roots
2. it doesn't need the charset (cause they are uri bound and the uri never goes away, so it never gets lost)
3. it doesn't need the keywords (same reason as the charset)
4. it doesn't need tens of sub functions calls
5. it's non-trivial-at-all to make this async, so the smaller the better

Additionally, when we will move to PlacesTransactions, it won't even need to collect children of containers. It doesn't indeed make sense to collect all children of a folder, just to move it around... Even when copying, the folder id is enough to find its children. PlacesTransactions use promiseBookmarksTree.
Due to the removal of .children, I had to add a small hack to the PlacesUIUtils code, to fetch children when needed. I tried to limit changes there.
I preferred to hack PlacesUIUtils rather than keeping .children, cause this hack will go away when we enable new transactions, and we'll be fine already.

So, based on this, I could simplify a lot the wrapNode code, it's about half the size.

Finally while testing the patch through the UI, I found an existing bug with tag containers, I filed Bug 1160193 and for now I've just added a workaround to disallow dangerous operation on title-less tags.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa0884db8fd8

Comment 6

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

Attachment: patch v1.1

Just an unbitrot

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8604127 [details] [diff] [review]
patch v1.1

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

Very tricky to review... didn't see any red flags. Guess we should do some manual testing too for it?

::: browser/components/places/PlacesUIUtils.jsm
@@ +212,5 @@
> +                                                    PlacesUtils.LMANNO_FEEDURI);
> +        let node = PlacesUtils.unwrapNodes(
> +          PlacesUtils.wrapNode(child, PlacesUtils.TYPE_X_MOZ_PLACE, isLivemark),
> +          PlacesUtils.TYPE_X_MOZ_PLACE
> +        ).shift();

Maybe: let [node] = PlacesUtils.unwrapNodes(...);

@@ +253,3 @@
>        let transactions = [];
> +      if (!aData.livemark && aData.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
> +        let root = PlacesUtils.getFolderContents(aData.id, false, false).root;

Can be fancy here too:

let {root} = PlacesUtils.getFolderContents(aData.id, false, false);

@@ +267,5 @@
>      if (aData.livemark && aData.annos) { // Copying a livemark.
>        return this._getLivemarkCopyTransaction(aData, aContainer, aIndex);
>      }
>  
> +    let root = PlacesUtils.getFolderContents(aData.id, false, false).root;

let {root} = PlacesUtils.getFolderContents(aData.id, false, false);

::: toolkit/components/places/PlacesUtils.jsm
@@ +148,5 @@
> +    data.dateAdded = aNode.dateAdded;
> +    data.lastModified = aNode.lastModified;
> +
> +    let annos = PlacesUtils.getAnnotationsForItem(data.id);
> +    if (annos.length != 0)

Nit: maybe if (annos.length > 0) ?

@@ +158,5 @@
> +    NetUtil.newURI(aNode.uri);
> +
> +    // Tag root accepts only folder nodes.
> +    if (data.parent == PlacesUtils.tagsFolderId)
> +      throw new Error("Unexpected node type");

Are the comment, condition, and error message in sync? Maybe I'm just not following it well enough. Feel free to ignore if so.

@@ +186,5 @@
> +        data.type = PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER;
> +      }
> +    }
> +    else {
> +      // This is a grouped container query, dinamically generated.

Nit: dynamically

@@ +594,5 @@
> +      // escape out potential HTML in the title
> +      let escapedTitle = node.title ? htmlEscape(node.title) : "";
> +
> +      if (aFeedURI) {
> +        return "<A HREF=\"" + aFeedURI + "\">" + escapedTitle + "</A>" + NEWLINE;

Could use a template string.

@@ +616,5 @@
> +        node.containerOpen = wasOpen;
> +        return childString + "</DL>" + NEWLINE;
> +      }
> +      if (PlacesUtils.nodeIsURI(node))
> +        return "<A HREF=\"" + node.uri + "\">" + escapedTitle + "</A>" + NEWLINE;

Template string?

Comment 8

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

Attachment: patch v1.2

Comment 9

[Pulsebot]

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

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f79a8bc7b3c2