PlacesUtils.wrapNode is using synchronous getKeywordForBookmark

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Depends on 1 bug)

Trunk
mozilla41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We could leave this issue to a later time, the problem is that we likely cannot make wrapNode async...
Flags: qe-verify-
Flags: firefox-backlog+
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.
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.
Points: 2 → 3
Blocks: 1140395
No longer blocks: placesAsyncBookmarks
No longer blocks: 1141547
picking up this
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8599937 - Flags: review?(ttaubert)
Posted patch patch v1.1 (obsolete) — Splinter Review
Just an unbitrot
Attachment #8599937 - Attachment is obsolete: true
Attachment #8599937 - Flags: review?(ttaubert)
Attachment #8604127 - Flags: review?(ttaubert)
Iteration: 40.3 - 11 May → 41.1 - May 25
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?
Attachment #8604127 - Flags: review?(ttaubert) → review+
Posted patch patch v1.2Splinter Review
Attachment #8604127 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f79a8bc7b3c2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1177654
Depends on: 1386513
No longer depends on: 1386513
You need to log in before you can comment on or make changes to this bug.