1251766 - Improve date/time handling in downloads.search()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

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

Comment 1

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1251766 - Add new Date type to webextensions schemas r?kmag

Review commit: https://reviewboard.mozilla.org/r/38279/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38279/

Comment 2

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1251766 - Accept more date formats for downloads.search() r?kmag

Review commit: https://reviewboard.mozilla.org/r/38281/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38281/

Comment 3

[Kris Maglione [:kmag]]

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`.

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Andrew Swan [:aswan]]

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?

Comment 6

[Kris Maglione [:kmag]]

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 7

[Andrew Swan [:aswan]]

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/

Comment 8

[Andrew Swan [:aswan]]

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 9

[Kris Maglione [:kmag]]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d298fc97819e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3644fa6e882a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d298fc97819e
https://hg.mozilla.org/mozilla-central/rev/3644fa6e882a