1265837 - Implement browser.history.deleteUrl

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 2

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/48779/#review45613

::: browser/components/extensions/test/browser/browser_ext_history.js:22
(Diff revision 1)
>    yield extension.startup();
>    yield extension.awaitFinish("history-schema");
>    yield extension.unload();
>  });
> +
> +add_task(function* test_delete_url() {

I know that I could test some other things, such as adding multiple different URLs and making sure that only the one that I specified is removed, but I chose not to do so. 

There are already extensive tests for `PlacesUtils.history.remove` [1] so I don't think it's necessary to test it more here. We really just need to test whether we have correctly called that method, and I think this does the trick.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/history/test_remove.js

::: browser/components/extensions/test/browser/head.js:144
(Diff revision 1)
>    }
>  
>    return Promise.resolve();
>  }
>  
> +// From toolkit/components/places/tests/head_common.js

I don't love the fact that I've copied and pasted code from `toolkit/components/places/tests/head_common.js` into this file, but I'm not sure if we can, or even want to, reference that file for these tests. What do you think?

::: browser/components/extensions/test/browser/head.js:208
(Diff revision 1)
> + * Checks how many visits exist for a specified page.
> + * @param aURI
> + *        nsIURI or address to look for.
> + * @return number of visits found.
> + */
> +function visits_in_database(aURI)

I realize that I'm not calling this function in the current test, but I will be using it for the next one I write, so I thought I would include it now as I was already doing the copying and pasting. If we can reference this code from `toolkit/components/places/tests/head_common.js` that will become a non-issue.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

https://reviewboard.mozilla.org/r/48779/#review45619

::: browser/components/extensions/ext-history.js:7
(Diff revision 1)
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");

We've generally done these imports lazily, can you do that here?

::: browser/components/extensions/ext-history.js:16
(Diff revision 1)
>    return {
> -    history: {},
> +    history: {
> +      deleteUrl: function(details) {
> +        let url = details.url;
> +        return History.remove(url).then(() => {
> +          return Promise.resolve();

The extra promise chained on here is unnecessary.
(or if `History.remove()` resolves to something that you want to scrub from the result, then you can just return undefined here instead of `Promise.resolve()`)

::: browser/components/extensions/test/browser/head.js:208
(Diff revision 1)
> + * Checks how many visits exist for a specified page.
> + * @param aURI
> + *        nsIURI or address to look for.
> + * @return number of visits found.
> + */
> +function visits_in_database(aURI)

Yes, lets not duplicate this code.  I'm not sure exactly how to do this for a browsert test, can you check in with Kris?

Comment 4

[Bob Silverberg [:bsilverberg]]

Thanks Andrew! I'll reply to those issues shortly.

Kris, how would I go about reusing code from `toolkit/components/places/tests/head_common.js` in a test that's in `browser/components/extensions/test/browser/`?

Comment 5

[Kris Maglione [:kmag]]

Generally speaking, you tests don't use test resources from other parts of the tree. If you need to share code like that, it's generally best to put it in a test-only JSM. In this case, probably PlacesTestUtils.jsm. Which should probably be reviewed by a Places peer.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 7

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/48779/#review45619

> Yes, lets not duplicate this code.  I'm not sure exactly how to do this for a browsert test, can you check in with Kris?

Kris suggested moving the functions I need into `PlacesTestUtils.jsm`, which is a good idea, although it's going to potentially be a lot of work, as I'll also need to update any tests that currently use the functions, and then everything will need to be reviewed by a Places peer. That means this patch will likely be on hold for a bit.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 9

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/48779/#review45619

> Kris suggested moving the functions I need into `PlacesTestUtils.jsm`, which is a good idea, although it's going to potentially be a lot of work, as I'll also need to update any tests that currently use the functions, and then everything will need to be reviewed by a Places peer. That means this patch will likely be on hold for a bit.

This is now fixed in this patch, but it won't work until bug 1267517 lands, and I'm not sure when that will be. That bug blocks the bug for this, so we will be notified when it lands at which point you can do the next review of this.

Comment 10

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/48779/#review45879

::: browser/components/extensions/ext-history.js:6
(Diff revision 3)
>  /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");

There's no point in using a lazy getter if you're just going to reference the symbol that you're importing on the next line.

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

https://reviewboard.mozilla.org/r/48779/#review45891

::: browser/components/extensions/ext-history.js:6
(Diff revision 3)
>  /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set sts=2 sw=2 et tw=80: */
>  "use strict";
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");

I was expecting something like: (this is off the top of my head, untested)
```
defineLazyGetter(this, "History", () => {
  Cu.import("resource://gre/modules/PlacesUtils.jsm");
  return PlacesUtils.history;
});
```

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/48779/#review45891

> I was expecting something like: (this is off the top of my head, untested)
> ```
> defineLazyGetter(this, "History", () => {
>   Cu.import("resource://gre/modules/PlacesUtils.jsm");
>   return PlacesUtils.history;
> });
> ```

By the time this script is loaded, PlacesUtils.jsm will already have been loaded, so it's probably not worth the trouble.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 14

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/48779/#review46131

::: browser/components/extensions/test/browser/browser_ext_history.js:48
(Diff revision 4)
> +  yield extension.startup();
> +  yield PlacesTestUtils.clearHistory();
> +  yield extension.awaitMessage("ready");
> +
> +  yield PlacesTestUtils.addVisits(TEST_URL);
> +  ok(yield PlacesTestUtils.isPageInDB(TEST_URL), `${TEST_URL} found in history database`);

Note that this is still dependent on bug 1267517 landing, although that should now happen within the next day or two.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

https://reviewboard.mozilla.org/r/48779/#review46173

looks good, comments are minor

::: browser/components/extensions/ext-history.js:17
(Diff revision 4)
>  extensions.registerSchemaAPI("history", "history", (extension, context) => {
>    return {
> -    history: {},
> +    history: {
> +      deleteUrl: function(details) {
> +        let url = details.url;
> +        return History.remove(url).then(() => {

This can be shortened a bit to `.then(() => undefined)`
A comment explaining that you're stripping the result of History.remove() would be nice (assuming that is why the .then exists?)

::: browser/components/extensions/test/browser/browser_ext_history.js:23
(Diff revision 4)
>    yield extension.awaitFinish("history-schema");
>    yield extension.unload();
>  });
> +
> +add_task(function* test_delete_url() {
> +  const TEST_URL = "http://example.com/" + Math.random();

nitpick: I think a template string would be nicer.

::: browser/components/extensions/test/browser/head.js:16
(Diff revision 4)
>   */
>  
>  var {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm");
>  var {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",

Is there a good reason to put this in head.js rather than just in browser_ext_history.js ?

Comment 16

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 17

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/48779/#review46173

> Is there a good reason to put this in head.js rather than just in browser_ext_history.js ?

I originally thought I might have multiple test files for the history tests, like we have for `tabs` and `windows`, but I think one file will suffice, so I've moved this into the file.

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8745035 [details]
MozReview Request: Bug 1265837 - Implement browser.history.deleteUrl, r?aswan

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

Comment 19

[Bob Silverberg [:bsilverberg]]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/094e48395a9d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/094e48395a9d

Comment 22

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/history/deleteUrl