This is similar to Bug 1031273. We should clear Synced Tabs if the user requests to Clear Private Data. Sync will, of course, put them back eventually; but this allows a savvy user to remove stale tab records (after disconnecting Sync, for example). (See also Bug 1145896, which should address part of that issue.) I think this can be a mentor bug, although I'm not quite sure how it's done. Co-mentoring it is!

I'd like to take this bug, does anyone has any tips on where to get started?

(In reply to Matt King from comment #1) > I'd like to take this bug, does anyone has any tips on where to get started? I have some tips, but as I say, I don't /really/ know. 1) Start by adding a new syncedTabs item to the Sanitizer in [1]. 2) The Synced Tabs are managed totally in Java, so you'll need to send a message-and-response (see [2]) to Java to do the actual clearing work. 3) Now, clearing the tabs is not particularly hard. You'll need to use the Android ContentProvider interface against the TabsProvider (see [3]); for an example, you can look at the code at [4]. 4) tests? Not sure what is here already, we'll cross this bridge later. 5) profit! Let me know how you go! vivek and rnewman can help with bits of this too. [1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm [2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/Messaging.jsm [3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/TabsProvider.java [4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/FennecTabsRepository.java#364

Adds option to clear synced tabs in the privacy preferences area.

Comment on attachment 8600867 [details] [diff] [review] Add "Synced Tabs" to Clear Private Data option list I think I got this in correctly. I found the method `deleteNonLocalClientsAndTabs` in FennecTabsRepository, which looks like it does what we want. I wasn't sure whether or not we had to clear the synced tabs in the background, but I followed how handleClearHistory works. Would like some pointers on how to add tests for this.

Comment on attachment 8600867 [details] [diff] [review] Add "Synced Tabs" to Clear Private Data option list Review of attachment 8600867 [details] [diff] [review]: ----------------------------------------------------------------- This commit is solid! I have one substantive request; see comments. I'd really like to see a meaningful test here, but https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testClearPrivateData.java doesn't really do anything meaningful. So let's add STR to test both situations (no account, option is disabled), (an account, option is enabled, and does the right thing). r+ since I'll land this and see the next version. Do you have try access? If so, I'd love to see a Robocop-only try push. Bravo! ::: mobile/android/base/tests/testSettingsMenuItems.java @@ +102,5 @@ > { mStringHelper.COOKIES_LABEL, "Enabled", "Enabled, excluding 3rd party", "Disabled" }, > { mStringHelper.REMEMBER_PASSWORDS_LABEL }, > MANAGE_LOGINS_ARR, > { mStringHelper.MASTER_PASSWORD_LABEL }, > + { mStringHelper.CLEAR_PRIVATE_DATA_LABEL, "", "Browsing history", "Search history", "Downloads", "Form history", "Cookies & active logins", "Saved passwords", "Cache", "dOffline website data", "Site settings", "Clear data" }, This looks like a mistake -- "dOffline..." Do we need to add "Synced tabs" here? ::: mobile/android/modules/Sanitizer.jsm @@ +270,5 @@ > + > + syncedTabs: { > + clear: function () > + { > + return Messaging.sendRequestForResult({ type: "Sanitize:ClearSyncedTabs" }) nit: indentation looks off by one space here. @@ +276,5 @@ > + }, > + > + get canClear() > + { > + return true; Hey, can we make this do the right thing depending on whether the user has an Account (Sync or Firefox)? Use a callback (there are examples in the file) and the API at https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Accounts.jsm @@ +282,2 @@ > } > + nit: kill this newline.

Okay, submitted updated patch. ::: mobile/android/base/tests/testSettingsMenuItems.java This was not an intentional update, I'm not sure how it got here? > I'd really like to see a meaningful test here, but https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testClearPrivateData.java doesn't really do anything meaningful. So let's add STR to test both situations (no account, option is disabled), (an account, option is enabled, and does the right thing). I'm not sure what you mean by 'add STR to test both situations'. More than willing to add tests if I can get clarification! I do have try access, I'll do a try push and post it here.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba9a967d804

(In reply to Matt King from comment #7) > Okay, submitted updated patch. > > ::: mobile/android/base/tests/testSettingsMenuItems.java > > This was not an intentional update, I'm not sure how it got here? > > > I'd really like to see a meaningful test here, but https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/testClearPrivateData.java doesn't really do anything meaningful. So let's add STR to test both situations (no account, option is disabled), (an account, option is enabled, and does the right thing). > > I'm not sure what you mean by 'add STR to test both situations'. More than > willing to add tests if I can get clarification! STR = steps to reproduce. > I do have try access, I'll do a try push and post it here. It's looking green \o/ Of course, testClearPrivateData is disabled everywhere, I think, so we're not really seeing anything in particular. Anyway: 1) post an updated patch; 2) post testing instructions (both cases, with and without an Account) for our QA team; 3) profit! I will keep my eyes open for a good next ticket for you. Unless you already have one?

Steps to reproduce: A. Without a sync account 1. Visit Settings -> Privacy -> Clear now 2. Make sure "Synced tabs" is present and checked 3. Tap "Clear data" 4. No errors should occur B. With a sync account 1. Set up a Firefox sync account 2. Sign in on two browsers (e.g. one desktop, and on Fennec) with the same account 3. Open up tabs on desktop 4. On Fennec, go to Settings -> Sync and tap "Sync now" 5. Go to home screen on Fennec, and swipe to "Synced tabs" to verify tabs are present from the desktop 6. Go to Settings -> Privacy -> Clear now 7. Make sure "Synced tabs" is present and checked 8. Tap "Clear data" 9. Visit "Synced tabs" again to verify that tabs are removed

Is this what you were looking for Nick? Let me know if there's anything else you need. I don't have another ticket immediately in mind, if you have any ideas please let me know!

(In reply to Matt King from comment #11) > Is this what you were looking for Nick? Let me know if there's anything else > you need. > > I don't have another ticket immediately in mind, if you have any ideas > please let me know! Hi Matt, I didn't realize there was a fresh patch up -- make sure to r? when action is needed. Your patch is great but there's a historical weirdness at play. We have two databases tracking "Sync clients" and you have cleared one of them, the one that shows up in Synced tabs. There's another one that shows up in "Send to other devices". Long term we want to get rid of the latter one, which is tracked by Bug 1159020, and just use the Synced tabs DB (which includes clients). I think for right now we'll be literal and say "Synced tabs" is just tabs, like you've done. We could add "Other devices" to wipe those later if we want. This is all made trickier because the data will be replaced by Sync, possibly immediately :)

(In reply to Matt King from comment #11) > I don't have another ticket immediately in mind, if you have any ideas > please let me know! I NIed you on some easy tickets. Bug 1159020 is harder but more valuable. mcomella and I could help flesh out directions for that ticket if you're interested.

Hey Matt! Sorry this took so long to land. Was this your first patch landed? Looks like it. Bravo!

Comment on attachment 8602000 [details] [diff] [review] Add "Synced Tabs" to Clear Private Data option list Review of attachment 8602000 [details] [diff] [review]: ----------------------------------------------------------------- Landed!

Tested with: Device: Samsung S5 (Android 4.4.4) Build: Firefox for Android 41.0a1 (2015-05-27) "Synced Tabs" option is present in Settings -> Privacy -> Clear private data -> "Clear now" and "Clear on exit" 1."Clear now" sub menu: based on steps from comment 10 - Without a sync account: works OK, "Private data cleared" message appears. - With a sync account: "Synced tabs" are removed from about:home after choosing clear now -> synced tabs. After opening a new tab on desktop, choosing "Sync now" on mobile, "Synced tabs" will appear again in about:home 2. "Clear on exit" sub menu: I've filled Bug 1168775 - Synced tabs are not cleared from "Synced Tabs" panel when "Clear on exit" is used