1355663 - Don't de-duplicate tabs from multiple devices

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

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

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

Comment 1

[Ryan Feeley [:rfeeley]]

Good idea! I'm all for showing what's open.

Comment 2

[Santiago Paez [:tiago]]

Attachment: Bug 1355663 - Don't de-duplicate tabs from multiple devices.

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

Comment 3

[Santiago Paez [:tiago]]

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!

Comment 4

[Edouard Oger [:eoger]]

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

Comment 5

[Santiago Paez [:tiago]]

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

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

Comment 6

[Edouard Oger [:eoger]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6572599cafa
Don't de-duplicate tabs from multiple devices. r=eoger

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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