647605 - More considered handling of mobile root query in bookmarks engine

Status: RESOLVED INCOMPLETE

(Firefox :: Sync, defect, P3)

Description

[r0polach]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0

1) When I open menu: Bookmarks / Show All Bookmarks, double-clicking "All bookmarks" does not expand the item (so I cannot move bookmarks, create folder etc. in this dialog), although I know there are many and subfolders... (I see them when I click menu Bookmarks directly... - I can also drag them directly in menu Bookmarks, create folders by right-clicking directly in menu etc.)

2) When I bookmark a page, I can select a folder in a list of few (5) last used folders (in the combobox), but clicking the "v" sign on the right of the combobox opens only empty folder list (and disabled "New folder" button)

Reproducible: Always

Steps to Reproduce:
1. open menu: Bookmarks / Show All Bookmarks
2. double-clicking "All bookmarks"

Actual Results:  
item does not expand, bookmarks manipulation imposible in this dislog

Expected Results:  
item expands...

The problem starts by no clear reason. Until today, everything was perfect in Firefox 4.0 (Ubuntu 10.10). I did not force any extension update or anything...

Comment 1

[r0polach]

I disabled all extensions and restart firefox, but it does not help

Comment 2

[r0polach]

Note: firefox is installed through repo:
http://ppa.launchpad.net/mozillateam/firefox-stable/ubuntu maverick main

Comment 3

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110407 Firefox/4.2a1pre

Can you please verify your issue using the oficial release package available here: http://www.mozilla.com/en-US/firefox/new/

Thanks!

Comment 4

[George Carstoiu]

Reporter, did you manage to verify what I suggested in Comment 3?

Comment 5

[George Carstoiu]

Marking issue as Resolved Incomplete. Reporter, feel free to reopen when you can provide more information.

Comment 6

[r0polach]

Sorry for a delay. Now I tried run firefox from official package,
and got the same result. I tried to take screenshot,
but PrintScreen key closes the bookmark "dialog" every time...

Comment 7

[r0polach]

Attachment: screenshot of bookmark dialog with empty content and disabled "new folder" button

Ok, I found how to make delayed screenshot, so here it is...

Comment 8

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110420 Firefox/6.0a1

r0polach, can you also try reproducing this issue on a clean profile, just so as to exclude any problems caused by one of your extensions:
http://support.mozilla.com/en-US/kb/Managing%20profiles

Thanks!

Comment 9

[Marco Bonardo [:mak]]

Looks like your database has some bogus entries, and that's really strange.
If your places.sqlite file is not too large I'd like to check it, since it contains a lot of your personal information don't attach it here, rather could you please send it to me by mail?

Comment 10

[r0polach]

I tried it on a clean new profile and there is no problem there.

On my default profile: The places.sqlite file is 20MiB. The strange thing is
that when I deleted most of bookmarks and all the history (I made a backup of my
.mozilla folder before..) it still takes 20MiB and it still contain deleted 
history items - even those I deleted with "forget about this site". I wonder
if it is due to a bug (the same as this one or another) or it is a default
way how firefox takes care of my privacy ;)
...nevertheless I can send you the file gzipped (1.6MiB) or 7zipped (1MiB)
if you are comfortable with the size.

Comment 11

[r0polach]

I tried VACUUM places.sqlite. It shrinks only to 10MiB, but it seems to
not contain deleted information at least few items I tried to find.
Also gzipped it takes only 43K, so I am sending it to you by mail.
All the bug is still completely reproducible with this places.sqlite

Comment 12

[Marco Bonardo [:mak]]

sounds like a case of corrupt db, btw I got the file, will check it soon.

Comment 13

[Marco Bonardo [:mak]]

This is caused by a folder shortcut pointing to a not existing folder, could have been caused by Sync, bug 641074 will ensure we don't bail out on these.

For this specific db you could do this:
Open the error console and evaluate this code:
Components.utils.import("resource://gre/modules/PlacesUtils.jsm");PlacesUtils.bookmarks.removeItem(615);

