Closed Bug 1125374 Opened 7 years ago Closed 7 years ago

Bookmarks.fetch() doesn't support DEFAULT_INDEX

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.1 - 26 Jan

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

While working on bug 1094846 I found tests that were trying to fetch bookmarks by specifying index=DEFAULT_INDEX.
I saw the other methods using |parent._childCount| instead of modifying the query... Let me know if that's what you think we should do.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8553991 - Flags: review?(mak77)
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
which test is that one?
Cause I'm not sure we have so compelling use-cases to fetch the last bookmark in a folder...
Ah, yeah I thought that might have been a conscious decision to leave it out.

browser/components/places/tests/unit/test_browserGlue_smartBookmarks.js is using it quite a lot.
I'm not thrilled by having to support a further input option, but looks like some code might find this useful, for example when we append a bookmark to a view the current code uses getIdForItemAt with DEFAULT_INDEX. so for porting code to the new API it might be useful.

You cannot use _childCount here cause we don't fetch info about the parent for this API call. Nor you can use the _childCount in the query cause it refers to the currently fetched bookmark.

Though, what you can do is changing the expression in the query to

AND b.position = IFNULL(:index, (SELECT count(*) - 1 FROM moz_bookmarks WHERE parent = p.id))

and then as param pass

index: info.index == this.DEFAULT_INDEX ? null : info.index
Comment on attachment 8553991 [details] [diff] [review]
0001-Bug-1125374-Let-Bookmarks.fetch-support-DEFAULT_INDE.patch

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

apart changing the query, the patch looks good.

::: toolkit/components/places/Bookmarks.jsm
@@ +934,5 @@
>       LEFT JOIN moz_bookmarks p ON p.id = b.parent
>       LEFT JOIN moz_keywords k ON k.id = b.keyword_id
>       LEFT JOIN moz_places h ON h.id = b.fk
> +     WHERE p.guid = :parentGuid ${position}
> +    `, params);

as I said, you can use the suggested query.
Attachment #8553991 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> > +     WHERE p.guid = :parentGuid ${position}
> > +    `, params);
> 
> as I said, you can use the suggested query.

Will do, thx!
https://hg.mozilla.org/mozilla-central/rev/ce36da2702b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.