1265834 - Implement browser.history.search

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

As per https://developer.chrome.com/extensions/history#method-search

Comment 1

[Bob Silverberg [:bsilverberg]]

We're not going to implement ` typedCount` for now, so this no longer depends on bug 1269398.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

Requesting review from mak for the changes to PlacesUtils.jsm.
Note that one of these changes (toPRTime) is also present in the patch for bug 1265836, but I anticipate that this patch may land before that bug.

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

Comment 3

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51655/#review48453

::: browser/components/extensions/test/browser/browser_ext_history.js:106
(Diff revision 1)
>    is(PlacesUtils.history.hasHistoryEntries, false, "history is empty");
>  
>    yield extension.unload();
>  });
> +
> +add_task(function* test_search() {

I realize that I am missing tests for searching based on a specific date range, in which results are expected. This is because it is currently not simple to add history with specific (i.e., expected) dates, but it will become so once bug 1265836 lands. I therefore opened bug 1271675 to track adding these tests at a later date.

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

https://reviewboard.mozilla.org/r/51655/#review48525

::: browser/components/extensions/ext-history.js:65
(Diff revision 1)
>          return History.remove(url).then(() => undefined);
>        },
> +      search: function(query) {
> +        let options = History.getNewQueryOptions();
> +        options.resultType = options.RESULTS_AS_URI;
> +        options.maxResults = query.maxResults || 100;

Is 0 a valid value for maxResults?  If it is, you're going to turn it in to 100 here.

::: browser/components/extensions/ext-history.js:69
(Diff revision 1)
> +        options.resultType = options.RESULTS_AS_URI;
> +        options.maxResults = query.maxResults || 100;
> +
> +        let historyQuery = History.getNewQuery();
> +        historyQuery.searchTerms = query.text;
> +        historyQuery.beginTime = PlacesUtils.toPRTime(query.startTime) || PlacesUtils.toPRTime(Date.now() -  24 * 60 * 60 * 1000);

Can you make this line and the next line be consistent?  ie, use the ternary operator in both lines...

::: browser/components/extensions/schemas/history.json:111
(Diff revision 1)
>                "text": {
>                  "type": "string",
>                  "description": "A free-text query to the history service.  Leave empty to retrieve all pages."
>                },
>                "startTime": {
>                  "type": "number",

For downloads, we expanded what is accepted for time values to include numbers, strings, and Date objects:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/downloads.json#212-224

It might be nice to do the same here...

::: browser/components/extensions/test/browser/browser_ext_history.js:106
(Diff revision 1)
>    is(PlacesUtils.history.hasHistoryEntries, false, "history is empty");
>  
>    yield extension.unload();
>  });
> +
> +add_task(function* test_search() {

this test (or a separate one) could test some negative cases as well.  like omitting the mandatory "text" property, a negative maxResults, an endTime that comes before startTime, etc.

::: browser/components/extensions/test/browser/browser_ext_history.js:147
(Diff revision 1)
> +    return results.find(r => r.url === url);
> +  }
> +
> +  function checkResult(results, url, expectedCount) {
> +    let result = findResult(url, results);
> +    is(result.visitCount, expectedCount, `history.search reports ${expectedCount} visit(s)`);

An `isnot(result, null, "...")` would be good here for better error reporting if a bug is introduced where something isn't found.

::: browser/components/extensions/test/browser/browser_ext_history.js:165
(Diff revision 1)
> +  ]);
> +
> +  extension.sendMessage("check-history");
> +
> +  let results = yield extension.awaitMessage("empty-search");
> +  ok(results.length >= 3, "history.search returned at least 3 results");

shouldn't this be exactly 3?  and why not look for the mozilla url?

::: browser/components/extensions/test/browser/browser_ext_history.js:175
(Diff revision 1)
> +  is(results.length, 1, "history.search returned 1 result");
> +  checkResult(results, MOZILLA_VISIT_URL, 1);
> +
> +  results = yield extension.awaitMessage("max-results-search");
> +  is(results.length, 1, "history.search returned 1 result");
> +  checkResult(results, DOUBLE_VISIT_URL, 2);

where is it specified which result(s) you get if you're limiting results with maxResults?

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51655/diff/1-2/

Comment 6

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51655/#review48525

> Is 0 a valid value for maxResults?  If it is, you're going to turn it in to 100 here.

The schema does have the minimum value for maxResults listed as 0, but that doesn't really make sense to me. Why would you run a query when you are saying you want 0 results back? That would be a waste of processing power to run the query. The default value is 100 [1], which is why I wrote the logic like that. I've updated the schema to make the minimum allowable value of maxResults 1, which I think makes sense.

[1] https://developer.chrome.com/extensions/history#method-search

> For downloads, we expanded what is accepted for time values to include numbers, strings, and Date objects:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/downloads.json#212-224
> 
> It might be nice to do the same here...

Interesting. I've enabled that logic for `search`'s `startTime` and `endTime`. I've moved `normalizeTime` into `ExtensionUtils.jsm` in order to share the function between the API classes. I've only  implemented this for `search` for now, as that's what this bug is about, but I wonder if I should also do it for `deleteRange` which also accepts date arguments. What do you think?

> this test (or a separate one) could test some negative cases as well.  like omitting the mandatory "text" property, a negative maxResults, an endTime that comes before startTime, etc.

Good ideas. I have chosen not to test for things that are caught automatically based on the schema. Things such as a missing "text" property (which is marked as mandatory in the schema) and a negative maxResults (which _was_ marked with a minimum of 0 and will be updated to a minimum of 1) are caught by code that is outside of the API code itself. I assume (perhaps incorrectly so) that we have unit tests in place which test that the schema rules are properly enforced, so it seems unnecesary to test for anything that breaks the schema rules in the tests for the API itself.

The mismatched start and end time are a good idea, and not something that I was specifically checking for in the API, so I've added a check and a test for that.

Do you see a problem with my approach of not testing for arguments that are forbidden by the schema?

> shouldn't this be exactly 3?  and why not look for the mozilla url?

Odd. When I was first writing the test history.search was returning 4 results, with one of them being the fake extension page. When I went to check the exact url that was being returned in the 4th result I now find that it's only returning 3, so yes, it looks like it's safe to assert that exactly 3 are returned. I  added a check for the Mozilla URL that was missing. I think I added that URL later in the development of the tests after this part was written.

> where is it specified which result(s) you get if you're limiting results with maxResults?

It's not specified in any docs anywhere. I am assuming we should sort it in reverse chronological order, and I have added a query option to specify that.

Comment 7

[Bob Silverberg [:bsilverberg]]

Btw, thanks for the detailed review, Andrew!

Comment 8

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/51655/#review48525

> Good ideas. I have chosen not to test for things that are caught automatically based on the schema. Things such as a missing "text" property (which is marked as mandatory in the schema) and a negative maxResults (which _was_ marked with a minimum of 0 and will be updated to a minimum of 1) are caught by code that is outside of the API code itself. I assume (perhaps incorrectly so) that we have unit tests in place which test that the schema rules are properly enforced, so it seems unnecesary to test for anything that breaks the schema rules in the tests for the API itself.
> 
> The mismatched start and end time are a good idea, and not something that I was specifically checking for in the API, so I've added a check and a test for that.
> 
> Do you see a problem with my approach of not testing for arguments that are forbidden by the schema?

I agree with not needing to repeatedly test the schema validation mechanism, but the gap here is if there is a typo or error in the schema, it could go unnoticed.  But overall I don't feel strongly about it here.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

https://reviewboard.mozilla.org/r/51655/#review48939

::: browser/components/extensions/schemas/history.json:95
(Diff revisions 1 - 2)
>              "description": "The $(topic:transition-types)[transition type] for this visit from its referrer."
>            }
>          }
> +      },
> +      {
> +        "id": "HistoryTime",

I think it would make sense to make this something shared at manifest.json.  :kmag will likely have an opinion about that too...

::: toolkit/components/extensions/ExtensionUtils.jsm:1207
(Diff revision 2)
> + * @returns (Number)
> + *      The number of milliseconds since the epoch for the date
> + */
> +function normalizeTime(date, start) {
> +  if (date == null) {
> +    return start ? 0 : Number.MAX_VALUE;

I think the handling of defaults here was specific to downloads and should remain there rather than in this shared function.
Also, I think it would make sense to have the move of normalizeTime() be part of a separate commit from the actual new history code.

::: toolkit/components/extensions/schemas/downloads.json:214
(Diff revision 2)
>            }
>          }
>        },
>        {
>          "id": "DownloadTime",
> -        "description": "A time specified as a Date object, a number of string representing milliseconds since the epoch, or an ISO 8601 string",
> +        "description": "A time specified as a Date object, a number or string representing milliseconds since the epoch, or an ISO 8601 string",

heh, thanks for catching that

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

https://reviewboard.mozilla.org/r/51655/#review49077

::: browser/components/extensions/ext-history.js:78
(Diff revision 2)
> +        if (beginTime > endTime) {
> +          return Promise.reject({message: "The startTime cannot be after the endTime"});
> +        }
> +
> +        let options = History.getNewQueryOptions();
> +        options.resultType = options.RESULTS_AS_URI;

RESULTS_AS_URI is the default, I think you can omit it

::: browser/components/extensions/ext-history.js:80
(Diff revision 2)
> +        }
> +
> +        let options = History.getNewQueryOptions();
> +        options.resultType = options.RESULTS_AS_URI;
> +        options.sortingMode = options.SORT_BY_DATE_DESCENDING;
> +        options.maxResults = query.maxResults || 100;

