Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Device Destroy #821

Merged
merged 2 commits into from Apr 25, 2019

Conversation

Projects
None yet
4 participants
@vladikoff
Copy link
Member

commented Mar 20, 2019

Fixes #746

TODO

  • Needs FFI
  • Needs changelog entry

@vladikoff vladikoff changed the title Add Device Destroy [WIP] Add Device Destroy Mar 20, 2019

@eoger eoger force-pushed the send-tab-devices-API branch 3 times, most recently from ba6ede7 to 9c3ce26 Mar 26, 2019

@vladikoff vladikoff force-pushed the device-destroy-v2 branch 2 times, most recently from 2b6f23a to 4416074 Mar 26, 2019

@@ -148,7 +148,23 @@ fn main() -> Result<(), failure::Error> {
.unwrap();
println!("Tab sent!");
}
2 => ::std::process::exit(0),
2 => {
let devices = acct.lock().unwrap().get_devices().unwrap();

This comment has been minimized.

Copy link
@vladikoff

vladikoff Mar 26, 2019

Author Member

@eoger do we want to make this reusable between option 0 and 2?

This comment has been minimized.

Copy link
@eoger

eoger Apr 23, 2019

Contributor

If you want

@eoger eoger referenced this pull request Mar 26, 2019

Merged

Send Tab #676

1 of 1 task complete

@vladikoff vladikoff changed the title [WIP] Add Device Destroy Add Device Destroy Mar 26, 2019

@grigoryk grigoryk force-pushed the send-tab-devices-API branch from 699b093 to 8b90eda Mar 27, 2019

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 9c3ce26 to 9d0bfce Mar 27, 2019

@rfk

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@vladikoff what's the status of this one, do we intend to merge it into the send-tab branch and land from there?

@vladikoff

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

@vladikoff what's the status of this one, do we intend to merge it into the send-tab branch and land from there?

Either merge it into send-tab, or wait for send-tab to land, then land this. up to @eoger

@rfk rfk requested a review from eoger Apr 3, 2019

@rfk

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

From meeting: let's get this merged into @eoger's send-tab PR, and ask @grigoryk to hook up the "signout" call as part of the lifecycle work he's currently doing.

@thomcc
Copy link
Contributor

left a comment

In general we should only use unsafe functions in the FFI if they're doing something weird. Most of them should be written with safe code to call out when they're doing weird things (I've called out a couple of them but all of them should be fixed).

Show resolved Hide resolved components/fxa-client/ffi/src/lib.rs Outdated
Show resolved Hide resolved components/fxa-client/ffi/src/lib.rs Outdated
Show resolved Hide resolved components/fxa-client/ffi/src/lib.rs Outdated
Show resolved Hide resolved components/fxa-client/ffi/src/lib.rs Outdated
@vladikoff

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@thomcc thanks for the review notes, That code is in https://github.com/mozilla/application-services/pull/676/files , somehow we need to transfer the comments into there or something

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 5eadc02 to 47cdfee Apr 4, 2019

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 147a375 to 7f869c2 Apr 12, 2019

@rfk

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Sounds like the send-tab patch is pretty close to merging, @eoger should we go ahead and land this device-destroy code into your PR in preparation for mege?

@rfk

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

That code is in https://github.com/mozilla/application-services/pull/676/files , somehow we need to transfer the comments into there or something

Just noting for completeness that these issues appear to have been addressed in the linked PR 👍

@eoger eoger force-pushed the send-tab-devices-API branch 2 times, most recently from 38ed596 to 57823e5 Apr 18, 2019

@vladikoff vladikoff force-pushed the device-destroy-v2 branch from 4416074 to ab80b60 Apr 23, 2019

@vladikoff vladikoff changed the base branch from send-tab-devices-API to master Apr 23, 2019

@vladikoff vladikoff force-pushed the device-destroy-v2 branch from ab80b60 to 4665285 Apr 23, 2019

@vladikoff vladikoff force-pushed the device-destroy-v2 branch 2 times, most recently from 6e96ee6 to f95195b Apr 23, 2019

@vladikoff

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@eoger rebased this on top of master, could we land it before we cut a release with send tab so @grigoryk could add device/destroy as part of sendtab work in a-c?

@vladikoff vladikoff force-pushed the device-destroy-v2 branch from f95195b to 8505880 Apr 23, 2019

@eoger

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Sorry for some reason I thought it had been merged, let's merge it yeah, please add a changelog entry also!

@eoger

eoger approved these changes Apr 23, 2019

Copy link
Contributor

left a comment

Thanks!

@@ -148,7 +148,23 @@ fn main() -> Result<(), failure::Error> {
.unwrap();
println!("Tab sent!");
}
2 => ::std::process::exit(0),
2 => {
let devices = acct.lock().unwrap().get_devices().unwrap();

This comment has been minimized.

Copy link
@eoger

eoger Apr 23, 2019

Contributor

If you want

@vladikoff vladikoff force-pushed the device-destroy-v2 branch from 8505880 to 535a210 Apr 23, 2019

@vladikoff vladikoff force-pushed the device-destroy-v2 branch from 535a210 to 5662b54 Apr 23, 2019

@vladikoff vladikoff merged commit 61a9264 into master Apr 25, 2019

9 of 11 checks passed

Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
ci/circleci: iOS build and test CircleCI is running your tests
Details
ci/circleci: Check Rust dependencies Your tests passed on CircleCI!
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Check Swift formatting Your tests passed on CircleCI!
Details
ci/circleci: Deploy website Your tests passed on CircleCI!
Details
ci/circleci: Lint Rust with clippy Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@vladikoff vladikoff deleted the device-destroy-v2 branch Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.