Implement browser.history.deleteAll and deleteRange

RESOLVED FIXED in Firefox 49

Status

RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

({dev-doc-complete})

unspecified
mozilla49
dev-doc-complete
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 3

3 years ago
Created attachment 8746635 [details]
MozReview Request: Bug 1265842 - Implement browser.history.deleteAll and deleteRange, r?aswan

Review commit: https://reviewboard.mozilla.org/r/49505/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49505/
Attachment #8746635 - Flags: review?(aswan)
(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
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

3 years ago
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete

Updated

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