735868 - Stop tracking last selected timestamp in tabs engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

At some point in the future session restore will just track that itself, so the extra event listening and work done to save it back into session restore won't be necessary.

Comment 1

[Richard Newman [:rnewman]]

Hurrah for less code!

Comment 2

[cheater]

How does this play with 747338, which is needed for 675539?

Comment 3

[Richard Newman [:rnewman]]

Taking a dependency on Bug 747338, which presumably will store this value..

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-735868-Stop-tracking-last-selected-timestamp-in-.patch

There's some tests that will need to be updated as well. What do we actually use the .lastUsed property for? Is it only to sort tabs when creating a record? Should we store the property at all then?

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8372022 [details] [diff] [review]
0001-Bug-735868-Stop-tracking-last-selected-timestamp-in-.patch

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

> There's some tests that will need to be updated as well. What do we actually
> use the .lastUsed property for? Is it only to sort tabs when creating a
> record? Should we store the property at all then?

The value is used for sorting tabs when displayed on other devices.

But in any case, it's part of the spec object format, so we can't strip it out.

r+ with test updates.

::: services/sync/modules/engines/tabs.js
@@ +128,5 @@
>          allTabs.push({
>            title: entry.title || "",
>            urlHistory: [entry.url],
>            icon: tab.attributes && tab.attributes.image || "",
> +          lastUsed: Math.floor(tab.lastAccessed / 1000)

|| 0;

-- just in case lastAccessed is something silly like undefined or null.

@@ +316,2 @@
>      // Only increase the score by whole numbers, so use random for partial score
> +    // For regular Tab events, do a full score bump.

Worth expanding this comment a little:

// For page shows, bump the score 10% of the time, emulating a partial score. We don't want to sync too frequently.
// For all other page events, always bump the score.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-735868-Stop-tracking-last-selected-timestamp-in-.patch

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-735868-Fix-tests.patch

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8372053 [details] [diff] [review]
0002-Bug-735868-Fix-tests.patch

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

Ideally also add a test that we're still adding the field, and it has the expected value. If that's a huge pain in the ass, just manually verify.

::: services/sync/tests/unit/test_tab_store.js
@@ +56,5 @@
>              }],
>              attributes: {
>                image: "image"
>              },
> +            lastAccessed: 1000

Make this something more tricksy, like 1499?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Richard Newman [:rnewman] from comment #8)
> Ideally also add a test that we're still adding the field, and it has the
> expected value. If that's a huge pain in the ass, just manually verify.

test_getAllTabs() does that, which tests the value you referenced below:

  do_check_eq(tabs[0].icon, "image");
  do_check_eq(tabs[0].lastUsed, 1);

> ::: services/sync/tests/unit/test_tab_store.js
> @@ +56,5 @@
> >              }],
> >              attributes: {
> >                image: "image"
> >              },
> > +            lastAccessed: 1000
> 
> Make this something more tricksy, like 1499?

Done.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/9edc0e18ac8e
https://hg.mozilla.org/integration/fx-team/rev/12523b5bff5d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9edc0e18ac8e
https://hg.mozilla.org/mozilla-central/rev/12523b5bff5d