Closed
Bug 1265842
Opened 9 years ago
Closed 9 years ago
Implement browser.history.deleteAll and deleteRange
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [history]triaged)
Attachments
(1 file)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 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 | ||
Comment 3•9 years ago
|
||
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•9 years ago
|
||
This implements `history.deleteRange` and `history.deleteAll`, but it depends on bug 1267517 and bug 1265837.
Depends on: 1265837
Comment 5•9 years ago
|
||
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•9 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•9 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•9 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•9 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 10•9 years ago
|
||
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 | ||
Comment 12•9 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•9 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•9 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 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 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
Comment 18•9 years ago
|
||
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.
Comment 19•9 years ago
|
||
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•9 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 ;)
Comment 21•9 years ago
|
||
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•9 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•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
Keywords: checkin-needed
Comment 24•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 25•8 years ago
|
||
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•