Remove HistoryTime and use Date type instead

RESOLVED FIXED in Firefox 51

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla51

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [history] triaged)

Attachments

(1 attachment, 1 obsolete attachment)

We currently have `DownloadTime` [1] and will soon [2] have `HistoryTime`, which is exactly the same, but in history.json. Andrew suggested we move these out of the specific schema files into a shared type, which I guess would go into extension_types.json.

Is there anything else we need to discuss about this before proceeding?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/downloads.json#213
[2] https://reviewboard.mozilla.org/r/52277/diff/1#1
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/extension_types.json
Assignee

Updated

3 years ago
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)

Comment 1

3 years ago
do at the schema level now and add as records type.  likely to need in a lot of places and limited amount of work.  bob will need info kris for advice
Assignee: nobody → bob.silverberg
Flags: needinfo?(aswan)
Priority: -- → P2
Whiteboard: [history] triaged
Assignee

Comment 2

3 years ago
Kris, if you don't mind giving me some pointers as to how we should implement this I'll give it a try.
Assignee

Comment 3

3 years ago
Kris, I probably have some time this week to work on this. I believe you said something about pre-processors when we discussed this in a meeting/triage. Would you be able to give me some pointers about how you think we should accomplish this?
Assignee

Comment 4

3 years ago
I've been looking at the code in Schemas.jsm to decide/figure out how to do this and I'm wondering, why should we use a preprocessor, rather than simply adding a `DateType` class that uses `normalize` to take the input and convert it to a date, via `normalizeTime` [1]? That seems more straightforward to me.

What I envision is that one could specify a type of `date` in a schema, and then the argument could accept any of the formats that are currently accepted by normalizeTime and would provide a date object as the result. This would mean changing some of our code (e.g., in ext-history.js and ext-downloads.js) to expect a date rather than a number, but I also think, in the long run, that will be simpler and easier to work with. 

I think this could also replace type: string, format: date, which doesn't seem to be used anywhere currently, or perhaps there is a different use case for that.

Kris/Andrew, what do you think of these suggestions?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#1236
Assignee

Comment 5

3 years ago
Andrew, do you have any thoughts on the above?
Flags: needinfo?(aswan)
I agree, normalize seems like a sane way to do this.
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
Assignee

Comment 8

3 years ago
mozreview-review
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73928/#review71780

::: toolkit/components/extensions/Schemas.jsm
(Diff revision 1)
>      if (error != null) {
>        throw new SyntaxError(error);
>      }
>      return string;
>    },
> -

I removed the string format `date` because, as far as I can tell, it isn't used anywhere, and I don't see why one would use it over the new `date` type that I have introduced. I'm happy to bring it back, however, if there is a reason to do so.

