1217295 - Add pref to remove "Sign in to Sync" from the menu

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Arthur Edelstein [:arthur]]

In Tor Browser, a patch was added to hide the "Sign in to Sync" item from the hamburger menu, That's because we're not entirely confident that Sync preserves the user's privacy, so it's best not to encourage the user to use it.

Here is the original Tor Browser ticket: https://trac.torproject.org/16488

And here is a link that tracks the latest version of the patch: https://torpat.ch/16488. 

We'd like to propose upstreaming this patch to Firefox -- hiding of Sync would be disabled by default.

Comment 1

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1217295-Pref-allows-hiding-Sign-in-to-Sync-from-.patch

Here's the Tor Browser patched rebased to the latest version of mozilla-central.

Comment 2

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

Comment on attachment 8677702 [details] [diff] [review]
0001-Bug-1217295-Pref-allows-hiding-Sign-in-to-Sync-from-.patch

Review of attachment 8677702 [details] [diff] [review]:
-----------------------------------------------------------------

FTR, I don't have a problem with this, although the preference should probably reflect it is the FxA state you want to hide and not just Sync - currently that's roughly the same thing, but eventually it may not be. IOW, it sounds like you are really trying to disable the entire concept of being "logged in to the browser" rather than *just* sync?

Please put up a new patch with the pref identity.fxaccounts.ui.hidden as the pref (no need to wrap it in the MOZ_SERVICES_SYNC #ifdef) and flag me for review.

Requesting feedback from ckarlof for the unlikely case that he has objections...

Comment 3

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1217295-Pref-allows-hiding-Sign-in-to-Sync-from-.patch

Here's a new version, rebased again to mozilla-central, and now using the "identity.fxaccounts.ui.hidden" pref as requested.

Comment 4

[Chris Karlof [:ckarlof]]

Is the goal to "not encourage the user to use Sync, but we'll let them if they really want to"?

FWIW, there are other Sync entry points, like in Tools menu and about:preferences#sync.

There are other FxA connected services available in the browser as well, like Hello and Pocket, you may want to look at as well.

As written, the patch is small, but I'm concerned about introducing code paths and complexity that we ourselves don't rely on. If it's not thoroughly documented why it's there, it may appear "unused" and removed in the future. Likewise, someone creating future Sync entry points may not guard them in this way.

Arthur, can you help me understand the value of upstreaming this patch? Do you know of any other forkers of Firefox this would be useful for?

Comment 5

[Chris Karlof [:ckarlof]]

Comment on attachment 8679179 [details] [diff] [review]
0001-Bug-1217295-Pref-allows-hiding-Sign-in-to-Sync-from-.patch

Review of attachment 8679179 [details] [diff] [review]:
-----------------------------------------------------------------

See Comment 4.

Comment 6

[Arthur Edelstein [:arthur]]

(In reply to Chris Karlof [:ckarlof] from comment #4)
> Is the goal to "not encourage the user to use Sync, but we'll let them if
> they really want to"?
> 
> FWIW, there are other Sync entry points, like in Tools menu and
> about:preferences#sync.
> 
> There are other FxA connected services available in the browser as well,
> like Hello and Pocket, you may want to look at as well.
> 
> As written, the patch is small, but I'm concerned about introducing code
> paths and complexity that we ourselves don't rely on. If it's not thoroughly
> documented why it's there, it may appear "unused" and removed in the future.
> Likewise, someone creating future Sync entry points may not guard them in
> this way.
> 
> Arthur, can you help me understand the value of upstreaming this patch? Do
> you know of any other forkers of Firefox this would be useful for?

Thanks, Chris. I think you're making good points here and we need to consider our approach to Sync/Hello/Pocket etc further. I'm going to close this ticket for now.