Closed Bug 1320986 Opened 8 years ago Closed 8 years ago

Implement browsingData.settings WebExtensions API method

Categories

(WebExtensions :: Compatibility, defect, P2)

Product:
WebExtensions
Component:
Compatibility
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Iteration:
53.3 - Dec 26
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [browsingData]triaged)

Attachments

(1 file)

Bug 1320986 - Implement browsingData.settings WebExtensions API method,
58 bytes, text/x-review-board-request
aswan
: review+
Details 
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
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 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 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 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.
Try looks good. Requesting check in.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/409240e746c8
Status: NEW → RESOLVED
Closed: 8 years 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.
Flags: needinfo?(bob.silverberg)
Keywords: dev-doc-needed
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)
(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!
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: