1296401 - Implement native changes to support simple chrome.bookmarks events

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Bob Silverberg [:bsilverberg]]

Attachment: Patch with native changes

This bug is for the native changes to support the completion of Bug #1221764

Comment 1

[Andy McKay]

I notice that you have this bug assigned, would you mind if someone else took it on?

Comment 2

[Tom S [:evilpie]]

Sorry, I was away, bu I don't mind.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1296401 - Implement simple chrome.bookmarks events,

Review commit: https://reviewboard.mozilla.org/r/82924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82924/

Comment 4

[Shane Caraveo (:mixedpuppy)]

I've unbitrotted the patch and updated tests for nsNavBookmarkObserver to test the changes.

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8797324 [details]
Bug 1296401 - Implement simple chrome.bookmarks events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82924/diff/1-2/

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8797324 [details]
Bug 1296401 - Implement simple chrome.bookmarks events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82924/diff/2-3/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8797324 [details]
Bug 1296401 - Implement simple chrome.bookmarks events,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82924/diff/3-4/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/833e5d3be485
Implement simple chrome.bookmarks events, r=mak

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/833e5d3be485