Unfortunately I fear this issue could become common, the broken shortcut was the mobile root :(

Philipp, looks like Sync is not annotating the mobile root correctly? (it should be annotated with EXCLUDE_FROM_BACKUP_ANNO and ORGANIZER_QUERY_ANNO)
also _ensureMobileQuery doesn't ensure that the shortcut is valid.

Comment 14

[r0polach]

Thanks Marco for a way to workaround this. It works.

Comment 15

[r0polach]

It happened again.
Is there any way to get the id of item causing problem?

Comment 16

[r0polach]

And it happened one more time on different firefox instance on other machine.
Bookmarks menu starts to behave strangely: Bookmarks moved by mouse landed elsewhere than dropped. After I restored bookmarks from backup, the bookmarks menu gets really messy: "The bookmarks toolbar" menu item is on the bottom of the menu instead of top, and others items like "Show all bookmarks" and "Bookmark this page" too. I tried several other backups but I cannot get anyone work correctly.

Please help!

Comment 17

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #13)

> Philipp, looks like Sync is not annotating the mobile root correctly? (it
> should be annotated with EXCLUDE_FROM_BACKUP_ANNO and ORGANIZER_QUERY_ANNO)
> also _ensureMobileQuery doesn't ensure that the shortcut is valid.

Clearing out some old tabs, thought I'd respond to this.

The bookmarks engine does this:

    // Add the mobile bookmarks query if it doesn't exist
    else if (mobile.length == 0) {
      let query = PlacesUtils.bookmarks.insertBookmark(all[0], queryURI, -1, title);
      PlacesUtils.annotations.setItemAnnotation(query, ORGANIZERQUERY_ANNO, MOBILE_ANNO, 0,
                                  PlacesUtils.annotations.EXPIRE_NEVER);
      PlacesUtils.annotations.setItemAnnotation(query, EXCLUDEBACKUP_ANNO, 1, 0,
                                  PlacesUtils.annotations.EXPIRE_NEVER);
    }

to my eyes, that looks like it's adding the correct annos.

Do we need to do this even if the mobile root already exists?

Is there anything that we need to change here? 

https://hg.mozilla.org/services/services-central/file/default/services/sync/modules/engines/bookmarks.js#l1400

Thanks!

Comment 18

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #17)
> to my eyes, that looks like it's adding the correct annos.

that has been fixed in bug 648364, after my comment here.

> Do we need to do this even if the mobile root already exists?

it probably doesn't matter at this point, also because FF8 will update the library leftpane versione, regenerating it

> Is there anything that we need to change here? 

The only possible improvement is to check whether the existing query is pointing to a valid id, and if not delete and regenerate it. I suppose you may compare the existing uri with queryURI and see if it's the same.

Comment 19

[Richard Newman [:rnewman]]

(In reply to Marco Bonardo [:mak] from comment #18)

> The only possible improvement is to check whether the existing query is
> pointing to a valid id, and if not delete and regenerate it. I suppose you
> may compare the existing uri with queryURI and see if it's the same.

That sounds like worthwhile investment to me and philikon. Probably a good second bug for one of our new hires; marking as [good first bug] just to avoid having stupid searches :D

Morphing, because the original user concern seems to have drifted substantially.

Comment 20

[Richard Newman [:rnewman]]

I think mak incorrectly referenced Bug 648364 in Comment 18 thanks to blame-changing commit https://hg.mozilla.org/services/services-central/rev/aa2c607672e1.

Removing.

Comment 21

[Marco Bonardo [:mak]]

yes I did, still I think that has been addressed after I looked at the code, somewhere else.

Comment 22

[sunnybunny8920]

[Tracking Requested - why for this release]:

Comment 23

[Chris Karlof [:ckarlof]]

Next steps are to reproduce and investigate.

Comment 24

[Chris Karlof [:ckarlof]]

Kit, maybe take this?

Comment 25

[Lina Butler [:lina]]

Mark and I discussed this on Vidyo yesterday. I think there are a couple of things we'll want to do:

* Remove the "excludeFromBackup" anno from the mobile root. The mobile *query* should have it, since it's a left pane query like https://dxr.mozilla.org/mozilla-central/rev/e27fe24a746fa839f1cabe198faf1bad42c7dc4b/browser/components/places/PlacesUIUtils.jsm#1111-1129, but the *root* shouldn't.

* Move the mobile query creation into PlacesUIUtils.jsm, since it already has logic for setting the correct annotations and recovering from corruption.

This might be folded in to bug 1274496 (or bug 1258127), but taking it for now.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #25)
> * Move the mobile query creation into PlacesUIUtils.jsm, since it already
> has logic for setting the correct annotations and recovering from corruption.

A complication with that might be that we only want the query shown when there are mobile bookmarks. IOW, I don't think we can create this query when PlacesUIUtils creates the rest of its special queries (unless there is already some facility for hiding it when there are no results, or we are willing to add that)

On the flip side though, it sounds like it might be possible to create the folder at any convenient time - without the query the folder itself isn't visible. This also means we can reduce the complexity in Sync for the creation of the folder itself.

Comment 27

[Marco Bonardo [:mak]]

(In reply to Mark Hammond [:markh] from comment #26)
> A complication with that might be that we only want the query shown when
> there are mobile bookmarks. IOW, I don't think we can create this query when
> PlacesUIUtils creates the rest of its special queries (unless there is
> already some facility for hiding it when there are no results, or we are
> willing to add that)

it may also be fine to hide it "visually", if that's not too complex.

Comment 28

[Lina Butler [:lina]]

Attachment: Bug 647605 - Create a mobile bookmarks Places root and query.

Review commit: https://reviewboard.mozilla.org/r/62564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62564/

Comment 29

[Lina Butler [:lina]]

A start at making mobile bookmarks a real Places root, moving the query into PlacesUIUtils, and adding an annotation (not currently used) that'll hide it if it's empty.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/62564/#review59592

looking like a good first-cut to me.

::: toolkit/components/places/Database.cpp:1832
(Diff revision 1)
> +
> +  nsresult rv = CreateMobileRoot();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  {
> +    nsCOMPtr<mozIStorageStatement> stmt;

is this needed?

::: toolkit/components/places/Database.cpp:1855
(Diff revision 1)
> +      "LIMIT 1"
> +    ), getter_AddRefs(stmt));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    mozStorageStatementScoper scoper(stmt);
> +
> +    rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("anno_name"),

It seems you might as well just put that literal in the sql?

::: toolkit/components/places/Database.cpp:1873
(Diff revision 1)
> +  if (oldRootId <= 0) {
> +    return NS_OK;
> +  }
> +
> +  int64_t newRootId;
> +  rv = GetIdForGuid(mMainConn, NS_LITERAL_CSTRING("mobile______"), &newRootId);

This seems to be the only use of this new function - I wonder if you could change getMobileRoot() to take an optional out arg that receives the id of the item it just created?

Also, is there any reason you can't just update the guid of an existing mobile root in-place?

Comment 31

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8768240 [details]
Bug 647605 - Create a mobile bookmarks Places root and query.

Hi Mak, when you have a chance, can you please comment on the general strategy here? The code is far from complete and will take a few iterations to get right, but I think it's worth getting feedback on the strategy itself so we don't waste time on something that will not fly.

IIUC, Kit is proposing:

* Making the "mobile root" a real root, with a guid of "mobile______", that's auto-created along with the other roots (and created for existing users as part of a schema upgrade) - this code is in the patch, and while I think it might need some tweaking, the general idea would be the same as in the patch.

* In PlacesUIUtils, unconditionally create a new "left pane folder" that shows all items under the mobile root - but add a new annotation on that folder of "PlacesOrganizer/HideIfEmpty" - this code is in the patch.

* Presumably implement special support for that new annotation - so only show the folder itself if the query returns some results - but that's not in this patch.

* We'd change Sync to never create the mobile root nor the query, and we'd unconditionally use the new "mobile______" GUID, in the same way we unconditionally use the special GUIDs for other roots - this also isn't in this patch.

Is that what you had in mind when we were discussing this, and think this would be landable when complete?

Comment 32

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/62564/#review59592

Thanks in advance, Mark and Mak! :-) I wanted to clean this up a bit before asking for feedback, but it'll be good to have it sooner than later.

> is this needed?

Some of the Places migration tests failed because the `moz_anno_attributes` table didn't exist. I didn't investigate further yet.

> It seems you might as well just put that literal in the sql?

Yes. :-)

