1104867 - Hook up the Send To Extension to the Sync clients system

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Hook up the Send To Extension to the Client Provider. This includes implementing the REST call in the Client Provider that pushes the share page to the cloud.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Pull Request

Patch that makes the Send To extension push the shared item to your sync data.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

This needs to be be revisted because I don't think this code works or is up to date with what we actually need to do.

Comment 3

[Richard Newman [:rnewman]]

I'm ripping out some of the old code now, and will get to this after Bug 1141845.

Comment 4

[Richard Newman [:rnewman]]

Revised summary:

This bug involves:

* Listing remote clients, fetching if necessary.
* Queuing a command for that remote client. We queue so that an immediate failure to send doesn't result in data loss. We also queue so that if our client set is blanked -- as it might be if your password changes or something else odd happens -- the 'wipe' event doesn't lose the tabs you're trying to send.
* Uploading the remote client record correctly.

Comment 5

[Emily Toop (:fluffyemily)]

Attachment: WIP Feedback Request

Comment 6

[Emily Toop (:fluffyemily)]

Attachment: WIP Feedback Request

So, I've updated some of the code (including a complete DB rewrite). It's still a bit messy, but I'm conscious that you're off on PTO from tomorrow and I still haven't figured out how to do the next bit yet (actually sending the data to the clients)!

Also, there is a test deleteAllCommands that I can't get to pass and I don't know why. For some reason all the commands are still in the DB after I've deleted them and all the responses are successes.....

Comment 7

[Emily Toop (:fluffyemily)]

Comment on attachment 8628965 [details]
WIP Feedback Request

St3fan: Can you please give me feedback on this code? See previous comments

Comment 8

[Richard Newman [:rnewman]]

I'll try to take a tropical look, too.

Comment 9

[Richard Newman [:rnewman]]

Comments:

Split sdweb change out. Less confusion on backout.
Trailing comma in 'queries' list is good style; turns a two line change into one line on addition.
CommandSyncTable: fix header indenting, remove // on first line.
Let TableClient -> TableClients, note that it is defined elsewhere.

Comment 10

[Richard Newman [:rnewman]]

Part of me wants the send logic to hide in RemoteClient, requiring the send extension to grab a client to send to, but 0fg for now. Looking at the sending bit shortly.

Comment 11

[Richard Newman [:rnewman]]

The downside of having a not null reference to the clients table is that we can't refer to a client that is ttled or deleted without careful attention in the clients engine. Also, wiping and redownloading clients will either fail with a constraint error or discard commands (or rely on transaction quirks and immediate insertion, which we don't do).

That probably needs to weaken.

Comment 12

[Richard Newman [:rnewman]]

SQL stuff is super low level. We should kill a lot of lines in there. 

Next bit: when clients syncs, after download, check if we have outstanding commands. If none, we are done. 

Otherwise, for each destination client, construct a client record with the outgoing command. Ideally we'd preserve the entire downloaded record payload, but that's harder here. 

Upload that record with a PUT, specifying IfUnmodifiedSince with the record's last seen modified time. See how we upload ours elsewhere in ClientsSynchronizer. The put might fail with a consistency error (409? 412?), in which case we need to download and re upload. If you choose, you can implement this always as a download-then-upload. 

On a successful upload, delete the uploaded row from the commands table.

Comment 13

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 14

[Emily Toop (:fluffyemily)]

rnewman: more updates.

Comment 15

[Emily Toop (:fluffyemily)]

simplified db. 
Removed unused methods. 
Returned RemoteClient with commands from getCommands() therefore not requiring 2 separate queries in syncronizer

Comment 16

[Richard Newman [:rnewman]]

I'll get to this first thing on Monday, Emily.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 8631532 [details] [review]
Pull request

Not quite there yet; two bugs with the same root cause. Back to you, Emily!

Comment 18

[Emily Toop (:fluffyemily)]

Attachment: Pull request

here we go again