Implement browser.history.search

RESOLVED FIXED in Firefox 49

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

(Depends on 1 bug, {dev-doc-complete})

unspecified
mozilla49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [history]triaged)

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Updated

3 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Whiteboard: [history]triaged
Assignee

Updated

3 years ago
Blocks: 1265835
Assignee

Updated

3 years ago
No longer blocks: 1265835
Assignee

Updated

3 years ago
Depends on: 1269398
Assignee

Comment 1

3 years ago
We're not going to implement ` typedCount` for now, so this no longer depends on bug 1269398.
No longer depends on: 1269398
Assignee

Updated

3 years ago
Depends on: 1271675
Assignee

Comment 2

3 years ago
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/
Attachment #8750831 - Flags: review?(mak77)
Attachment #8750831 - Flags: review?(aswan)
Assignee

Comment 3

3 years ago
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.
Assignee

Updated

3 years ago
Iteration: --- → 49.2 - May 23
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?
Attachment #8750831 - Flags: review?(aswan)
Assignee

Updated

3 years ago
Depends on: 1271801
Assignee

Comment 5

3 years ago
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/
Attachment #8750831 - Flags: review?(aswan)
Assignee

Comment 6

3 years ago
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.
Assignee

Comment 7

3 years ago
Btw, thanks for the detailed review, Andrew!
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 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
Attachment #8750831 - Flags: review?(aswan)
Attachment #8750831 - Flags: review?(mak77)
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)
Assignee

Comment 11

3 years ago
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/
Attachment #8750831 - Attachment description: MozReview Request: Bug 1265834 - Implement browser.history.search, r?aswan r?mak → MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak
Attachment #8750831 - Flags: review?(mak77)
Attachment #8750831 - Flags: review?(aswan)
Assignee

Comment 12

3 years ago
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.
Assignee

Comment 13

3 years ago
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.
Assignee

Comment 14

3 years ago
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.
(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.
Assignee

Comment 16

3 years ago
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/
Attachment #8750831 - Attachment description: MozReview Request: Bug 1265834 - Part 2: Implement browser.history.search, r?aswan r?mak → MozReview Request: Bug 1265834 - Part 1: Move normalizeTime from downloads into a shared util, r?aswan
Attachment #8751893 - Flags: review?(mak77)
Attachment #8751893 - Flags: review?(aswan)
Attachment #8750831 - Flags: review?(mak77)
Assignee

Comment 17

3 years ago
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/
Assignee

Comment 18

3 years ago
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.
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 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()
Attachment #8751893 - Flags: review?(aswan) → review+
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
Attachment #8751893 - Flags: review?(mak77) → review+
Assignee

Comment 22

3 years ago
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/
Assignee

Updated

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

Comment 23

3 years ago
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.
Assignee

Comment 24

3 years ago
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/
Attachment #8752211 - Flags: review?(mak77)
Attachment #8752211 - Flags: review?(aswan)
Assignee

Comment 25

3 years ago
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 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 ;)
Attachment #8752211 - Flags: review?(mak77) → review+
Assignee

Comment 27

3 years ago
Thanks Marco. I didn't think I needed one, but ReviewBoard requested one on my behalf. Should I have just removed the flag myself?
ideally it should not change a + flag, likely due to the rebase. not a big deal.
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?
Attachment #8750831 - Flags: review?(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.
Attachment #8750831 - Flags: review+
Assignee

Comment 31

3 years ago
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/
Assignee

Updated

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

Updated

3 years ago
Attachment #8753023 - Flags: review?(aswan)
Assignee

Updated

3 years ago
Attachment #8752211 - Flags: review?(aswan)
Assignee

Updated

3 years ago
No longer depends on: 1271675
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 35

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b80bb33f4df5
https://hg.mozilla.org/mozilla-central/rev/fe117635382b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Updated

3 years ago
Keywords: dev-doc-needed
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.

Updated

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