> This seems to be the only use of this new function - I wonder if you could change getMobileRoot() to take an optional out arg that receives the id of the item it just created?
> 
> Also, is there any reason you can't just update the guid of an existing mobile root in-place?

Sure. I broke it out because `MigrateV34Up` was getting quite lengthy, but I like your suggestion. I didn't update the root in-place because I wanted to clean up tags, annos (including our favorite `places/excludeFromBackup`), and other references that don't make sense for a root. But, if that's not a concern, this can be simpler. Mak, WDYT?

Comment 33

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #31)
> IIUC, Kit is proposing:

FTR, Mark's summary of my proposal is spot on.

Comment 34

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/62564/#review62200

The only doubt I have is about the "hideIfEmpty" annotation, from a performance point of view, since it requires synchronous I/O.
I am thinking maybe we should make it different, in the UI code we can detect if an element is a root (by guid) and then just set a special attribute (or a cell property in trees), without having to read and write anything. Since otherwise we should query that annotation when we query the bookmark node, and it start becoming hairy. Is there any other reason to use an anno, like synchronizing it or such?

::: toolkit/components/places/Database.cpp:1834
(Diff revision 1)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  {
> +    nsCOMPtr<mozIStorageStatement> stmt;
> +    nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "SELECT item_id FROM moz_anno_attributes"

Something is wrong here... that table doesn't have an item_id column.

::: toolkit/components/places/Database.cpp:1850
(Diff revision 1)
> +    rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "SELECT a.item_id "
> +      "FROM moz_anno_attributes n "
> +      "JOIN moz_items_annos a ON n.id = a.anno_attribute_id "
> +      "WHERE n.name = :anno_name "
> +      "LIMIT 1"

limit 1 is not needed, you executeStep just once.

::: toolkit/components/places/Database.cpp:1874
(Diff revision 1)
> +    return NS_OK;
> +  }
> +
> +  int64_t newRootId;
> +  rv = GetIdForGuid(mMainConn, NS_LITERAL_CSTRING("mobile______"), &newRootId);
> +  NS_ENSURE_SUCCESS(rv, rv);

