Closed
Bug 1362994
Opened 8 years ago
Closed 7 years ago
Implement browsingData.settings WebExtension API method on android
Categories
(WebExtensions :: Android, enhancement, P5)
WebExtensions
Android
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: shatur, Assigned: shatur, Mentored)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [triaged])
Attachments
(2 files, 1 obsolete file)
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•8 years ago
|
Blocks: gsoc2017-android-webext
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Priority: -- → P5
Whiteboard: [triaged]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years 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!!
Updated•8 years ago
|
Assignee: nobody → tushar.saini1285
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 3•8 years ago
|
||
I'll add mattw from the web extensions team as second reviewer!
Updated•8 years ago
|
Attachment #8868188 -
Flags: review?(mwein)
Comment 4•8 years 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
Comment 5•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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.
Updated•8 years ago
|
Attachment #8868188 -
Flags: review?(s.kaspari)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•8 years 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•8 years 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•8 years 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•8 years 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.
Comment 17•8 years ago
|
||
Oh, yeah, sorry, I think I confused this with the web extensions API for removing by time.
Assignee | ||
Comment 18•8 years 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•8 years ago
|
Flags: needinfo?(s.kaspari)
Flags: needinfo?(mwein)
Comment 19•8 years ago
|
||
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•8 years ago
|
Attachment #8868188 -
Flags: review?(mwein)
Assignee | ||
Comment 22•8 years 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) |
Comment 25•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8872405 -
Flags: review?(mixedpuppy)
Comment 30•7 years 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 31•7 years ago
|
||
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 33•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8868188 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 41•7 years 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•7 years ago
|
Attachment #8888418 -
Flags: review?(s.kaspari)
Comment 42•7 years 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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d589dfabec7
https://hg.mozilla.org/mozilla-central/rev/ea938674f102
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Keywords: dev-doc-needed
Comment 45•7 years ago
|
||
Updated compat data: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/settings#Browser_compatibility
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•