Regarding the 0 discussion, I honestly think passing 0 is a coding mistake, so this should throw or reject, rather than hiding the coding mistake.

::: toolkit/components/places/PlacesUtils.jsm:291
(Diff revision 2)
> +   * @return time
> +   *        milliseconds from the epoch.
> +   */
> +  toTime: function PU_toTime(time) {
> +    return time / 1000;
> +  },

There is something wrong with javadocs here, and I thought we were just inserting a toDate here (getting time from Date is trivial and mostly automatic)

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51655/diff/2-3/

Comment 12

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51655/#review48939

> I think it would make sense to make this something shared at manifest.json.  :kmag will likely have an opinion about that too...

Do you think we can open a follow-up bug about that? I'm wary of adding more and more shared stuff to this bug.

> I think the handling of defaults here was specific to downloads and should remain there rather than in this shared function.
> Also, I think it would make sense to have the move of normalizeTime() be part of a separate commit from the actual new history code.

I've moved the definition of `normalizeTime` plus the changes to downloads into a separate commit. I do think that the defaults logic makes sense outside of the context of downloads, which is why I brought it along. It's a nice helper that creates a logical default for dates when used as start and end dates, and I'm even making use of that logic in the history code.

Having said that, is is specific to dates that represent the beginning and/or end of a range, rather than *all* dates, so maybe the function name should be changed to reflect that? `normalizeTimeBoundary` maybe? Although I don't really like that either. What do you think? I can just remove the logic as you suggest, but I do think modules other than downloads could find it useful.

