Implement browser.history.getVisits

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [history]triaged)

Attachments

(1 attachment)

Assignee

Updated

3 years ago
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
No longer depends on: 1265834
Assignee

Updated

3 years ago
Blocks: 1265836
Assignee

Updated

3 years ago
No longer blocks: 1265836
Assignee

Updated

3 years ago
Depends on: 1269398
Depends on: 1271801
Assignee

Updated

3 years ago
No longer depends on: 1271801
Assignee

Comment 2

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

Comment 4

3 years ago
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/
Attachment #8757930 - Attachment description: MozReview Request: Bug 1265835 - Implement browser.history.getVisits, r?aswan → Bug 1265835 - Implement browser.history.getVisits,
Attachment #8757930 - Flags: review?(bob.silverberg)
Attachment #8757930 - Flags: review?(aswan)
Assignee

Comment 5

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

Comment 7

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

Comment 8

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

Not sure how I managed to r? myself on this bug. :P
Attachment #8757930 - Flags: review?(bob.silverberg)
Comment on attachment 8757930 [details]
Bug 1265835 - Implement browser.history.getVisits,

clearing this for now so it stops nagging me
Attachment #8757930 - Flags: review?(aswan)
Assignee

Updated

3 years ago
Duplicate of this bug: 1271675
Assignee

Comment 11

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

Updated

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

Comment 13

3 years ago
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/
Attachment #8757930 - Flags: review?(bob.silverberg)
Assignee

Comment 14

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

Comment 16

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

Comment 18

3 years ago
There's a lot of orange in that try run, but none of it seems to be related to the changes in this patch.
Keywords: checkin-needed
Assignee

Updated

3 years ago
Attachment #8757930 - Flags: review?(bob.silverberg)
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!
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed
Assignee

Comment 20

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

Comment 21

3 years ago
Thanks Carsten. I've fixed the merge conflicts. Let's try again. :)
Flags: needinfo?(bob.silverberg)
Keywords: checkin-needed

Comment 22

3 years ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/518e72986fab
Implement browser.history.getVisits, r=aswan
Keywords: checkin-needed

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/518e72986fab
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

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