1427850 - Sync looks up username in the prefs for every input change in the location bar from PlacesRemoteTabsAutocompleteProvider

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[:Gijs (he/him)]

Not sure if bug 1426480 will make this moot, and also not sure exactly how expensive this is in practice, but while investigating bug 1426135 I noticed that typing in the URL bar also hits the `getMatches` implementation in the remote tabs autocomplete provider, which in turn pokes the pref service to see if the user has configured sync.

It seems like this jsm should, on first call, initialize state, and then observe state as propagated through weave/sync components, rather than doing manual lookups to the pref service to see if sync is configured.

Comment 1

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

There's an easier fix for this, we later check if sync is ready, and will return an empty list if it is not yet ready. We can just check that instead. Sync won't ever be considered ready for non-sync users.

I'll just do this since that was the hard part.

Comment 2

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

Attachment: Bug 1427850 - Check that sync is ready directly instead of reading the sync username to determine it

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

Comment 3

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

Comment on attachment 8939634 [details]
Bug 1427850 - Check that sync is ready directly instead of reading the sync username to determine it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209936/diff/1-2/

Comment 4

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

> Sync won't ever be considered ready for non-sync users

This is untrue. I'm not sure if it would matter though, but to be safe, I've made it look at the `enabled` property on the xpcom service as well, and changed that service to use a lazy preference getter instead (should be useful for bug 1426627 as well).

FWIW bug 1426480 would be unlikely to make this (or this sort of issue) moot, since I think we'd still consider the username to be a preference, as opposed to part of sync internal state.

Comment 5

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

Comment on attachment 8939634 [details]
Bug 1427850 - Check that sync is ready directly instead of reading the sync username to determine it

https://reviewboard.mozilla.org/r/209936/#review215568

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcec2e929c2c
Check that sync is ready directly instead of reading the sync username to determine it r=markh

Comment 7

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/fcec2e929c2c