Comment 13

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51655/#review49077

> Regarding the 0 discussion, I honestly think passing 0 is a coding mistake, so this should throw or reject, rather than hiding the coding mistake.

By changing the schema to state that `1` is the minimum allowable value I have effectively done that.

> There is something wrong with javadocs here, and I thought we were just inserting a toDate here (getting time from Date is trivial and mostly automatic)

This function is to convert a `PRTime` into a number that represents the date in milliseconds since the epoch. It is a simple calculation (just divide by 1000), but I do have to do it a few times in my code which is why I added the function. It's true that I could convert to a Date instead (using `toDate` which is being added in a different patch), but then I'd also have to convert that Date into a Number, which is easy, but is extra processing. I thought it made sense that if all I want to do is convert a PRTime into a time, I might as well just divide by 1000, rather than converting to a Date first, which is why I added this helper method.

I can remove it and just add and use `toDate` instead if you disagree with my logic above. Please let me know.

Comment 14

[Bob Silverberg [:bsilverberg]]

Andrew and Marco, I somehow managed to mess up the review on MozReview (I think because I rebased in order to reorder commits) and now I cannot figure out how to get both commits up. I have a separate commit, which isn't there, which deals with the `normalizeTime` stuff. When I try to push it it keeps *replacing* the current commit, which is not what I want. I'm not sure how to resolve this, but I'll try to figure it out. Maybe I can just delete the whole review and start again, but I don't want to do that while there are active comments.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> > There is something wrong with javadocs here, and I thought we were just inserting a toDate here (getting time from Date is trivial and mostly automatic)

My comment was mostly due to the fact Places APIs will accept Date objects, rather than timestamps.
Btw, it's not a big deal, you can add both toDate and toTime, we won't surely enter a crysis for an additional one line helper. I didn't mean to stop you!

