Closed
Bug 1296401
Opened 8 years ago
Closed 8 years ago
Implement native changes to support simple chrome.bookmarks events
Categories
(Toolkit :: Places, defect, P3)
Toolkit
Places
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
mak
:
review+
|
Details |
This bug is for the native changes to support the completion of Bug #1221764
Comment 1•8 years ago
|
||
I notice that you have this bug assigned, would you mind if someone else took it on?
Flags: needinfo?(evilpies)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
I've unbitrotted the patch and updated tests for nsNavBookmarkObserver to test the changes.
Assignee: evilpies → mixedpuppy
Assignee | ||
Updated•8 years ago
|
Attachment #8797324 -
Flags: review?(markh) → review?(mak77)
Comment 5•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/833e5d3be485 Implement simple chrome.bookmarks events, r=mak
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/833e5d3be485
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•