you don't need this, you can use a subquery in the query iself (parent = (SELECT id FROM moz_bookmarks WHERE guid = ...)

::: toolkit/components/places/Database.cpp:1884
(Diff revision 1)
> +    rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "UPDATE moz_bookmarks SET "
> +        "parent = :newId, "
> +        "position = (SELECT count(*) FROM moz_bookmarks "
> +          "WHERE parent = :newId) "
> +      "WHERE parent = :oldId"

Sync won't see any of these changes, since we are not notifying from here. Is that ok?

::: toolkit/components/places/Database.cpp:1902
(Diff revision 1)
> +
> +  // Remove the old root.
> +  {
> +    nsCOMPtr<mozIStorageStatement> stmt;
> +    rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +      "DELETE FROM moz_bookmarks WHERE id = :id"

This is not enough, you should also remove any leftover item annotations that may point to this id.

::: toolkit/components/places/nsNavBookmarks.h:284
(Diff revision 1)
>  
>    inline bool IsRoot(int64_t aFolderId) {
>      return aFolderId == mRoot || aFolderId == mMenuRoot ||
>             aFolderId == mTagsRoot || aFolderId == mUnfiledRoot ||
> -           aFolderId == mToolbarRoot;
> +           aFolderId == mToolbarRoot || aFolderId == mMobileRoot;
>    }

IIRC there is a similar method in PlacesUtils.

::: toolkit/components/places/nsNavBookmarks.cpp:329
(Diff revision 1)
>    return NS_OK;
>  }
>  
>  
> +NS_IMETHODIMP
> +nsNavBookmarks::GetMobileFolder(int64_t* aRoot)

