Implement browsingData.settings WebExtensions API method

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
WebExtensions: Compatibility
P2
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

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

unspecified
mozilla53
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [browsingData]triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

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

[1] https://developer.chrome.com/extensions/browsingData#method-settings
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
Some implementation notes:

This method returns an object with three keys, `options`, `dataToRemove` and `dataRemovalPermitted` but they do not all necessarily make sense in the context of Firefox. Here are some notes about them:

options: We will return a `since` property which is based on the user's value for the `privacy.sanitize.timeSpan` property. We will not return an `originTypes` property as that does not have meaning in Firefox's context.

dataToRemove: We will return an object that only contains those properties which correspond to Firefox preferences. The values will be based on the user's prefs.

dataRemovalPermitted: As this doesn't have meaning to Firefox, we will return `true` for each properties that is also contained in the `dataToRemove` object.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

a year ago
mozreview-review
Comment on attachment 8815297 [details]
Bug 1320986 - Implement browsingData.settings WebExtensions API method,

https://reviewboard.mozilla.org/r/96290/#review97236

Some little things but looks good.

::: browser/components/extensions/ext-browsingData.js:9
(Diff revision 6)
> +                                  "resource:///modules/Sanitizer.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +
> +const PREF_DOMAIN = "privacy.cpd.";
> +const PREF_LIST = ["cache", "cookies", "history", "formdata", "downloads"];

These prefs are just about the settings() api, could you just move them down there rather than having them here globally?  (or give them names that indicate what they apply to)
Also a comment explaining why these are the only things that are pertinent for settings()