(In reply to Bob Silverberg [:bsilverberg] from comment #14)
> Andrew and Marco, I somehow managed to mess up the review on MozReview (I
> think because I rebased in order to reorder commits)

I thought the problem was resolved with mozreview ids in the commit messages... But maybe not.
In any case, just fix the zombie javadoc in PlacesUtils, and I'm done.

Comment 16

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

Requesting review from mak for the changes to PlacesUtils.jsm.
Note that one of these changes (toPRTime) is also present in the patch for bug 1265836, but I anticipate that this patch may land before that bug.

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

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51655/diff/3-4/

Comment 18

[Bob Silverberg [:bsilverberg]]

Ok, I think I fixed the MozReview issue, although the issues are now aligned with the wrong commits, but I think that's ok as we all know what the current status is, and can just continue the review from here.

Comment 19

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/51655/#review48939

> Do you think we can open a follow-up bug about that? I'm wary of adding more and more shared stuff to this bug.

sure

> I've moved the definition of `normalizeTime` plus the changes to downloads into a separate commit. I do think that the defaults logic makes sense outside of the context of downloads, which is why I brought it along. It's a nice helper that creates a logical default for dates when used as start and end dates, and I'm even making use of that logic in the history code.
> 
> Having said that, is is specific to dates that represent the beginning and/or end of a range, rather than *all* dates, so maybe the function name should be changed to reflect that? `normalizeTimeBoundary` maybe? Although I don't really like that either. What do you think? I can just remove the logic as you suggest, but I do think modules other than downloads could find it useful.

My instinct is that that is specialized enough -- some APIs might take null to represent "from the earliest time possible" or "until the latest time possible", others might take Date objects with extreme values -- that the individual callers should do this themselves.  Plus it is such a simple operation that I don't think there's a lot of value in having a shared implementation of it.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8751893 [details]
MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

https://reviewboard.mozilla.org/r/52277/#review49313

::: browser/components/extensions/ext-history.js:69
(Diff revision 1)
>          let url = details.url;
>          // History.remove returns a boolean, but our API should return nothing
>          return History.remove(url).then(() => undefined);
>        },
> +      search: function(query) {
> +        let beginTime = (query.startTime == null) ?

Ah, this is exactly what I was talking about on the other patch, a missing start time means something specific to history here and you handle it here so this path will never use the start time logic you currently have in normalizeTime()

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8751893 [details]
MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

https://reviewboard.mozilla.org/r/52277/#review49363

::: toolkit/components/places/PlacesUtils.jsm:270
(Diff revision 1)
> +   *
> +   * @param date
> +   *        the Date object to convert.
> +   * @return microseconds from the epoch.
> +   */
> +  toPRTime: function PU_toPRTime(date) {

nit: it is no more needed to provide a label for the function, in addition to the property name. And you could also use ES6 shorthands, so this becomes just:

toPRTime(date) {
  ...

See https://developer.mozilla.org/it/docs/Web/JavaScript/Reference/Functions/Method_definitions

::: toolkit/components/places/PlacesUtils.jsm:282
(Diff revision 1)
> +   * @param time
> +   *        microseconds from the epoch.
> +   * @return time
> +   *        milliseconds from the epoch.
> +   */
> +  toTime: function PU_toTime(time) {

ditto

Comment 22

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51655/diff/4-5/

Comment 23

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/51655/#review48939

> sure

Ok, I've opened bug 1272676 to track this.

> My instinct is that that is specialized enough -- some APIs might take null to represent "from the earliest time possible" or "until the latest time possible", others might take Date objects with extreme values -- that the individual callers should do this themselves.  Plus it is such a simple operation that I don't think there's a lot of value in having a shared implementation of it.

I agree, now. :)  I have changed it to remove the default handling.

Comment 24

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

Requesting review from mak for the changes to PlacesUtils.jsm.
Note that one of these changes (toPRTime) is also present in the patch for bug 1265836, but I anticipate that this patch may land before that bug.

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

Comment 25

[Bob Silverberg [:bsilverberg]]

Ugh! I kept the MozReview-Commit-ID from the previous commit message in each of the commits, but it still seems to be messed up, again. It has now discarded the entire history for Part 2, which sucks, but I believe I have addressed all of the comments, so I think this is finally ready to land. Unfortunately I need you two to give it one last look. My apologies for messing this up so badly. I think it was the rebase that I did and I will try to avoid doing that again in the future.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 8752211 [details]
MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

https://reviewboard.mozilla.org/r/52471/#review49449

FWIW, you didn't need a further review just for these changes ;)

Comment 27

[Bob Silverberg [:bsilverberg]]

Thanks Marco. I didn't think I needed one, but ReviewBoard requested one on my behalf. Should I have just removed the flag myself?

Comment 28

[Marco Bonardo [:mak]]

ideally it should not change a + flag, likely due to the rebase. not a big deal.

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

https://reviewboard.mozilla.org/r/51655/#review49746

::: toolkit/components/extensions/ext-downloads.js:245
(Diff revision 5)
>          queryTerms.push(term.toLowerCase());
>        }
>      }
>    }
>  
> -  function normalizeTime(arg, before) {
> +  function normalizeDownloadTime(arg, before) {

why are you changing this?

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8750831 [details]
MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

https://reviewboard.mozilla.org/r/51655/#review49748

::: toolkit/components/extensions/ext-downloads.js:245
(Diff revision 5)
>          queryTerms.push(term.toLowerCase());
>        }
>      }
>    }
>  
> -  function normalizeTime(arg, before) {
> +  function normalizeDownloadTime(arg, before) {

Oh sorry, didn't read closely enough.

Comment 31

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8752211 [details]
MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52471/diff/1-2/

Comment 32

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05fa42ce4e70

Comment 33

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan

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

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b80bb33f4df5
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe117635382b

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b80bb33f4df5
https://hg.mozilla.org/mozilla-central/rev/fe117635382b

Comment 36

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/cf467a128c95a82bcaa1e2e583289e4c2e12513f
Bug 1265834: Follow-up: Fix JSDoc syntax. r=trivial DONTBUILD

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf467a128c95

Comment 38

[Eric Shepherd [:sheppy]]

Docs updated: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/History/search

Also noted history API is available in Firefox 49.

This is part of a cleanup and update of the entire history API section.