Implement browser.history.deleteAll and deleteRange

RESOLVED FIXED in Firefox 49

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

({dev-doc-complete})

unspecified
mozilla49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [history]triaged)

Attachments

(1 attachment)

Assignee

Updated

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

Updated

3 years ago
Blocks: 1265845
Assignee

Updated

3 years ago
No longer blocks: 1265845
Assignee

Updated

3 years ago
Duplicate of this bug: 1265838
Assignee

Comment 2

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

Updated

3 years ago
Depends on: 1267517
Assignee

Comment 4

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Updated

3 years ago
Duplicate of this bug: 1269302
Assignee

Comment 12

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

Comment 13

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

Comment 14

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

Comment 17

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

Comment 20

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

Comment 22

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

Updated

3 years ago
Keywords: checkin-needed

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e02797aae3ce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Updated

3 years ago
Keywords: dev-doc-needed

Updated

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