1265835 - Implement browser.history.getVisits

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1265835 - Implement browser.history.getVisits,

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

Comment 2

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56288/#review52926

::: browser/components/extensions/test/browser/browser_ext_history.js:182
(Diff revision 1)
>      isnot(result, null, `history.search result was found for ${url}`);
>      is(result.visitCount, expectedCount, `history.search reports ${expectedCount} visit(s)`);
>      is(result.title, `test visit for ${url}`, "title for search result is correct");
>    }
>  
> +  function checkVisitResults(results, url) {

I'd like to add some tests that check for specific times and trasitions in the results, but we have no way to add test data with specific times and transitions until bug 1265836 lands, which will add `addUrl`.  If this is ready to land before that one lands, then I think we should land it and open a bug to add more tests. Alternatively we can wait for bug 1265836 to land first, but I'm not sure how long that will be as it was backed out for a reason that I have yet to figure out.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

https://reviewboard.mozilla.org/r/56288/#review53596

::: browser/components/extensions/ext-history.js:20
(Diff revision 1)
> +const TRANSITION_TYPE_TO_TRANSITIONS_MAP = new Map([
> +  [History.TRANSITION_LINK, "link"],
> +  [History.TRANSITION_TYPED, "typed"],
> +  [History.TRANSITION_BOOKMARK, "auto_bookmark"],
> +  [History.TRANSITION_EMBED, "auto_subframe"],
> +  [History.TRANSITION_REDIRECT_PERMANENT, "link"],
> +  [History.TRANSITION_REDIRECT_TEMPORARY, "link"],
> +  [History.TRANSITION_DOWNLOAD, "link"],
> +  [History.TRANSITION_FRAMED_LINK, "manual_subframe"],
> +]);
> +
> +function getTransition(transitionType) {
> +  return TRANSITION_TYPE_TO_TRANSITIONS_MAP.get(transitionType) || "link";
> +}

This looks like the inverse of the information in the addUrl() patch?  You could make this a little more DRY by initializing one map from the other, here is an example of some existing code that does something similar:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#3072-3091
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#3291-3306

And although its verbose, I think names that explain both the input and output types might be helpful here for when you eventually have functions for mapping both directions, e.g. `getTransitionFromString()`

::: browser/components/extensions/ext-history.js:40
(Diff revision 1)
>  /*
> - * Converts a nsINavHistoryResultNode into a plain object
> + * Converts a nsINavHistoryResultNode into either a HistoryItem or a VisitItem
>   *
>   * https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsINavHistoryResultNode
>   */
> -function convertNavHistoryResultNode(node) {
> +function convertNavHistoryResultNode(node, format) {

Putting these both into a single function feels awkward to me, any good reason not to just have separate functions for HistoryItem and VisitItem?

::: browser/components/extensions/ext-history.js:63
(Diff revision 1)
>  /*
>   * Converts a nsINavHistoryContainerResultNode into an array of objects
>   *
>   * https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsINavHistoryContainerResultNode
>   */
> -function convertNavHistoryContainerResultNode(container) {
> +function convertNavHistoryContainerResultNode(container, format = "HistoryItem") {

Instead of passing a string here, if you create separate functions for converting to HistoryItem and VisitItem, then you could just make this take the node conversion function as a parameter.

::: browser/components/extensions/ext-history.js:128
(Diff revision 1)
> +        let queryResult = History.executeQuery(historyQuery, options).root;
> +        let results = convertNavHistoryContainerResultNode(queryResult, "VisitItem");

This code all runs synchronously?  wow...

::: browser/components/extensions/test/browser/browser_ext_history.js:148
(Diff revision 1)
> +        browser.test.assertEq(
> +          error.message,
> +          "A URL must be provided for getVisits",
> +          "history.getVisits rejects with an empty url argument");
> +      }).then(() => {
> +        return browser.history.getVisits({url: "http://example.com/"});

you have const's for these above, use them here?  (and again a few lines down)

::: browser/components/extensions/test/browser/browser_ext_history.js:153
(Diff revision 1)
> +        browser.test.sendMessage("get-visits", results);
> +        browser.test.assertEq(2, results.length, "Expected number of visits returned");

you do some of the validation here but then some of the validation back in the test script, can you just put it all in one place?  (presumably the test script)
also, why do you validate the results of the second call to getVisits() but not the first?

::: browser/components/extensions/test/browser/browser_ext_history.js:187
(Diff revision 1)
> +  function checkVisitResults(results, url) {
> +    let visitTime, visitId;
> +    for (let result of results) {
> +      is(result.id, urlIds.get(url), "id for history.getVisits is correct");
> +      is(result.transition, "link", "transition for history.getVisits is correct");
> +      isnot(result.visitTime, visitTime, "each visit has a different visitTime");

what actually guarantees this?

::: browser/components/extensions/test/browser/browser_ext_history.js:188
(Diff revision 1)
> +    let visitTime, visitId;
> +    for (let result of results) {
> +      is(result.id, urlIds.get(url), "id for history.getVisits is correct");
> +      is(result.transition, "link", "transition for history.getVisits is correct");
> +      isnot(result.visitTime, visitTime, "each visit has a different visitTime");
> +      isnot(result.visitId, visitId, "each visit has a different visitId");

you're just comparing these pair-wise, if you want to make sure they're unique, how about putting them all in a `Set`

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

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

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56288/#review53596

> This looks like the inverse of the information in the addUrl() patch?  You could make this a little more DRY by initializing one map from the other, here is an example of some existing code that does something similar:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#3072-3091
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#3291-3306
> 
> And although its verbose, I think names that explain both the input and output types might be helpful here for when you eventually have functions for mapping both directions, e.g. `getTransitionFromString()`

Interesting. I do like the looks of that. I recall that the first time I wrote this (the code for which I seem to have lost), it wasn't a direct mapping both ways, which is why I maintaned two maps, but it does look that way now. I think I'll hold off on this until bug 1265836 lands (which, with any luck, should be soon) and then rebase and figure out the best way to reduce duplication.

> This code all runs synchronously?  wow...

Yes. The executeQuery is synchronous, and I don't think the converter adds much, but I guess it might if we had a huge result set. Do you think I should try to make that asynchronous?

> you have const's for these above, use them here?  (and again a few lines down)

The consts are declared in the task, but this code is running in the background page, so it doesn't have access to those consts. I changed the code to pass the consts that I need into the background page, but it seems a bit ugly. Is there a better way to share variables between the test and the background page?

> what actually guarantees this?

They are created one after another, but I guess if they are created quickly enough it's possible that two could share the same time, so I have removed this.

Comment 6

[Andrew Swan [:aswan]]

Bob, it looks like 1265836 is getting close to landing, can review of this wait until you've rebased onto that and completed tests etc?

Comment 7

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #6)
> Bob, it looks like 1265836 is getting close to landing, can review of this
> wait until you've rebased onto that and completed tests etc?

Definitely. In fact it just landed on inbound, but I'm not going to count my chickens yet. I'll add a comment once it's been rebased and ready for another review.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

Not sure how I managed to r? myself on this bug. :P

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

clearing this for now so it stops nagging me

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

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

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

https://reviewboard.mozilla.org/r/56288/#review54840

::: browser/components/extensions/ext-history.js:7
(Diff revision 3)
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",

Can you keep the imports sorted alphabetically

::: browser/components/extensions/ext-history.js:44
(Diff revision 3)
>      throw new Error(`|${transition}| is not a supported transition for history`);
>    }
>    return transitionType;
>  }
>  
> +function getTransition(transitionType) {

The distinction between "Transition" and "TransitionType" isn't immediately clear, but I don't have a great idea for better names.

::: browser/components/extensions/ext-history.js:83
(Diff revision 3)
> +/*
>   * Converts a nsINavHistoryContainerResultNode into an array of objects
>   *
>   * https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsINavHistoryContainerResultNode
>   */
> -function convertNavHistoryContainerResultNode(container) {
> +function convertNavHistoryContainerResultNode(container, converter = convertNodeToHistoryItem) {

I think overall readability would be better if you took out the default value.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

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

Comment 14

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/56288/#review54840

> Can you keep the imports sorted alphabetically

I went ahead and removed the lazy import which didn't really make sense but was there to deal with that bug we saw trying to land addUrl. Now that the bug is fixed we don't need the lazy import anymore. I'm not sure if the sequence is okay now or not. I feel like it's better to keep the `Cu.import` calls together and separate from the `XPCOMUtils.defineLazyModuleGetter` calls, which is what I've done.

> The distinction between "Transition" and "TransitionType" isn't immediately clear, but I don't have a great idea for better names.

Yeah, I can see how this isn't wonderfully clear to someone not familiar with places and the API. TransitionType is used by Firefox/Gecko and Transition is used by Chrome/the API, which is where the distinction lies.

Comment 15

[Bob Silverberg [:bsilverberg]]

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

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

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

Comment 17

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=612b1b82d576

Comment 18

[Bob Silverberg [:bsilverberg]]

There's a lot of orange in that try run, but none of it seems to be related to the changes in this patch.

Comment 19

[Carsten Book [:Tomcat]]

seems has conflicts:

patching file browser/components/extensions/ext-history.js
Hunk #1 FAILED at 0
Hunk #2 FAILED at 123
2 out of 2 hunks FAILED -- saving rejects to file browser/components/extensions/ext-history.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue 

could you take a look, thanks!

Comment 20

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

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

Comment 21

[Bob Silverberg [:bsilverberg]]

Thanks Carsten. I've fixed the merge conflicts. Let's try again. :)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/518e72986fab
Implement browser.history.getVisits, r=aswan

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/518e72986fab