Closed Bug 1397551 Opened 3 years ago Closed 2 years ago

Don't show the Mobile Bookmarks folders when Sync is active but not syncing with a mobile

Categories

(Firefox :: Sync, defect, P1)

57 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: u601862, Assigned: eoger)

References

Details

Attachments

(1 file)

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)
Component: Untriaged → Bookmarks & History
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.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
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.
Severity: normal → enhancement
Status: RESOLVED → REOPENED
Component: Bookmarks & History → Sync
Ever confirmed: true
Resolution: INVALID → ---
Summary: browser.bookmarks.showMobileBookmarks reset on update → Don't show the Mobile Bookmarks folders when Sync is active but not syncing with a mobile
Status: REOPENED → NEW
(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)
Severity: enhancement → normal
Flags: needinfo?(eoger)
Flags: needinfo?(eoger) → needinfo?(rfeeley)
See Also: → 1407970
Duplicate of this bug: 1410875
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.
Flags: needinfo?(rfeeley)
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
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?
Flags: needinfo?(rfeeley)
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...
Attachment #8926975 - Flags: review?(markh)
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).
Flags: needinfo?(rfeeley)
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!
Attachment #8926975 - Flags: review?(markh)
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?
Attachment #8926975 - Flags: review?(markh) → review+
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
https://hg.mozilla.org/mozilla-central/rev/94001a39d9b6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
See Also: → 1548109
You need to log in before you can comment on or make changes to this bug.