Bookmarks.jsm needs to be updated too

Comment 36

[Lina Butler [:lina]]

Thanks for the initial feedback, Mak! I'm running into some complexity around hiding the "Mobile Bookmarks" query if it's empty.

My first thought was to check if the mobile root has any children, and hide the query depending on that...but I don't see a clear place to add that check. There's places.js, browserPlacesViews.js, and treeView.js, and I guess they all create different queries and register observers to update themselves.

My next thought was to add an "excludeIfEmpty" query option, and exclude the query from the result set if it's empty. But that doesn't feel right, either; it requires modifying nsNavHistoryResult and nsNavHistoryQuery to carry along and manage this flag, and it's not the right answer if we use an empty "Mobile Bookmarks" to promote Sync.

The other option is to always show it, even if it's empty. This is how other roots behave, though it's a bit annoying to have an undeletable query in the left pane if you don't have any mobile bookmarks. But it does sidestep a lot of this complexity, and makes the patch a trivial migration.

(We also talked about using "Mobile Bookmarks" to advertise Sync, just like Synced Tabs. I'm not sure offhand how that would work, given that the tree widget manages the left and right panes, and we'd still need an exception for the mobile root to show a different element in the deck).

Could you offer some guidance, please? Thanks!

Comment 37

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge [:kitcambridge] from comment #36)
> Thanks for the initial feedback, Mak! I'm running into some complexity
> around hiding the "Mobile Bookmarks" query if it's empty.

The broader solution would be to have a property on a container node that tells if it's empty or not... Unfortunately the container doesn't know until it is actually oepened, and opening it just to check if it's empty could be expensive. A solution would be to query for emptyness when we query for the container, but again it has a cost and then the data must be brought forward and kept up-to-date. For a single use-case, doesn't sound worth it.

Since it's just for the Library, we could do a one-time query when the library opens, and set an hide attribute on the mobile root if it's empty. We could keep it updated or not, but I don't think it's critical that we do, it's not a window people tend to keep open forever.

> (We also talked about using "Mobile Bookmarks" to advertise Sync, just like
> Synced Tabs. I'm not sure offhand how that would work, given that the tree
> widget manages the left and right panes, and we'd still need an exception
> for the mobile root to show a different element in the deck).

Imo so far this is the best proposed solution, we can stack a box on the right pane and show it when the mobile root is selected and the right pane is empty, hide it otherwise. I wouldn't mind some special code for this in the Library. All the code managing left/right panes is in places.js so it should be trivial to detect when the the right pane loads the mobile query.

Comment 38

[Lina Butler [:lina]]

Comment on attachment 8768240 [details]
Bug 647605 - Create a mobile bookmarks Places root and query.

Marking this patch as obsolete because I moved the mobile root work to bug 1302901. Once that lands, we can use this bug for the query.

Comment 39

[Lina Butler [:lina]]

I'm not actively working on this. We discussed adding a "sign in to Sync to see your mobile bookmarks" overlay in the product meeting (see also bug 1296564), but it's on the backburner for now.

Comment 40

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #38)
> Comment on attachment 8768240 [details]
> Bug 647605 - Create a mobile bookmarks Places root and query.
> 
> Marking this patch as obsolete because I moved the mobile root work to bug
> 1302901. Once that lands, we can use this bug for the query.

From what I can tell, it seems your work landed. How would I go about using this bug for the query and/or continuing the work you were doing? If that's alright of course :)

Comment 41

[Lina Butler [:lina]]

Hi Leni, sorry for the delay! You're most welcome to pick this up, I think it's going to be simpler than I expected. AFAICT, in bug 1295237 (and bug 1372927, comment 4), we decided to always show an entry for Mobile Bookmarks on Desktop, even if you haven't synced any.