::: toolkit/components/extensions/Schemas.jsm:983
(Diff revision 1)
>  
> +class DateType extends Type {
> +  normalize(value, context) {
> +    let normalized;
> +    try {
> +      normalized = {value: normalizeTime(value)};

This is what we were debating in IRC. This seems like the proper place for the check to me, and it doesn't seem right to do all of the checks that `normalizeTime` does, before calling `nromalizeTime`.

However, after completing all of the changes I have found that we do not really need `normalizeTime` at all anymore, as this is the only place in the code from whence it's called. It is still used by some of the tests, but I guess it might be better to duplicate the code from `normalizeTime` into either the tests or `head.js` rather than keep it as a method in `ExtensionUtils.jsm`. What do you think?

Comment 9

3 years ago
mozreview-review
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72102

This now accepts anything that the Date constructor accepts which I think is overly broad.
(Since it accepts Date objects, an extension author who wants to rely on Date constructor formats can just create a Date object themselves and pass it in).
Also, please move normalizeTime into Schemas since that's now the only place it is used (you can just use the Date constructor from the unit test)

::: toolkit/components/extensions/ExtensionUtils.jsm:1580
(Diff revision 1)
>   *      A Date object
>   */
>  function normalizeTime(date) {
>    // Of all the formats we accept the "number of milliseconds since the epoch as a string"
>    // is an outlier, everything else can just be passed directly to the Date constructor.
> -  return new Date((typeof date == "string" && /^\d+$/.test(date))
> +  let result = new Date((typeof date == "string" && /^-?\d+$/.test(date))

Why would we accept negative numbers here?
Attachment #8784485 - Flags: review?(aswan) → review-
Comment hidden (mozreview-request)
Assignee

Comment 11

3 years ago
mozreview-review-reply
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72102

Why is it overly broad? What's the downside? Why is it better to restrict it to an arbitrary set of formats that represent dates that we have chosen?

I moved `normalizeTime` into Schemas.jsm in the commit I just pushed.

> Why would we accept negative numbers here?

Because they are valid. A negative number is just a date that is before 1970-01-01, and it's possible that a user could pass a date from the past into a method (e.g., a start date for a search which is before 1970).

Comment 12

3 years ago
mozreview-review-reply
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72102

I don't know why it was designed to be so broad, that's a question for the designers and implementors of Date.
"valid ISO8601 dates" is a restricted set but I wouldn't describe it as "arbitrary".

> Because they are valid. A negative number is just a date that is before 1970-01-01, and it's possible that a user could pass a date from the past into a method (e.g., a start date for a search which is before 1970).

Given that Firefox didn't exist then, nor did Netscape or even the web, and that the arpanet barely existed, I'm not seeing the need to support these.

Comment 13

3 years ago
mozreview-review-reply
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72102

There doesn't appear to be an anchor there to link to but see the note next to the dateString parameter documentation at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

Comment 14

3 years ago
mozreview-review
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72154

The issue about removing the ISO constraint on string inputs remains unaddressed.
Attachment #8784485 - Flags: review?(aswan) → review-
Comment hidden (mozreview-request)
Assignee

Comment 16

3 years ago
mozreview-review-reply
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72102

Fixed in `normalizeTime`.

> Given that Firefox didn't exist then, nor did Netscape or even the web, and that the arpanet barely existed, I'm not seeing the need to support these.

Fair enough. I was thinking more about what general data might be stored, and therefore retrieved, and that would not be limited by the age of the internet. Given that these dates are only currently used for history and downloads, I see your point about it being unlikely that we'd need to support such old dates. I was trying to make it flexible, but I guess that, because these will only be used by WebExtensions API methods, it makes sense that we'd not likely see a date that is far in the past.
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

Passing this over to Kris since I'm about to go out on PTO.
He also has more background with the schemas, I thought the built-in types handled by Schemas.jsm were meant to be the primitive types from json schema, so doing this new Date format as a built-in type in Schemas upends that, I don't know if that's a concern or not.

I would also favor keeping the "milliseconds since the epoch formatted as a string" out of any generic date type and just leave it for chrome compatibility in the downloads implementation.  It is an abomination that, even if we can't kill it, we shouldn't spread any further.
Attachment #8784485 - Flags: review?(aswan) → review?(kmaglione+bmo)

Comment 18

3 years ago
mozreview-review
Comment on attachment 8784485 [details]
Bug 1272676 - Refactor DownloadTime and HistoryTime into a shared schema type,

https://reviewboard.mozilla.org/r/73930/#review72584

See comments below. JSON Schema is very strict about what can be used in the "type" field, and "date" isn't an option.

I also agree with Andrew that we should only accept "milliseconds since the epoch" strings in the downloads API, where it's necessary for Chrome compatibility.

::: browser/components/extensions/schemas/history.json:112
(Diff revision 3)
>                "text": {
>                  "type": "string",
>                  "description": "A free-text query to the history service.  Leave empty to retrieve all pages."
>                },
>                "startTime": {
> -                "$ref": "HistoryTime",
> +                "type": "date",

The type field can only be one of the primitive types defined by JSON Schema: http://json-schema.org/latest/json-schema-core.html#rfc.section.3.5

::: toolkit/components/extensions/Schemas.jsm:20
(Diff revision 3)
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  Cu.import("resource://gre/modules/ExtensionUtils.jsm");
>  var {
> -  instanceOf,
> +  instanceOf

Trailing comma is required.

::: toolkit/components/extensions/Schemas.jsm:100
(Diff revision 3)
> +  const PATTERN = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?(Z|([-+]\d{2}:?\d{2})))?$|^\d+$/;
> +  if (typeof date == "string" && !PATTERN.test(date)) {
> +    throw new Error(`${date} is not a date in a valid format`);
> +  }
> +  let result = new Date((typeof date == "string" && /^\d+$/.test(date))
> +                        ? parseInt(date, 10) : date);

This is a bit hard to follow. How about something like:

    const PATTERN = /^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d{3})?(Z|([-+]\d{2}:?\d{2})))?$/;
  
    if (typeof date == "string") {
      if (/^\d+$/.test(date)) {
        date = parseInt(date, 10);
      } else if (!PATTERN.test(date)) {
        throw new Error(`${JSON.stringify(date)} is not a a valid date format`);
      }
    }
  
    let result = new Date(date);
Attachment #8784485 - Flags: review?(kmaglione+bmo) → review-
Assignee

Comment 19

3 years ago
Ok, I understand that my approach is incorrect, now. I thought I had made my intention clear in comment #4 and asked for feedback, but at that time nobody pointed out any issues with it, which is why I proceeded in this direction. Given that what I did was incorrect, and given that we need to keep some specific code in the downloads API, I'm not really sure what needs to be done for this bug, if anything. I'm going to leave it for now and address some other higher priority bugs I have, but if anyone has some specific advice about what exactly should be done to address this bug I'd appreciate hearing it.
I was looking at the original description about putting a shared type into extension_types.json.  But looking more closely, I guess we already have Date there.  I'm sorry for not reading this more closely, why doesn't history just use the built-in Date type?  It looks like HistoryTime supports strings with a numeric count of milliseconds since the epoch, but why?  Chrome doesn't support that and its such a bizarre and ambiguous representation that I don't think we should support it.
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8784485 - Attachment is obsolete: true
Assignee

Comment 22

3 years ago
From the beginning I was assuming that we just wanted to do the same thing for History dates as we did with Downloads dates, but now I finally understand that the thing we were doing for downloads, allowing for a string which is actually a number, was a specific hack required for Chrome compatibility. I agree that we don't need to, and shouldn't, do that for History, so I think all we really need to do at this point is remove that hack from the History code. I have attached a patch that does that. Let me know if you think we need to do anything else in terms of refactoring, but I don't think we need to at this point.
Flags: needinfo?(kmaglione+bmo)
Summary: Refactor DownloadTime and HistoryTime into a shared schema type → Remove HistoryTime and use Date type instead
Comment hidden (mozreview-request)

Comment 24

3 years ago
mozreview-review
Comment on attachment 8788530 [details]
Bug 1272676 - Remove HistoryTime and use Date type instead,

https://reviewboard.mozilla.org/r/76994/#review75132
Attachment #8788530 - Flags: review?(aswan) → review+
Assignee

Comment 25

3 years ago
Try looks good (including Windows tests ;), so requesting checkin.
Keywords: checkin-needed

Comment 26

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89d3a48ed472
Remove HistoryTime and use Date type instead, r=aswan
Keywords: checkin-needed

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89d3a48ed472
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.