Don't de-duplicate tabs from multiple devices

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P2
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: markh, Assigned: tiago, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
We currently de-dupe tabs open on multiple devices - ie, if the same tab is open on 2 devices, we only show it for one of those devices.

While this saves space, I think it is misguided - eg, I might know the tab is open on one of my devices, but have forgotten it is also open on another. However, if I look at the device where I know it is open, I may not find it there, causing confusion.

The user in bug 1347078 expressed exactly that confusion.

Ryan, how do you feel about this?

The de-duping is at https://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/services/sync/modules/SyncedTabs.jsm#112-149
Flags: needinfo?(rfeeley)
Good idea! I'm all for showing what's open.
Flags: needinfo?(rfeeley)

Updated

6 months ago
Mentor: eoger@fastmail.com
Keywords: good-first-bug
Priority: -- → P2
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months ago
Hi!

I attached a patch for this bug. Running locally with the changes, I was able to see the duplicated tabs on different devices. I also ran the services/sync tests and the xpcshell-test of services/sync, all of them passing.

Should I write a test for this bug? If yes, what type of test do you recommend?

Thanks!
Flags: needinfo?(markh)
Flags: needinfo?(eoger)

Comment 4

5 months ago
mozreview-review
Comment on attachment 8866626 [details]
Bug 1355663 - Don't de-duplicate tabs from multiple devices.

https://reviewboard.mozilla.org/r/138230/#review141640

Good patch Santiago!

As for tests, a unit test will probably suffice.
You can add a test case in services/sync/tests/unit/test_syncedtabs.js, test_filter is a good example that you can copy and modify to fit your needs. configureClients is the meat of the test.
Your test should fail without the fix and succeed with it.

Thanks!

::: commit-message-d8762:1
(Diff revision 1)
> +Bug 1355663 - De-duplicate tabs from multiple devices. r?markh, eoger

Don't de-duplicate*

::: commit-message-d8762:1
(Diff revision 1)
> +Bug 1355663 - De-duplicate tabs from multiple devices. r?markh, eoger

This is a fairly simple patch, no need to add more reviews on Mark's plate :)

::: services/sync/modules/SyncedTabs.jsm:125
(Diff revision 1)
>        log.debug("Processing client", clientRepr);
>  
>        for (let tab of client.tabs) {
>          let url = tab.urlHistory[0];
>          log.debug("remote tab", url);
>          // Note there are some issues with tracking "seen" tabs, including:

You should probably kill the whole comment
Attachment #8866626 - Flags: review?(eoger)

Updated

5 months ago
Flags: needinfo?(eoger)
Attachment #8866626 - Flags: review?(markh)

Updated

5 months ago
Assignee: nobody → tiago.paez11
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 6

5 months ago
mozreview-review
Comment on attachment 8866626 [details]
Bug 1355663 - Don't de-duplicate tabs from multiple devices.

https://reviewboard.mozilla.org/r/138230/#review142574

Great patch, I can confirm the fix works and the test is correct.
Attachment #8866626 - Flags: review?(eoger) → review+

Comment 7

5 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6572599cafa
Don't de-duplicate tabs from multiple devices. r=eoger

Comment 8

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6572599cafa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Reporter)

Updated

5 months ago
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.