Closed Bug 1265842 Opened 8 years ago Closed 8 years ago

Implement browser.history.deleteAll and deleteRange

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Iteration:
49.1 - May 9
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

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

Attachments

(1 file)

MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan
58 bytes, text/x-review-board-request
aswan
: review+
Details
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
No longer depends on: 1265838
Blocks: 1265845
No longer blocks: 1265845
This will also include implementing deleteRange, as per https://developer.chrome.com/extensions/history#method-deleteRange.
Iteration: --- → 49.1 - May 9
Summary: Implement browser.history.deleteAll → Implement browser.history.deleteAll and deleteRange
Depends on: 1267517
This implements `history.deleteRange` and `history.deleteAll`, but it depends on bug 1267517 and bug 1265837.
Depends on: 1265837
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

https://reviewboard.mozilla.org/r/49505/#review46343

::: browser/components/extensions/test/browser/browser_ext_history.js:34
(Diff revision 1)
> +        browser.history.deleteUrl({url: arg}).then(result => {
> -        browser.test.assertEq(undefined, result, "browser.history.deleteUrl returns nothing");
> +          browser.test.assertEq(undefined, result, "browser.history.deleteUrl returns nothing");
> -        browser.test.sendMessage("url-deleted");
> +          browser.test.sendMessage("url-deleted");
> -      });
> +        });
> +      } else {
> +        if (msg === "delete-range") {

combine this with the line above in a single "else if" instead of nesting the if.

::: browser/components/extensions/test/browser/browser_ext_history.js:39
(Diff revision 1)
> +        if (msg === "delete-range") {
> +          browser.history.deleteRange(arg).then(result => {
> +            browser.test.assertEq(undefined, result, "browser.history.deleteUrl returns nothing");
> +            browser.test.sendMessage("range-deleted");
> +          });
> +        } else {

add a test for `msg == "delete-all"` for clarity

::: browser/components/extensions/test/browser/browser_ext_history.js:51
(Diff revision 1)
>      });
>  
>      browser.test.sendMessage("ready");
>    }
>  
> +  const REFEENCE_DATE = new Date(1999, 9, 9, 9, 9);

s/REFEENCE/REFERENCE/ ?

