1225690 - Add a SyncedTabs module to abstract away Sync internals.

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Attachment: 0003-Bug-XXXXXXX-implement-a-SyncedTabs-module-to-abstrac.patch

The new UI we are building around Synced Tabs could do with a helper module to abstract away the Sync internals. This module isn't particularly clever - it does no sorting or caching and returns simple data structures consumers can transform into what they desire.

This module also doesn't handle CloudSync - while it potentially could, I've no idea how to configure or test CloundSync. For this reason, about:sync-tabs has *not* been updated to use that module. Bug 1201331 is also removing all places where about:sync-tabs is exposed in the UI, so in effect, this means about:sync-tabs will end up as legacy UI just for CloudSync users.

Richard, it probably makes sense for you to defer the review to Zach, who wrote the initial version of this and needs to use it in bug 1210586 - your call!

Comment 1

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

Attachment: 0003-Bug-1225690-implement-a-SyncedTabs-module-to-abstrac.patch

This version doesn't attempt to sync tabs when Sync isn't configured to avoid "login error" type log spew.

Comment 2

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

Attachment: 0002-Bug-1225690-implement-a-SyncedTabs-module-to-abstrac.patch

This patch adds a new haveSyncedThisSession getter to the synced-tabs module, with the tabs engine tracking a flag for it.

The intention is that consumers may want to treat "zero clients" differently, depending on whether we have ever synced in this session - if we haven't we show a "fetching" UI, but if we have, we show a promo to get moar clients! The Synced Tabs UI was trying to deduce this, but it was unreliable in edge-cases, so having a canonical flag for that makes sense to me.

Comment 3

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

Hey Richard, this is blocking the synced tabs menu that we hope to land on 45 - do you think you will find time to look at this over the next few days or so?

Comment 4

[Richard Newman [:rnewman]]

Sorry, my dog died on Wednesday, which threw a little bit of a wrench in my plans. I'll take a look now.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8691228 [details] [diff] [review]
0002-Bug-1225690-implement-a-SyncedTabs-module-to-abstrac.patch

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

r+ with concerns addressed. Thanks for being patient!

::: services/sync/modules/SyncedTabs.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";

Nit: newline before makes this stand out more.

@@ +25,5 @@
> +// A topic we fire whenever we have new tabs available. This might be due
> +// to a request made by this module to refresh the tab list, or as the result
> +// of a regularly scheduled sync. The intent is that consumers just listen
> +// for this notification and update their UI in response.
> +const NEWTABS_TOPIC = "services.sync.tabs.update";

.update_d_? It's fired after they arrive, right?

Should this be .changed instead?

@@ +60,5 @@
> +    }
> +    return {
> +      type:  "tab",
> +      title: tab.title || url,
> +      url,

This is where some of our abstractions and naming start to fall down, no?

Synced tab records have a URL history here. Within aboutSyncTabs.js, where we constructed a list of things to display, flattening was fine.

Here, where you're providing an accessor to synced tabs, I think it's misleading to call this 'makeTab' -- this representation is very coupled to the current needs of something like about:sync-tabs, not the data representation.

My suggested fix is to include the whole urlHistory array here.

@@ +80,5 @@
> +  }),
> +
> +  _tabMatchesFilter(tab, filter) {
> +    return tab.url.toLowerCase().includes(filter) ||
> +           tab.title.toLowerCase().includes(filter);

I don't like the use of toLowerCase here. Not only is it not that cheap, it's also going to be wrong because it's blind to locales -- if that's a Turkish title, bad things will happen.

Depending on where the filter comes from, you might be able to match this instead with RegExp(filter, "i").

@@ +106,5 @@
> +
> +      for (let tab of client.tabs) {
> +        let url = tab.urlHistory[0];
> +        log.debug("remote tab", url);
> +        if (!url || seenURLs.has(url)) {

Observation: this doesn't maximize timestamp if a URL is open more than once. Perhaps it should? Do we care?

@@ +130,5 @@
> +    if (!force) {
> +      // Don't bother refetching tabs if we already did so recently
> +      let lastFetch = 0;
> +      try {
> +        lastFetch = Services.prefs.getIntPref("services.sync.lastTabFetch");

If you use the Services prefs helper, you don't need to handle exceptions when the pref is missing.

@@ +132,5 @@
> +      let lastFetch = 0;
> +      try {
> +        lastFetch = Services.prefs.getIntPref("services.sync.lastTabFetch");
> +      }
> +      catch (e) {

Nit: cuddle.

@@ +137,5 @@
> +        /* Just use the default value of 0 */
> +      }
> +
> +      let now = Math.floor(Date.now() / 1000);
> +      if (now - lastFetch < 30) {

Pull that out as a const or pref?

::: services/sync/modules/engines/tabs.js
@@ +46,5 @@
> +  // A flag to indicate if we have synced in this session. This is to help
> +  // consumers of remote tabs that may want to differentiate between "I've an
> +  // empty tab list as I haven't yet synced" vs "I've an empty tab list
> +  // as there really are no tabs"
> +  haveSyncedThisSession: false,

hasSyncedThisSession. TabEngine is singular:

  if (engine.hasSyncedThisSession) {
      …

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4cbc740f137d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cbc740f137d