572216 - Bookmark Library search doesn't find mobile bookmarks

Status: RESOLVED DUPLICATE of bug 1359984

(Firefox :: Bookmarks & History, defect)

Description

[Ed Lee :Mardak]

Ragavan noticed the iPhone client listed 408 bookmarks while Firefox searching for : found 403 bookmarks.

Turns out the 5 missing bookmarks were located in the Mobile Bookmarks that we sync from Fennec and the Library search doesn't know to look there.

Internally these bookmarks are stored in a mobile folder directly under the bookmarks root.

Comment 1

[Marco Bonardo [:mak]]

IIRC the query we build for that field just searches menu/toolbar/unsorted. we should check if mobile root exists and if so add it to the query.

Comment 2

[Marco Bonardo [:mak]]

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#839

Comment 3

[Joshua Gaines]

Hello, I am looking to gain some development experience by volunteering to Mozilla Project. I would like to take a crack at fixing this bug.

Comment 4

[Joshua Gaines]

Sorry but it appears I am not equipped to recreate this issue as I don't have an android device to create mobile bookmarks through sync. I only have iOS device.

Comment 5

[kevincrane]

Like Joshua, I'm interested in contributing to Firefox for the first time, and this bug looks good to start with. Is there anything special I should know about starting out?

Comment 6

[Marco Bonardo [:mak]]

when searching we search just a group of folders
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#727
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#911
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/bookmarksPanel.js#17

so, we likely need an helper method in PlacesUIUtils to get a list of all of the "roots" we should search into, this list should contain the mobile root or not, depending on its existence. 
The mobile root is created here http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#144 and it should have the "mobile/bookmarksRoot" item annotations

So this special helper should just search the item with mobile/bookmarksRoot annotation, and return an array of roots adding this item if it exists. Caching of the result is suggested.
Then all code points doing searches, as indicated above, should use this helper.

Comment 7

[Marcos Aruj]

Attachment: WIP Patch to show Mobile bookmarks

WIP

Comment 8

[Marcos Aruj]

Hi guys. I've been trying to test this bug and I'm getting some issues when trying to sync bookmarks. This has happened since Friday/Saturday. I'll get back to this one, as I can test any sync now.

Comment 9

[Marcos Aruj]

Hi guys,

Question. I see there's a findMobileRoot method in file 

mozilla-central/services/sync/modules/engines/bookmarks.js

Can I use that method from within PlacesUIUtils.jsm? How can I access it from there?

Thanks

Comment 10

[Marco Bonardo [:mak]]

no, you cannot access to that method, since it's an internal method used by Sync.

You should try to add an util method to PlacesUIUtils to fetch a specific root based on the annotation it has... Unfortunately roots management is a bit fragile and has never been polished, thus these misses.

Comment 11

[Marcos Aruj]

Hi Marco. I've added the util method in PlacesUIUtils. It returns '30' when asked for the mobile root. That one is now returned as part of the bookmark roots if it's present. However, when I open the bookmarks panel (ctrl + shift + O), it is not shown, although the method is called correctly. I changed bookmarksPanel.js to use the new bookmarkRoots getter, as well as all the required callers I could find. Do you know where's the code that handles that Bookmarks Window so I can debug better? I can't seem to find it.

Thanks!

Comment 12

[Marcos Aruj]

No worries, already found it :) Thanks.

Comment 13

[Marcos Aruj]

I may be missing something, but If I add a bookmark from my Android phone and sync it with my desktop firefox, should it be stored somewhere in SQLite or something? Like places.sqlite? I can't seem to find if the sync is working correctly or not.

Thanks for your help.

Comment 14

[Marco Bonardo [:mak]]

yes, sync just uses the Places APIs to add history or bookmarks, those are added to places.sqlite database as any other entry.

PS: when you directly ask questions someone, please use a needinfo field below, otherwise it's possible to miss the questions in the middle of bugmails.

Comment 15

[Marcos Aruj]

I'm still working on this bug, testing with an Android device to see how the sync works and if the bookmarks are correctly retrieved and shown in the desktop firefox. I made a few changes in code to check the what bookmarks are retrieved when syncing on the desktop. I'll post a WIP patch tomorrow.