Implement browsingData.settings WebExtension API method on android

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Android
P5
normal
RESOLVED FIXED
3 months ago
2 days ago

People

(Reporter: shatur, Assigned: shatur, Mentored)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 months ago
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
(Assignee)

Updated

3 months ago
Blocks: 1351269
(Assignee)

Updated

3 months ago
Blocks: 1362118
No longer blocks: 1351269

Updated

3 months ago
Priority: -- → P5
Whiteboard: [triaged]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
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!!
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I'll add mattw from the web extensions team as second reviewer!
Attachment #8868188 - Flags: review?(mwein)

Comment 4

3 months ago
mozreview-review
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
(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

3 months ago
mozreview-review
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.
Attachment #8868188 - Flags: review?(mwein) → review-

Comment 7

3 months ago
mozreview-review-reply
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

3 months ago
mozreview-review-reply
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.
(Assignee)

Comment 9

3 months ago
mozreview-review-reply
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.
(Assignee)

Comment 10

3 months ago
mozreview-review-reply
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.
Attachment #8868188 - Flags: review?(s.kaspari)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

3 months ago
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

3 months ago
mozreview-review
Comment on attachment 8868188 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

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

LGTM!
Attachment #8868188 - Flags: review?(s.kaspari) → review+

Comment 15

3 months ago
mozreview-review
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?).
Attachment #8872405 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 16

3 months ago
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.
Oh, yeah, sorry, I think I confused this with the web extensions API for removing by time.
(Assignee)

Comment 18

2 months ago
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
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mwein)
(Assignee)

Updated

2 months ago
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mwein)
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
Attachment #8868188 - Flags: review?(mwein)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8868188 - Flags: review?(mwein)
(Assignee)

Comment 22

2 months ago
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.
Flags: needinfo?(mwein)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 months ago
Cancelling ni flag, review flag is enough.
Flags: needinfo?(mwein)

Comment 25

2 months ago
mozreview-review
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

2 months ago
mozreview-review
Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

https://reviewboard.mozilla.org/r/143910/#review159170
Attachment #8872405 - Flags: review?(mwein)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

a month ago
mozreview-review-reply
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!

Updated

a month ago
Attachment #8872405 - Flags: review?(mixedpuppy)

Comment 30

a month ago
mozreview-review
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);
Attachment #8872405 - Flags: review?(mwein) → review+
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.
Attachment #8872405 - Flags: review?(mixedpuppy) → review?(bob.silverberg)
Comment hidden (mozreview-request)
Comment on attachment 8872405 [details]
Bug 1362994 - Implement browsingData.settings WebExtension API method on android.

Restoring review request
Attachment #8872405 - Flags: review?(bob.silverberg)

Comment 34

a month ago
mozreview-review
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.
Attachment #8872405 - Flags: review?(bob.silverberg) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 37

a month ago
mozreview-review-reply
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

29 days ago
mozreview-review
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.
Attachment #8872405 - Flags: review?(bob.silverberg) → review-
Comment hidden (mozreview-request)
(Assignee)

Updated

29 days ago
Attachment #8868188 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 41

29 days ago
mozreview-review
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?
Attachment #8872405 - Flags: review?(bob.silverberg) → review+
(Assignee)

Updated

29 days ago
Attachment #8888418 - Flags: review?(s.kaspari)

Comment 42

29 days ago
mozreview-review
Comment on attachment 8888418 [details]
Bug 1362994 - Add support for HashMap prefs in SharedPreferences

https://reviewboard.mozilla.org/r/159360/#review165106
Attachment #8888418 - Flags: review?(s.kaspari) → review+

Comment 43

29 days ago
Pushed by s.kaspari@gmail.com:
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

28 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d589dfabec7
https://hg.mozilla.org/mozilla-central/rev/ea938674f102
Status: ASSIGNED → RESOLVED
Last Resolved: 28 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.