Closed
Bug 735868
Opened 12 years ago
Closed 10 years ago
Stop tracking last selected timestamp in tabs engine
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: zpao, Assigned: ttaubert)
References
Details
(Whiteboard: [sync:tabs])
Attachments
(2 files, 1 obsolete file)
3.28 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
8.23 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Hurrah for less code!
Component: Firefox Sync: UI → Firefox Sync: Backend
OS: Mac OS X → All
QA Contact: sync-ui → sync-backend
Hardware: x86 → All
Comment 3•11 years ago
|
||
Taking a dependency on Bug 747338, which presumably will store this value..
Depends on: 747338
Whiteboard: [sync:tabs]
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8372022 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8372022 -
Attachment is obsolete: true
Attachment #8372052 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8372053 -
Flags: review?(rnewman)
Comment 8•10 years ago
|
||
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?
Attachment #8372053 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9edc0e18ac8e https://hg.mozilla.org/integration/fx-team/rev/12523b5bff5d
Whiteboard: [sync:tabs] → [sync:tabs][fixed-in-fx-team]
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9edc0e18ac8e https://hg.mozilla.org/mozilla-central/rev/12523b5bff5d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sync:tabs][fixed-in-fx-team] → [sync:tabs]
Target Milestone: --- → mozilla30
Updated•6 years ago
|
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in
before you can comment on or make changes to this bug.
Description
•