1362994 - Implement browsingData.settings WebExtension API method on android

Status: RESOLVED FIXED

(WebExtensions :: Android, enhancement, P5)

Description

[Tushar Saini (:shatur)]

This method returns the user's preferences for clearing browsing data. The doc can be found at [1].

[1]. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/settings

Comment 1

[Tushar Saini (:shatur)]

Attachment: Bug 1362994 - Add support for HashMap prefs in SharedPreferences

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

Comment 2

[Tushar Saini (:shatur)]

Hey Sebastian!

This patch, for now does not contain tests. I have some questions as well :

1]. On android firefox, do we provide a functionality to clear history with respect to time? I was not able to find one and that's why for now I have returned value of since to "Everything".

2]. Also, I found out that android clear data settings and gecko preferences (like privacy.item) are both different things. Changing one does not reflect on other. So which preferences we want to return? (Gecko one or android one) For now I am returning gecko preferences. [In contrast to desktop firefox, changing preferences in about:config reflects in settings as well!)

Thanks!!

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll add mattw from the web extensions team as second reviewer!

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145620

::: mobile/android/components/extensions/ext-browsingData.js:34
(Diff revision 1)
> +
> +          for (let item of PREF_LIST) {
> +            // The property formData needs a different case than the
> +            // formdata preference.
> +            const name = item === "formdata" ? "formData" : item;
> +            dataToRemove[name] = Preferences.get(`${PREF_DOMAIN}${item}`);

I haven't tried this patch yet - are those the same preferences that we use in Settings -> Clear private data?

::: mobile/android/components/extensions/schemas/browsing_data.json:155
(Diff revision 1)
> +            ]
> +          }
> +        ]
> +      },
> +      {
> +        "name": "remove",

Do we import the full schema even though we only implement one method in this patch? (mattw might be able to answer that)

::: mobile/android/components/extensions/schemas/jar.mn:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  chrome.jar:
> +	content/schemas/browsing_data.json

nit: the indentation seems to be different in this line

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Tushar Saini (:shatur) from comment #2)
> 1]. On android firefox, do we provide a functionality to clear history with
> respect to time? I was not able to find one and that's why for now I have
> returned value of since to "Everything".

No that I know of. This is something we would need to add.

> 2]. Also, I found out that android clear data settings and gecko preferences
> (like privacy.item) are both different things. Changing one does not reflect
> on other. So which preferences we want to return? (Gecko one or android one)
> For now I am returning gecko preferences. [In contrast to desktop firefox,
> changing preferences in about:config reflects in settings as well!)

I guess the one Android uses (in the settings UI). While about:config is functional in Fennec we do not officially support tinkering with it. I assume Chrome also reflects the changes in the UI here and not something that is somewhere in chrome://flags/ - After all those gecko prefs might go away while we still support removing the data in the UI - I guess this current patch uses the Gecko preferences?

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145628

Thanks for taking this on! I'd like to see a test written for this before I'll be ready to r+.