::: browser/components/extensions/ext-browsingData.js:11
(Diff revision 6)
> +                                  "resource://gre/modules/Services.jsm");
> +
> +const PREF_DOMAIN = "privacy.cpd.";
> +const PREF_LIST = ["cache", "cookies", "history", "formdata", "downloads"];
> +
> +function getBoolPref(branch, name) {

You could just use Preferences.jsm for this I think

::: browser/components/extensions/schemas/browsing_data.json:31
(Diff revision 6)
> +        "id": "RemovalOptions",
> +        "type": "object",
> +        "description": "Options that determine exactly what data will be removed.",
> +        "properties": {
> +          "since": {
> +            "type": "number",

Can we use extensionTypes.Date here

::: browser/components/extensions/schemas/browsing_data.json:60
(Diff revision 6)
> +            }
> +          }
> +        }
> +      },
> +      {
> +        "id": "DataTypeSet",

i know you already went over this with kris but we don't support some of these (eq websql, probably others?)  i don't think they should be in the schema at all if we can't/won't ever support them

::: browser/components/extensions/test/xpcshell/test_ext_browsingData_settings.js:37
(Diff revision 6)
> +
> +  let since = Sanitizer.getClearRange()[0] / 1000;
> +
> +  // Because it is based on the current timestamp, we cannot know the exact
> +  // value to expect for since, so allow a 1s variance.
> +  ok(since - 500 < settings.options < since + 500,

This seems almost certain to become an intermittent failure.  I would bump up the slop quite a bit (10 seconds?)  I think this would also be expressed more cleanly as `Math.abs(settings.options - since) < threshold`

::: browser/components/extensions/test/xpcshell/test_ext_browsingData_settings.js:42
(Diff revision 6)
> +  ok(since - 500 < settings.options < since + 500,
> +     "settings.options contains the expected since value.");
> +
> +  let dataTypeSet = settings.dataToRemove;
> +  for (let key of Object.keys(dataTypeSet)) {
> +    let pref = (key == "formData") ? "formdata" : key;

what's the point of this line?

::: browser/components/extensions/test/xpcshell/test_ext_browsingData_settings.js:68
(Diff revision 6)
> +  extension.sendMessage("settings");
> +  settings = yield extension.awaitMessage("settings");
> +
> +  equal(settings.dataToRemove[SINGLE_PREF], false, "Preference that was set to false returns false.");
> +
> +  do_register_cleanup(() => {

if we're going to do this, do it up where the pref is first set so it happens if something in the middle throws an exception
Attachment #8815297 - Flags: review?(aswan) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8815297 [details]
Bug 1320986 - Implement browsingData.settings WebExtensions API method,

https://reviewboard.mozilla.org/r/96290/#review97252

::: browser/components/extensions/ext-browsingData.js:24
(Diff revision 6)
> +}
> +
> +extensions.registerSchemaAPI("browsingData", "addon_parent", context => {
> +  return {
> +    browsingData: {
> +      settings: function() {

This can just be `settings() { ...`
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
mozreview-review-reply
Comment on attachment 8815297 [details]
Bug 1320986 - Implement browsingData.settings WebExtensions API method,

https://reviewboard.mozilla.org/r/96290/#review97236

> i know you already went over this with kris but we don't support some of these (eq websql, probably others?)  i don't think they should be in the schema at all if we can't/won't ever support them

Yes, that makes sense, and it also removes the need to verify that they are not being used by the caller, which I was doing in the patch for bug 1321303.

> what's the point of this line?

The property in the objects returned by settings is called `formData`, but the pref I need to retrieve from Prefs is `formdata`. All of the other ones match, so this line is just to deal with that difference. I suppose I could just use `toLowerCase()` on the property name and that would work in every case and allow me to remove this somewhat confusing line of code. I'll do that.
(Assignee)

Comment 12

a year ago
Try looks good. Requesting check in.
Keywords: checkin-needed

Comment 13

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/409240e746c8
Implement browsingData.settings WebExtensions API method, r=aswan
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/409240e746c8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
docs page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/settings. Please let me know if it looks correct.

A couple of things:

* with Dev Edition (53.0a2) this function doesn't work. I get this error:

    Sanitizer.getClearRange(...) is null      ext-browsingData.js:190

* with Nightly (54.0a1) it works, but the dataToRemove object has the following properties:

  Object {
    cache: false, 
    cookies: false, 
    history: true, 
    formdata: false, 
    downloads: true, 
    formData: false 
  }

I would have expected it would have a property for each "type" (i.e. including passwords and pluginData). It's also strange that "formData" appears twice, with different capitalization.

Updated

10 months ago
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed
(Assignee)

Comment 16

10 months ago
Thanks for that feedback, Will. I checked out the docs and they look good. 

Regarding the other issues you raised, I have yet to look into the first one (the Dev Edition issue), but I did verify that the second one (formdata/formData) is a bug. I opened a bug for it and already have a patch ready to attach to that bug. As to which properties are included, what you are seeing is expected. Our dialog does not contain settings for passwords or pluginData, so it doesn't make sense for us to include properties for those.

Would you mind opening a bug for the Dev Edition issue?
Flags: needinfo?(bob.silverberg) → needinfo?(wbamberg)
Thanks for the quick review, Bob.

(In reply to Bob Silverberg [:bsilverberg] from comment #16)

> As to which properties are included, what you
> are seeing is expected. Our dialog does not contain settings for passwords
> or pluginData, so it doesn't make sense for us to include properties for
> those.

The docs currently say "All data types will be present in this object." So perhaps they should say something like: "This object will contain a property for every data type that can be toggled through the browser's UI"?

> Would you mind opening a bug for the Dev Edition issue?

-> https://bugzilla.mozilla.org/show_bug.cgi?id=1339881
Flags: needinfo?(wbamberg)

Updated

10 months ago
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 18

10 months ago
(In reply to Will Bamberg [:wbamberg] from comment #17)
> Thanks for the quick review, Bob.
> 
> (In reply to Bob Silverberg [:bsilverberg] from comment #16)
> 
> > As to which properties are included, what you
> > are seeing is expected. Our dialog does not contain settings for passwords
> > or pluginData, so it doesn't make sense for us to include properties for
> > those.
> 
> The docs currently say "All data types will be present in this object." So
> perhaps they should say something like: "This object will contain a property
> for every data type that can be toggled through the browser's UI"?

Yes, that sounds better.

> 
> > Would you mind opening a bug for the Dev Edition issue?
> 
> -> https://bugzilla.mozilla.org/show_bug.cgi?id=1339881

Thanks!
You need to log in before you can comment on or make changes to this bug.