Closed Bug 1265837 Opened 4 years ago Closed 4 years ago

Implement browser.history.deleteUrl

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [history]triaged)

Attachments

(1 file)

Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
No longer depends on: 1265836
Blocks: 1265838
No longer blocks: 1265838
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.
Attachment #8745035 - Flags: review?(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?
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/`?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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/
Attachment #8745035 - Flags: review?(aswan)
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.
Depends on: 1267517
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/
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.
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 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;
});
```
Attachment #8745035 - Flags: review?(aswan)
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 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/
Attachment #8745035 - Flags: review?(aswan)
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 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 ?
Attachment #8745035 - Flags: review?(aswan) → review+
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/
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 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/
Blocks: 1265842
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/094e48395a9d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Keywords: dev-doc-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.