Closed Bug 1129498 Opened 5 years ago Closed 5 years ago

Update remove_all_bookmarks() to use Bookmarks.jsm

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

remove_all_bookmarks() in toolkit/components/places/tests/head_common.js has to be updated to use Bookmarks.jsm.

In most cases it's used just to cleanup roots, but that's done by eraseEverything.

In other cases though, like in test_leftpane_corruption_handling.js, it's really needed to remove also other roots (like the left pane folder), we can do that with the new API, by fetching children of the root that are not the main roots and removing them.
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment on attachment 8594749 [details] [diff] [review]
0001-Bug-1129498-Replace-remove_all_bookmarks-with-.erase.patch

Review of attachment 8594749 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/tests/unit/test_leftpane_corruption_handling.js
@@ +9,5 @@
>   */
>  
>  function run_test() {
> +  run_next_test();
> +}

This is no more needed on trunk, you can completely remove run_test

::: toolkit/components/places/tests/unit/test_update_frecency_after_delete.js
@@ +139,5 @@
> +  yield PlacesTestUtils.promiseAsyncUpdates();
> +  do_print("Bookmarked => frecency of URI should be != 0");
> +  do_check_neq(frecencyForUrl(TEST_URI), 0);
> +
> +  PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.unfiledBookmarksFolderId);

let's create bm2 inside a folder, and remove that folder. the test outcome should be the same, basically only one bookmark out of 2 is removed.
So we don't need removeFolderChildren here

@@ +156,2 @@
>    run_next_test();
>  }

can remove

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js
@@ +184,1 @@
>  }

can remove
Attachment #8594749 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/d8f98759d531
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.