1125374 - Bookmarks.fetch() doesn't support DEFAULT_INDEX

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

While working on bug 1094846 I found tests that were trying to fetch bookmarks by specifying index=DEFAULT_INDEX.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1125374-Let-Bookmarks.fetch-support-DEFAULT_INDE.patch

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.

Comment 2

[Marco Bonardo [:mak]]

which test is that one?
Cause I'm not sure we have so compelling use-cases to fetch the last bookmark in a folder...

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 4

[Marco Bonardo [:mak]]

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 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(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!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 8

[Carsten Book [:Tomcat]]

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