Closed Bug 1251766 Opened 8 years ago Closed 8 years ago

Improve date/time handling in downloads.search()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [downloads])

Attachments

(2 files)

downloads.search()[1] accepts some time-related query parameters as iso strings and others as strings that contain a number of milliseconds since the epoch (!!).  Lets clean this up by adding support in the WebExtensions schema layer for Date objects that also accepts ISO strings or stringified milliseconds, and then use that support in search().

[1] https://developer.chrome.com/extensions/downloads#method-search
Assignee: nobody → aswan
Blocks: 1213445
Iteration: --- → 47.3 - Mar 7
Whiteboard: [downloads]
Comment on attachment 8726857 [details]
MozReview Request: Bug 1251766 - Add new Date type to webextensions schemas r?kmag

https://reviewboard.mozilla.org/r/38279/#review34743

This looks good, but we need a test for the date format in `test/xpcshell/test_ext_schemas.js`

::: toolkit/components/extensions/Schemas.jsm:255
(Diff revision 1)
> +    const PATTERN = /(\d{8}T\d{6}Z)|(\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(Z|([-+]\d{2}:\d{2}(\.\d{3})?)))?)/;

This expression needs to be anchored.

::: toolkit/components/extensions/schemas/extension_types.json:77
(Diff revision 1)
> +            "additionalProperties": { "type": "any" }

This isn't necessary. We don't check the properties of any object that has `isInstanceOf`.
Attachment #8726857 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726858 [details]
MozReview Request: Bug 1251766 - Accept more date formats for downloads.search() r?kmag

https://reviewboard.mozilla.org/r/38281/#review34745

Looks good. Thanks.

::: toolkit/components/extensions/schemas/downloads.json:314
(Diff revision 1)
> +                "choices": [

We should add a type for this (maybe "DownloadTime") and just reference it from all of these.
Attachment #8726858 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/38279/#review34743

> This isn't necessary. We don't check the properties of any object that has `isInstanceOf`.

I was hitting this exception without adding that fragment:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#544
Maybe the condition for throwing that exception should be relaxed?
https://reviewboard.mozilla.org/r/38279/#review34743

> I was hitting this exception without adding that fragment:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#544
> Maybe the condition for throwing that exception should be relaxed?

Ah, sorry, you're right.
Comment on attachment 8726857 [details]
MozReview Request: Bug 1251766 - Add new Date type to webextensions schemas r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38279/diff/1-2/
Attachment #8726857 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726858 [details]
MozReview Request: Bug 1251766 - Accept more date formats for downloads.search() r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38281/diff/1-2/
Comment on attachment 8726857 [details]
MozReview Request: Bug 1251766 - Add new Date type to webextensions schemas r?kmag

https://reviewboard.mozilla.org/r/38279/#review34787
Attachment #8726857 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d298fc97819e
https://hg.mozilla.org/mozilla-central/rev/3644fa6e882a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.