Closed Bug 1296401 Opened 3 years ago Closed 3 years ago

Implement native changes to support simple chrome.bookmarks events

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: bsilverberg, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

This bug is for the native changes to support the completion of Bug #1221764
I notice that you have this bug assigned, would you mind if someone else took it on?
Flags: needinfo?(evilpies)
Sorry, I was away, bu I don't mind.
Flags: needinfo?(evilpies)
I've unbitrotted the patch and updated tests for nsNavBookmarkObserver to test the changes.
Assignee: evilpies → mixedpuppy
Attachment #8797324 - Flags: review?(markh) → review?(mak77)
Comment on attachment 8797324 [details]
Bug 1296401 - Implement simple chrome.bookmarks events,

https://reviewboard.mozilla.org/r/82924/#review82480

nothing blocking, the test was already horrible to follow, and this doesn't improve it :(

::: toolkit/components/places/nsNavBookmarks.cpp:477
(Diff revision 1)
> +}
> +bool SkipDescendants(nsCOMPtr<nsINavBookmarkObserver> obs) {
> +  bool skipDescendantsOnItemRemoval = false;
> +  (void) obs->GetSkipTags(&skipDescendantsOnItemRemoval);
> +  return skipDescendantsOnItemRemoval;
> +}

If these are only used in this file, it may be nice to move them into the anonymous namespace at the top.

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js:10
(Diff revision 1)
>  
>  var gBookmarksObserver = {
>    expected: [],
> +  setup(expected) {
> +    this.expected = expected;
> +		this.promise = new Promise(function(resolve, reject) {

please fix indentation.
Off-hand it may be simpler to do:
this.deferred = PromiseUtils.defer();
return this.deferred.promise;
and later call this.deferred.resolve();

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js:13
(Diff revision 1)
> +  setup(expected) {
> +    this.expected = expected;
> +		this.promise = new Promise(function(resolve, reject) {
> +			this.resolve = resolve;
> +			this.reject = reject;
> +		}.bind(this));

use an arrow function?

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js:66
(Diff revision 1)
> +  skipDescendantsOnItemRemoval: true,
> +
> +  expected: null,
> +  setup(expected) {
> +    this.expected = expected;
> +		this.promise = new Promise(function(resolve, reject) {

ditto

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js:117
(Diff revision 1)
> -    { name: "onEndUpdateBatch",
> +      { name: "onEndUpdateBatch",
> -     args: [] },
> +       args: [] },
> -  ];
> +    ]),
> +    gBookmarkSkipObserver.setup([
> +      "onBeginUpdateBatch", "onEndUpdateBatch"
> +  ])]).then(run_next_test);

fwiw, the test may be cleaner if it'd use add_task instead of add_test and wouldn't use run_next_test at all...
But I'm also fine if we do that in a followup (in such a case please file it)

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js:570
(Diff revision 1)
> +                                     TITLE,
> +                                     PlacesUtils.bookmarks.DEFAULT_INDEX);
> +  PlacesUtils.bookmarks.insertBookmark(folder,
> +                                       uri, PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                       BMTITLE);
> +

just in case, could we go another level down?
So:
folder1
  bookmark1
  folder2
    bookmark2
Attachment #8797324 - Flags: review?(mak77) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/833e5d3be485
Implement simple chrome.bookmarks events, r=mak
https://hg.mozilla.org/mozilla-central/rev/833e5d3be485
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.