::: mobile/android/components/extensions/ext-browsingData.js:17
(Diff revision 1)
> +this.browsingData = class extends ExtensionAPI {
> +  getAPI(context) {
> +    return {
> +      browsingData: {
> +        settings() {
> +          const PREF_DOMAIN = "privacy.item.";

Instead of copying over this implementation from `browser`, could you create a shared module (something like ExtensionBrowsingData.jsm) in `toolkit` to put this code in so both mobile and desktop can share it?

::: mobile/android/components/extensions/schemas/browsing_data.json:1
(Diff revision 1)
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.

Instead of creating the schema as a new file, please copy it over from `browser` using `hg cp browser/components/extensions/schemas/browsing_data.json ` in order to minimize your diff and make it easier to review.

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145620

> Do we import the full schema even though we only implement one method in this patch? (mattw might be able to answer that)

No, we don't import functions or types which are marked as unsupported. It will be easier to see what has been marked as unsupported once the schema file is copied over from `browser` and then modified.

Comment 8

[Matthew Wein [:mattw]]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145628

> Instead of copying over this implementation from `browser`, could you create a shared module (something like ExtensionBrowsingData.jsm) in `toolkit` to put this code in so both mobile and desktop can share it?

I'm also okay with creating a separate bug for looking into creating a shared module for both the mobile and desktop implementations to use.

Comment 9

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145620

> I haven't tried this patch yet - are those the same preferences that we use in Settings -> Clear private data?

No, for now I have been using Gecko preferences. I will change it to Settings UI preferences.

Comment 10

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review145628

> I'm also okay with creating a separate bug for looking into creating a shared module for both the mobile and desktop implementations to use.

I also think it should be done in seprate bug. I will create shared module after the implementation of all methods of this API. That way we will be able incorporate all other methods share code as well. But If you think this approach will be problamatic, then I can do this here as well.

Comment 11

[Tushar Saini (:shatur)]

Attachment: Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

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

Comment 12

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

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

Comment 13

[Tushar Saini (:shatur)]

Hey,

For now please overlook tests, its not complete. I will update test in upcoming patch.

I have tried to resolve all the above issues, please have a look and let me know If we are moving in right direction.

Thanks!

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/139774/#review148260

LGTM!

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review148264

From my side this looks good. Can you push this to try?

::: mobile/android/components/extensions/ext-browsingData.js:25
(Diff revision 1)
> +          // We do not provide option to delete history by time
> +          // so, since value is given 0, which means Everything
> +          let since = 0;

This is something we are going to implement when adding the other APIs, right? Can you file a bug for adding support for this here too (and mark it as "depends" on the other bugs?).

Comment 16

[Tushar Saini (:shatur)]

Hey!

> This is something we are going to implement when adding the other APIs,
> right? Can you file a bug for adding support for this here too (and mark it
> as "depends" on the other bugs?).

Settings returns the selected user preferences, currently we do not have time preference in fennec. To return this preference here, we are required to add this preference in UI. We will be adding support to remove private data with respect to time, so, it makes sense to add this functionality later on in UI as well.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, yeah, sorry, I think I confused this with the web extensions API for removing by time.

Comment 18

[Tushar Saini (:shatur)]

Hi!

I found a problem, until we modify our preferences once, our setting method was not able to get preferences correctly. So I trace back the problem to here [1] & [2]. The problem was, in first case (when preferences are imported first time) we were saving it using forApp method and in second case (when preferences are changed by user) we were saving it, using forProfile. I have updated [1] one to add it to profile. This will reflect in next patch:) [Am I right to do so?]

Also, how can I import SharedPreferences.jsm module in my mochitest?


[1]. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/MultiPrefMultiChoicePreference.java#57

[2]. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/MultiChoicePreference.java#209

Regards

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

This looks fine to me, but I don't think this commit needs a second reviewer

Comment 20

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

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

Comment 21

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

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

Comment 22

[Tushar Saini (:shatur)]

Hey Matthew,

I have updated the patch with test. Please have a look whenever you get time and let me know If something needs modifying. :)

Thanks.

Comment 23

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

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

Comment 24

[Tushar Saini (:shatur)]

Cancelling ni flag, review flag is enough.

Comment 25

[Matthew Wein [:mattw]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review159166

This is close - I mostly have nits but one of the tests needs to be fixed before I'll feel comfortable r+'ing.

::: mobile/android/base/java/org/mozilla/gecko/preferences/MultiPrefMultiChoicePreference.java:57
(Diff revision 3)
>      protected synchronized void loadPersistedValues() {
>          // This will load the new pref if it exists.
>          super.loadPersistedValues();
>  
>          // First check if we've already done the import the old data. If so, nothing to load.
> -        final SharedPreferences prefs = GeckoSharedPrefs.forApp(getContext());
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(getContext());

Please move java changes to a separate commit.

::: mobile/android/components/extensions/ext-browsingData.js:20
(Diff revision 3)
> +      browsingData: {
> +        settings() {
> +          const PREF_DOMAIN = "android.not_a_preference.privacy.clear";
> +          const PREF_KEY_PREFIX = "private.data.";
> +          // The following prefs are the only ones in Firefox that match corresponding
> +          // values used by Chrome when rerturning settings.

nit: rerturning -> returning

::: mobile/android/components/extensions/ext-browsingData.js:23
(Diff revision 3)
> +          let dataToRemove = {};
> +          let dataRemovalPermitted = {};

nit: Please declare these right before they are used below.

::: mobile/android/components/extensions/ext-browsingData.js:28
(Diff revision 3)
> +          let since = 0;
> +          let dataTrue = SharedPreferences.forProfile().getSetPref(PREF_DOMAIN);
> +          let options = {since};

nit: "since" is an optional parameter, and it already defaults to 0 if absent, so I think we should just leave it out of the options we resolve with for now.

::: mobile/android/components/extensions/ext-browsingData.js:40
(Diff revision 3)
> +              case "cookies_sessions":
> +                name = "cookies";
> +                break;
> +              case "downloadFiles":
> +                name = "downloads";
> +                break;

Out of curiosity, why are these property names changed on Android but not on Desktop?

::: mobile/android/components/extensions/ext-browsingData.js:49
(Diff revision 3)
> +                name = "downloads";
> +                break;
> +              default:
> +                name = item;
> +            }
> +            dataToRemove[name] = dataTrue.indexOf(`${PREF_KEY_PREFIX}${item}`) > -1;

nit: how about `dataTrue.includes(...)`?

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:25
(Diff revision 3)
> +const PREF_DOMAIN = "android.not_a_preference.privacy.clear";
> +const PREF_KEY_PREFIX = "private.data.";
> +const SETTINGS_LIST = ["cache", "cookies", "history", "formData", "downloads"];
> +
> +function checkPrefs(key, dataToRemove, prefs, PREF_SUFFIX) {
> +  let PREF_VALUE = prefs.indexOf(`${PREF_KEY_PREFIX}${PREF_SUFFIX}`) > -1;

nit: please either make this a const or name it `prefValue`.

Also, how about `prefs.includes(...)`?

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:29
(Diff revision 3)
> +function checkPrefs(key, dataToRemove, prefs, PREF_SUFFIX) {
> +  let PREF_VALUE = prefs.indexOf(`${PREF_KEY_PREFIX}${PREF_SUFFIX}`) > -1;
> +  is(dataToRemove, PREF_VALUE, `${key} property of dataToRemove matches the expected pref.`);
> +}
> +
> +function testSettingsPrefrences(dataToRemove){

nit: Prefrences -> Preferences

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:49
(Diff revision 3)
> +        checkPrefs(key, dataToRemove[key], prefs, key);
> +    }
> +  }
> +}
> +
> +add_task(function* testSettings() {

nit: please use async functions now that we support them.

e.g.,

add_task(async function testSettings() {
  async function background() {
    browser.test.onMessage.addListener(async function(msg) {
      let settings = await browser.browsingData.settings();
      browser.test.sendMessage("settings", settings);
    });
  }
  ...
  await extension.startup();
});

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:67
(Diff revision 3)
> +    },
> +  });
> +
> +  yield extension.startup();
> +
> +  extension.sendMessage("settings");

nit: please rename this to "retrieve-settings" and check for this message in the onMessage listener before returning the settings.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:68
(Diff revision 3)
> +  let settings = yield extension.awaitMessage("settings");
> +  let dataToRemove = settings.dataToRemove;
> +  let dataRemovalPermitted = settings.dataRemovalPermitted;

nit: let {dataToRemove, dataRemovalPermitted} = await extension.awaitMessage("settings");

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:73
(Diff revision 3)
> +  for (let key in SETTINGS_LIST) {
> +    ok(key in Object.keys(dataToRemove), `dataToRemove contains expected property.`);
> +    ok(key in Object.keys(dataRemovalPermitted), `dataRemovalPermitted contains expected property.`);
> +  }

I don't think this is doing what you think it is. In this loop, `key` is actually the index in the `SETTINGS_LIST` array (e.g. 0,1,2,3,...), not the key.

I think you'll want something like:

is(SETTINGS_LIST.length, Object.keys(dataToRemove).length, ...);
is(SETTINGS_LIST.length, Object.keys(dataRemovalPermitted).length, ...);
for (let key of SETTINGS_LIST) {
  ok(key in dataToRemove, ...);
  ok(key in dataRemovalPermitted, ...);
}

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:96
(Diff revision 3)
> +  // Explicitly set some prefs to true
> +  const NEW_PREFS = ["private.data.cache", "private.data.cookies_sessions"];
> +
> +  SharedPreferences.forProfile().setSetPref(PREF_DOMAIN, NEW_PREFS);
> +
> +  extension.sendMessage("settings");

nit: "settings" -> "retrieve-settings"

Comment 26

[Matthew Wein [:mattw]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review159170

Comment 27

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139774/diff/3-4/

Comment 28

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143910/diff/3-4/

Comment 29

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review159166

> nit: "since" is an optional parameter, and it already defaults to 0 if absent, so I think we should just leave it out of the options we resolve with for now.

I don't understand what we want to do here, are you suggesting to remove since key from options object. I think our user will expect this to be present. Currently we remove 'everything' so, I think it will be suitable to return 0. What's your opinion?

> Out of curiosity, why are these property names changed on Android but not on Desktop?

All properties in our desktop implementation matches chrome properties names, except formData, which we do change in our desktop implementation. On the other hand on android all properties name are different!

Comment 30

[Matthew Wein [:mattw]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review160876

Thanks! I've added Shane as a reviewer to give a final look before landing.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:24
(Diff revision 4)
> +
> +const PREF_DOMAIN = "android.not_a_preference.privacy.clear";
> +const PREF_KEY_PREFIX = "private.data.";
> +const SETTINGS_LIST = ["cache", "cookies", "history", "formData", "downloads"];
> +
> +async function checkPrefs(key, dataToRemove, prefs, PREF_SUFFIX) {

nit: please only use all-caps for constants: PREF_SUFFIX -> prefSuffix.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:54
(Diff revision 4)
> +        browser.browsingData.settings().then(settings => {
> +          browser.test.sendMessage("settings", settings);
> +        });

let settings = await browser.browsingData.settings();
browser.test.sendMessage("settings", settings);

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

With r+ from both mwein and bsilverberg on this I'll be happy.

Comment 32

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143910/diff/4-5/

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Restoring review request

Comment 34

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review161238

Nice work Tushar! Just a few small comments that could improve the code.

::: mobile/android/components/extensions/ext-browsingData.js:25
(Diff revision 5)
> +          let since = 0;
> +          let options = {since};

You could shorten this to `let options = {since: 0};` or even just use `{since: 0}` in place of `options` at line 56.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:24
(Diff revision 5)
> +
> +const PREF_DOMAIN = "android.not_a_preference.privacy.clear";
> +const PREF_KEY_PREFIX = "private.data.";
> +const SETTINGS_LIST = ["cache", "cookies", "history", "formData", "downloads"];
> +
> +async function checkPrefs(key, dataToRemove, prefs, prefSuffix) {

The argument name `dataToRemove` is a bit misleading here. Throughout the code that usually indicates an object with a number of properties, but in the context of this function it's just the value of one of those properties. Maybe something like `value` or `actualValue` would be better.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:51
(Diff revision 5)
> +  }
> +}
> +
> +add_task(async function testSettings() {
> +  function background() {
> +    browser.test.assertTrue("browsingData" in browser, "Namespace 'browsingData' exists in browser");

This isn't really necessary because the call to `browser.browsingData.settings()` at line 54 would fail if the namespace does not exist.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:83
(Diff revision 5)
> +  for (let key of Object.keys(dataRemovalPermitted)) {
> +    is(true, dataRemovalPermitted[key],
> +      `${key} property of dataRemovalPermitted matches the expected pref.`);
> +  }

You could combine this into the check at line 77, where you are checking that `dataRemovalPermitted` contains all the expected keys. Instead of just checking that the key exists, you could check that its value is `true`.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:93
(Diff revision 5)
> +  // Verify object options returned as expected.
> +  // For now, We do not provide option to delete history by time, so,
> +  // since value is given 0, which means Everything.
> +  is(options.since, 0, `options contains expected value.`);
> +
> +  // Explicitly set some prefs to true

Are you only doing a test with some prefs explicitly set to `true` because the default is assumed to be all are `false`? It might be a good idea to also explcitly set some to `false` in case that assumption is incorrect, or the defaults change at some point in the future.

Comment 35

[Tushar Saini (:shatur)]

Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139774/diff/4-5/

Comment 36

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143910/diff/5-6/

Comment 37

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review161238

Thanks for the review!

> Are you only doing a test with some prefs explicitly set to `true` because the default is assumed to be all are `false`? It might be a good idea to also explcitly set some to `false` in case that assumption is incorrect, or the defaults change at some point in the future.

By default, all prefs are set to be 'true'. So, what we are doing is setting cache and cookies_session to true explicitly and others will be setted to false (as they are not present in the sharedPrefs set) and then verify our changes in testSettingsPreferences.

Comment 38

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review164692

This looks good, Tushar, thanks. Just some minor issues with the test.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:24
(Diff revision 6)
> +
> +const PREF_DOMAIN = "android.not_a_preference.privacy.clear";
> +const PREF_KEY_PREFIX = "private.data.";
> +const SETTINGS_LIST = ["cache", "cookies", "history", "formData", "downloads"];
> +
> +async function checkPrefs(key, actualValue, prefs, prefSuffix) {

It doesn't seem like this needs to be `async`.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:29
(Diff revision 6)
> +async function checkPrefs(key, actualValue, prefs, prefSuffix) {
> +  let prefValue = prefs.includes(`${PREF_KEY_PREFIX}${prefSuffix}`);
> +  is(actualValue, prefValue, `${key} property of dataToRemove matches the expected pref.`);
> +}
> +
> +async function testSettingsPreferences(dataToRemove){

As above, if `checkPrefs` doesn't need to be `async`, then neither does this.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html:76
(Diff revision 6)
> +  // Verify that we get the keys we expect.
> +  is(SETTINGS_LIST.length, Object.keys(dataToRemove).length, `dataToRemove contains expected no of keys`);
> +  is(SETTINGS_LIST.length, Object.keys(dataRemovalPermitted).length, `dataRemovalPermitted contains expected no of keys`);
> +  for (let key of SETTINGS_LIST) {
> +    ok(key in dataToRemove, `dataToRemove contains expected property.`);
> +    ok(key in dataRemovalPermitted, `dataRemovalPermitted contains expected property.`);

This isn't neccesary because the next test will fail if the key is not present.

Comment 39

[Tushar Saini (:shatur)]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143910/diff/6-7/

Comment 40

[Tushar Saini (:shatur)]

Attachment: Bug 1362994 - Add support for HashMap prefs in SharedPreferences

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

Comment 41

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review164752

Looks good. Nice work, Tushar.

::: mobile/android/components/extensions/test/mochitest/test_ext_browsingData_settings.html
(Diff revisions 6 - 7)
>  
>    // Verify that we get the keys we expect.
>    is(SETTINGS_LIST.length, Object.keys(dataToRemove).length, `dataToRemove contains expected no of keys`);
>    is(SETTINGS_LIST.length, Object.keys(dataRemovalPermitted).length, `dataRemovalPermitted contains expected no of keys`);
>    for (let key of SETTINGS_LIST) {
> -    ok(key in dataToRemove, `dataToRemove contains expected property.`);

Did you mean to remove this line too? I guess this is also tested in `testSettingsPreferences(dataToRemove)` below?

Comment 42

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8888418 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/159360/#review165106

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d589dfabec7
Add support for HashMap prefs in SharedPreferences r=sebastian
https://hg.mozilla.org/integration/autoland/rev/ea938674f102
Implement browsingData.settings WebExtension API method on android. r=bsilverberg,mattw,sebastian

Comment 44

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

https://hg.mozilla.org/mozilla-central/rev/6d589dfabec7
https://hg.mozilla.org/mozilla-central/rev/ea938674f102

Comment 45

[Will Bamberg [:wbamberg]]

Updated compat data: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/settings#Browser_compatibility