1587228 - Remove support for "old" sendtab clients.

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

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

All our mobile browsers support "new" send tab and supporting "old" sendtab adds complexity leading to bugs like bug 1587227.

Comment 1

[Edouard Oger [:eoger]]

ni? Ana to tell us when we should pull the trigger (I assume after we migrate Fennec users to Fenix).

Comment 2

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

(In reply to Edouard Oger [:eoger] from comment #1)

ni? Ana to tell us when we should pull the trigger (I assume after we migrate Fennec users to Fenix).

Oops, my bad, I thought Fennec already moved to the new system. I agree we can't do this for some time, so I'll take the ni? off Ana, then add it back in a few months :)

Comment 3

[Vlad Filippov (:vladikoff)]

After discussing this with Ana, the rough timeline for this is around September 2020 based on current Fennec/Fenix schedule. However that can change at any point.

Comment 4

[Ryan Kelly [:rfkelly]]

ni? myself to provide a bit more context on how to actually do this

Comment 5

[Ryan Kelly [:rfkelly]]

The place to start for this bug is in the sendTabToDevice function, which is what gets invoked from browser UI in Desktop when sending a tab. If you step through the code in that function, you'll see that it first tries to send the tab via FxA device commands, and if that doesn't work then it sends via Sync.

The smallest fix for this issue is to just delete the second part where it falls back to Sync.

If you enjoy deleting code, one could also track down the code for Weave.Service.clientsEngine.sendURIToClientForDisplay and fully delete it from there as well, since I don't believe it will ever be used again for anything else.

Comment 6

[Tarik Eshaq [:teshaq]]

Attachment: Bug 1587228 - Removes support for old send-tab clients. r=rfkelly,markh

• Removes the fallback to the old sent-tab
• Deletes the api to send URIs using commands
• Modifies tests that depended on that api to now use
  other commands, namely the wipeEngine command

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a18936814a7
Removes support for old send-tab clients. r=markh

Comment 8

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 0a18936814a7 (bug 1587228) for causing bc failures in browser_contextmenu_sendpage.js.
https://hg.mozilla.org/integration/autoland/rev/9685511fb522359561bfc10d17c53132919db393

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=0a18936814a7a5c97c0cacaeba5c9f15c51386ec&selectedTaskRun=B8MTdK21QyiRDM2vm4XEow.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=343522998&repo=autoland&lineNumber=2571

Comment 9

[Ryan Kelly [:rfkelly]]

Oof, sorry Tarik, looks like some test bustage here. We'll need to see if we can repro those test failures locally, then make sure we get a clean run on the try servers before attempting to land this again.

Comment 10

[Ryan Kelly [:rfkelly]]

We'll need to see if we can repro those test failures locally

FYI, something like ./mach test ./browser/base/content/test/sync/ should be able to run just the subset of tests of interest, for faster debugging.

Comment 11

[Tarik Eshaq [:teshaq]]

Apologies!!! That was my bad - I should have looked more carefully at other tests. I wasn't aware of those tests.

For context here's what went wrong:

The tests in browser/base/content/test/sync/ include test cases that test old send tab clients. The tests expect that those clients show up in popups and such. With my patch, we remove support for old clients, so those clients will not show up in the popups.

I'll fix the tests right away.

Comment 12

[Tarik Eshaq [:teshaq]]

https://treeherder.mozilla.org/jobs?revision=3b16659cf078a640747ebdea93dfd5f620e05d37&repo=try

There are a few failures, but they all seem unrelated to send-tab and sync, there's the browser/base/content/test/tabcrashed/browser_aboutRestartRequired_buildid.js that is also unrelated, but I don't see an issue filed for it. I can repro the failure with a clean build though, so it's unrelated to the change

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5738d1a06fe6
Removes support for old send-tab clients. r=markh

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5738d1a06fe6