It's not clear to me if we want to hide the folder until the first time you sign in to Sync, though. Ryan, do you have an opinion here?

In the future, we might add a message to "sign in to Sync on Android or iOS to see your mobile bookmarks here", or "bookmarks you add on Android or iOS will show up here" message if you're already signed in. However, I don't think we have designs for that at the moment, and I'm not entirely sure how we'd make that overlay work with the existing XUL tree in the Library window. That's another question for Ryan, too. :-)

Comment 42

[Lina Butler [:lina]]

Leni, once Ryan gets back to us, the approach I'd suggest is to make a new left pane query:

* Add a query for Mobile Bookmarks in `PlacesUIUtils` (https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/PlacesUIUtils.jsm#1129-1147 and https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/PlacesUIUtils.jsm#1307-1349). The `MobileBookmarksFolderTitle` string already exists, so you can use it for `concreteTitle`.

* Bump `ORGANIZER_LEFTPANE_VERSION`.

* Add a case for your new `MobileBookmarks` query to https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/tests/browser/browser_library_left_pane_fixnames.js#69-82, so we can verify it does the right thing if the query is renamed. Also, we should add a check to `onLibraryReady` and make sure that Sync doesn't track those changes. (The query GUID shouldn't exist in the object returned by `PlacesSyncUtils.bookmarks.pullChanges`). You'll need to import `PlacesSyncUtils.jsm`, and maybe `PlacesUtils.jsm`, in https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/tests/browser/head.js#1-6.

* Remove `_ensureMobileQuery` (https://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/services/sync/modules/engines/bookmarks.js#1049-1093).

* `mach test browser/components/places toolkit/components/places services/sync` should run all the tests we care about.

Does that make sense? Thanks for taking this on!

Comment 43

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #42)
> Leni, once Ryan gets back to us, the approach I'd suggest is to make a new
> left pane query:
> 
> * Add a query for Mobile Bookmarks in `PlacesUIUtils`
> (https://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/
> PlacesUIUtils.jsm#1129-1147 and
> https://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/
> PlacesUIUtils.jsm#1307-1349). The `MobileBookmarksFolderTitle` string
> already exists, so you can use it for `concreteTitle`.
> 
> * Bump `ORGANIZER_LEFTPANE_VERSION`.
> 
> * Add a case for your new `MobileBookmarks` query to
> https://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/tests/
> browser/browser_library_left_pane_fixnames.js#69-82, so we can verify it
> does the right thing if the query is renamed. Also, we should add a check to
> `onLibraryReady` and make sure that Sync doesn't track those changes. (The
> query GUID shouldn't exist in the object returned by
> `PlacesSyncUtils.bookmarks.pullChanges`). You'll need to import
> `PlacesSyncUtils.jsm`, and maybe `PlacesUtils.jsm`, in
> https://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/components/places/tests/
> browser/head.js#1-6.
> 
> * Remove `_ensureMobileQuery`
> (https://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/services/sync/modules/engines/
> bookmarks.js#1049-1093).
> 
> * `mach test browser/components/places toolkit/components/places
> services/sync` should run all the tests we care about.
> 
> Does that make sense? Thanks for taking this on!
Yes it does. Unfortunately, Ryan seems to be taking a while in getting back to us, and I'm not sure what timezone he's in, otherwise I'd have pinged him in IRC and asked myself. If it's not too much trouble, do you mind doing that on my behalf? Alternatively, let me know what timezone he's in and I'll try to catch him and help fill out the needinfo on his behalf. Cheers!

Comment 44

[Thom Chiovoloni [:tcsc] (ex-moco)]

This is being sorted out in bug 1397551. Clearing my needinfo, which was clarify what the comment in question meant.

I don't know if this bug is strictly a duplicate, but from the perspective of my needinfo it is.

Comment 45

[Mark Hammond [:markh] [:mhammond]]

This bug is getting confusing and we are still landing other related changes around this, so let's just kill it.