User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170904220027 Steps to reproduce: set browser.bookmarks.showMobileBookmarks to false in about:config, then updated firefox Actual results: browser.bookmarks.showMobileBookmarks reset to true Expected results: config remains unchanged after update always reproducible (every day, using nightly)

From what I can tell, this is intentional - the pref is set to true when using Firefox Sync, after syncing has finished to ensure the synced mobile bookmarks are displayed. The value stored in prefs is there to avoid a slightly more expensive lookup when displaying menus, it is not there as a configuration option. Therefore I think this is invalid as it isn't meant to be a configuration option.

I think this is a mistake. It should be a configuration option. > the pref is set to true when using Firefox Sync It shouldn't be if I'm not using sync on mobile. It shouldn't clutter up the interface with features that I don't use. > to avoid a slightly more expensive lookup when displaying menus I don't know your code, but I'd imagine this should be as simple as `if (Config.useMobileBookmarks())`. Is the performance impact enough to justify cluttering up the interface with options that many users would consider useless?

(In reply to rasq37 from comment #2) > > the pref is set to true when using Firefox Sync > > It shouldn't be if I'm not using sync on mobile. It shouldn't clutter up the > interface with features that I don't use. This seems to be the real issue... > > to avoid a slightly more expensive lookup when displaying menus > > I don't know your code, but I'd imagine this should be as simple as `if > (Config.useMobileBookmarks())`. Is the performance impact enough to justify > cluttering up the interface with options that many users would consider > useless? The performance isn't necessarily an issue, the more configuration options we have the more complexity there is in the code base that we then have to support. However, I think it is reasonable that we don't display the bookmarks menu if we're not syncing with bookmarks, so I'll re-open for that and re-title it.

(In reply to Mark Banner (:standard8) from comment #3) > (In reply to rasq37 from comment #2) > > > the pref is set to true when using Firefox Sync > > > > It shouldn't be if I'm not using sync on mobile. It shouldn't clutter up the > > interface with features that I don't use. > > This seems to be the real issue... In theory this should already happen - the code which sets this up is at http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/toolkit/components/places/PlacesSyncUtils.jsm#1024 and the comments say "If there are no mobile bookmarks, the query will not be created, or will be removed if it already exists." - so I guess that isn't quite working as expected (and therefore I'm making this a normal bug rather than an enhancement)

Removing the ni? from Ryan as I believe the intent is clear - only create the mobile query if the mobile folder has entries and remove it otherwise.

Sorry Ed, but as you saw from IRC, I was mistaken about the intent here - I need to punt this back to Ryan. Ryan, currently all Sync users see a "Mobile" folder in the bookmark UI even when there are no bookmarks in that folder. The user is unable to remove that folder. Some users are finding this frustrating - they have no intention of syncing from a mobile device and find the empty and non-removable folder pointless and a distraction. I'm sympathetic to their concerns. How do you feel about it? Should we only show that folder when there are mobile bookmarks, or should we leave things the way I describe above?

Comment on attachment 8926975 [details] Bug 1397551 - Delete Mobile Bookmarks query when there are no mobile bookmarks. https://reviewboard.mozilla.org/r/198206/#review203580 Sorry Ed :( Let's see what rfeeley says...

Thanks for asking! I don't have any strong feelings, but some observations: 1. If this is a special blessed folder of value to few, it had better be worthy of the prime real estate it's taking up for everyone 2. If this is a special blessed folder, it should have a special icon 3. If this is a folder for mobile bookmarks, and the user doesn't have sync set up, we should probably do what we do in the Page Action Menu and say "None connected — Connect Another Device…" I don't imagine the feature warrants all the work described above, so I'm all for hiding it when the user is not syncing a mobile device (or better, has no mobile bookmarks even if mobile devices are connected).

I believe that since we're not showing anything helpful like a call to action button, we should go on and hide that menu since it rubs some users the wrong way. If you feel the same way Mark, feel free to resume your review on my patch!

Comment on attachment 8926975 [details] Bug 1397551 - Delete Mobile Bookmarks query when there are no mobile bookmarks. https://reviewboard.mozilla.org/r/198206/#review205688 Thanks! ::: toolkit/components/places/PlacesSyncUtils.jsm:1119 (Diff revision 1) > + let maybeMobileQueryGuids = await fetchGuidsWithAnno(db, > + ORGANIZER_QUERY_ANNO, ORGANIZER_MOBILE_QUERY_ANNO_VALUE); > + if (!maybeMobileQueryGuids.length) { > + return; > + } > + let mobileQueryGuid = maybeMobileQueryGuids[0]; while it seems impossible, maybe we should warn if .length != 1?

Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/94001a39d9b6 Delete Mobile Bookmarks query when there are no mobile bookmarks. r=markh