::: browser/components/extensions/test/browser/browser_ext_history.js:71
(Diff revision 1)
> -  ok(yield PlacesTestUtils.isPageInDB(TEST_URL), `${TEST_URL} found in history database`);
> +
> +  // Add 5 visits for one uri and 3 visits for 3 others
> +  for (let i = 0; i < 8; ++i) {
> +    let uri = "http://mozilla.com/test_history/";
> +    if (i > 4) {
> +      uri = `${uri}${i}/${Math.random()}`;

At first glance, this looked to be an instance of:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Avoiding_intermittent_oranges#Tests_that_rely_on_Math.random().C2.A0to_create_unique_values

However, you're actually appending to the uri each time so you will get unique uris.  But you'll get some pretty unwieldy uris that seem like they would be inconvenient to wade through in the event this test fails at some point.  can you make them more uniform and simple?

::: browser/components/extensions/test/browser/browser_ext_history.js:80
(Diff revision 1)
> +
> +    let visit = {
> +      uri,
> +      title: "visit " + i,
> +      visitDate: dbDate,
> +      test: {

I'm assuming this is ignored or at least not meaningful to the places database?  could you rewrite the code below to just reference visitDate rather than keeping two copies of each timestamp in different formats?

::: browser/components/extensions/test/browser/browser_ext_history.js:106
(Diff revision 1)
> +  };
> +
> +  extension.sendMessage("delete-range", filter);
> +  yield extension.awaitMessage("range-deleted");
> +
> +  ok(yield PlacesTestUtils.isPageInDB(visits[0].uri), "expected uri found in history database");

Maybe this is a problem with the documentation but as I read this, you're explicitly testing for behavior different from what is documented here:
https://developer.chrome.com/extensions/history#method-deleteRange
which says "Pages will not be removed from the history unless all visits fall within the range."

::: browser/components/extensions/test/browser/browser_ext_history.js:122
(Diff revision 1)
> +  ok(!(yield PlacesTestUtils.isPageInDB(visits[0].uri)), "expected uri not found in history database");
> +  is(yield PlacesTestUtils.visitsInDB(visits[0].uri), 0, "0 visits for uri found in history database");
> +  ok(!(yield PlacesTestUtils.isPageInDB(visits[5].uri)), "expected uri not found in history database");
> +  is(yield PlacesTestUtils.visitsInDB(visits[5].uri), 0, "0 visits for uri found in history database");
> +
> +  ok(yield PlacesTestUtils.isPageInDB(visits[7].uri), "expected uri found in history database");

I'm curious about the logic for what you decide to check when.  You never refer to visits[6] for instance?

::: browser/components/extensions/test/browser/browser_ext_history.js:126
(Diff revision 1)
> +
> +  ok(yield PlacesTestUtils.isPageInDB(visits[7].uri), "expected uri found in history database");
> +
> +  extension.sendMessage("delete-all");
> +  yield extension.awaitMessage("urls-deleted");
> +  ok(!PlacesUtils.history.hasHistoryEntries, "history is empty");

can you write this as `is(hasHistoryEntries, false, ..)`
Attachment #8746635 - Flags: review?(aswan)
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49505/diff/1-2/
Attachment #8746635 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/49505/#review46343

> Maybe this is a problem with the documentation but as I read this, you're explicitly testing for behavior different from what is documented here:
> https://developer.chrome.com/extensions/history#method-deleteRange
> which says "Pages will not be removed from the history unless all visits fall within the range."

No, I think I am testing for that exact behaviour. This line checks that the url used with visit[0] *does* still exist in the history. At this point in the test we have only deleted 3 of the 5 visits associated with that url, so the url should still exist.

> I'm curious about the logic for what you decide to check when.  You never refer to visits[6] for instance?

I can add some comments to the code to describe what's happening at different stages, if you think that would be an improvement. `visits[6]` is used for the test of `deleteUrl` (which is the first thing we test). First we check that it exists in the database (line 92 of this diff), then we call `deleteUrl` passing that url in (line 94), and then we check that it no longer exists in the database (line 96). `visits[7]` is added so that there will be something left to delete when we test `deleteAll` at the end of the test.
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

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

::: browser/components/extensions/test/browser/browser_ext_history.js:8
(Diff revision 3)
>  "use strict";
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
>                                    "resource://testing-common/PlacesTestUtils.jsm");
>  
> -add_task(function* test_history_schema() {
> +add_task(function* test_delete() {

I realized that now that we're exercising the API this original test for the schema is pointless, so I've removed it in this patch.
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

https://reviewboard.mozilla.org/r/49505/#review46519

::: browser/components/extensions/test/browser/browser_ext_history.js:52
(Diff revisions 1 - 3)
>  
>    // Add 5 visits for one uri and 3 visits for 3 others
>    for (let i = 0; i < 8; ++i) {
>      let uri = "http://mozilla.com/test_history/";
>      if (i > 4) {
> -      uri = `${uri}${i}/${Math.random()}`;
> +      uri = `${uri}${i}/`;

it looks like this is still concatenating so for instance your last url is:
http://mozilla.org/test_history/5/6/7/
not a big deal but was that your intent?

::: browser/components/extensions/test/browser/browser_ext_history.js:54
(Diff revisions 1 - 3)
>    for (let i = 0; i < 8; ++i) {
>      let uri = "http://mozilla.com/test_history/";
>      if (i > 4) {
> -      uri = `${uri}${i}/${Math.random()}`;
> +      uri = `${uri}${i}/`;
>      }
> -    let jsDate = new Date(Number(REFEENCE_DATE) + 3600 * 1000 * i);
> +    let jsDate = new Date(Number(REFERENCE_DATE) + 3600 * 1000 * i);

At this point, I don't think you really need a Date object here?  You're working entirely with numbers...
Attachment #8746635 - Flags: review?(aswan) → review+
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

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

> it looks like this is still concatenating so for instance your last url is:
> http://mozilla.org/test_history/5/6/7/
> not a big deal but was that your intent?

Neither intentional nor a problem, but fixed anyway, as it wasn't intentional.
https://reviewboard.mozilla.org/r/49505/#review46343

> No, I think I am testing for that exact behaviour. This line checks that the url used with visit[0] *does* still exist in the history. At this point in the test we have only deleted 3 of the 5 visits associated with that url, so the url should still exist.

You didn't reply to this. Is it okay for me to drop it? In the future should I just assume a non-response means I can drop the issue?

> I can add some comments to the code to describe what's happening at different stages, if you think that would be an improvement. `visits[6]` is used for the test of `deleteUrl` (which is the first thing we test). First we check that it exists in the database (line 92 of this diff), then we call `deleteUrl` passing that url in (line 94), and then we check that it no longer exists in the database (line 96). `visits[7]` is added so that there will be something left to delete when we test `deleteAll` at the end of the test.

Same as above. I was awaiting a response from you to my response.
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49505/diff/4-5/
Attachment #8746635 - Attachment description: MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, f?aswan → MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan
https://reviewboard.mozilla.org/r/49505/#review46343

> You didn't reply to this. Is it okay for me to drop it? In the future should I just assume a non-response means I can drop the issue?

Okay, I guess the distinction between removing "pages" and removing "visits" won't really become apparent until search() gets implemented...

> Same as above. I was awaiting a response from you to my response.

Ah.  That confused my linear thinking :-)
This is one of those cases where its hard to know how thoroughly to test in extension code since all the corner cases for the history module are presumably tested in depth elsewhere.  But what you have here seems good.
https://reviewboard.mozilla.org/r/49505/#review46787

damn it reviewboard.  the usual "i didn't press all the right buttons" excuse, sorry about that...
https://reviewboard.mozilla.org/r/49505/#review46343

> Okay, I guess the distinction between removing "pages" and removing "visits" won't really become apparent until search() gets implemented...

`deleteRange` deletes visits, and unless all visits for a page are removed, the page will still exist in the history database. `deleteUrl` removes a page, which means that all visits associated with that page will also be removed. `deleteAll` deletes everything ;)
https://reviewboard.mozilla.org/r/49505/#review46343

> `deleteRange` deletes visits, and unless all visits for a page are removed, the page will still exist in the history database. `deleteUrl` removes a page, which means that all visits associated with that page will also be removed. `deleteAll` deletes everything ;)

This is getting rather academic now but I understand what the `delete*` methods do, I was trying to say that the notion of a "Page" being removed from history will become more clear when `search()` is done since its results are organized around pages rather than individual visits.
Comment on attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49505/diff/5-6/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e02797aae3ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: