The current notifications system is really inefficient: 1. There are separate observers for each component, it should be simpler to just register a listener for places and specific events 2. each observer must handle ALL the notifications even if it only cares about some of them. Thus everytime we must send all the notifications with a lot of overhead. 3. Every change is notified one by one, rather than in a batch. First, it would be great to have a single point to register/unregister observers, likely the history service is the best point atm. PlacesUtils.add/removeListener could alias and provide a wrapper. Second, the listener should receive an array of changes (that may even just contain 1 change), so that it can pick the best batching strategy to update itself. The Places APIs should try to batch notifications for large changes. There are different possible approaches: a. register for single events, like PlacesUtils.addListener("bookmark-added", mylistener, weak); b. register for multiple events like PlacesUtils.addListener(["visit-added", "title-changed"], mylistener, weak); The difference may be important for batching, for example removing 10 bookmarks may send 10 "bookmark-removed" and 10 "page-removed" notifications, if a consumer cares about both, in case a) it would get 2 notifications with 10 changes, in cases b) or c) it would get 20 notifications (maybe even intermixed).

Marco, is there anything I can do to help with this? If you have a finalized idea of what the APIs should look like I can get started on implementing. If you'd like to wait and explore it yourself I understand, but I think bug 1426245 is the next step for acceptable migration performance, so if there's anything I can do to speed this up I'd be happy to.

We don't have a complete plan yet, pretty much what is at comment 0, plus the idea to try to reduce xpconnect, that may mean having a dispatcher in Cpp and one in Js communicating and registering listeners in the right dispatcher (I'd still like to have a single registration point and then the listeners could be passed to the dispatchers I guess). Starting with an architectural draft (on google docs) would be a good first step and looking at the draft it should be possible to check current listeners and see if they fit. So if you want to try to apply a fresh look at the above suggestions and look into that, it would be useful, I clearly have some bias from years of touching this ugly code :)

What are we thinking for type safety? I'm feeling like the best policy, since most of the consumers are JS anyway, is going to be to just make it use jsvals for everything. Since each event type is going to have a different shape of payload, I can't think of an elegant and efficient way to get any type safety on the C++ side of things, and jsvals should be fast enough anyway. Thoughts?

We should look at an API proposal to have a full answer. For things like bookmarks, I'd like the API to pass out a bookmark object as defined by Bookmarks.jsm instead of single properties. This sort-of requires most consumers to be js indeed. An nsIBookmarkObject would also be a possibility in alternative to jsval that would work with cpp... To put it into perspective, history additions still notify from cpp, and nsnavhistoryresult still observes from cpp, though nothing would disallow us from designing a js only collector/dispatcher and a couple ad-hoc methods to exchange info with the few cpp consumers. A list of cpp consumers and exchanged info may be useful (most of nsnavbookmarks.cpp is going away very soon in 60). There was also a discussion ongoing regarding a rewrite of the observers service in dev-platform that may be partially overlapping, instead of a notification name we could use an enum for example, reducing strings costs.

Just a heads up that I've started to work on a doc for this. Mostly it's just a rough of what I think the API should look like and documentation of publishers/subscribers. Still a WIP, but feel free to chime in if you think it's off track. https://docs.google.com/document/d/1G45vfd6RXFXwNz7i4FV40lDCU0ao-JX_bZdgJV4tLjk/edit?usp=sharing

Update: I think the doc is in a decent spot now for a more critical look.

I left some comments, there's a lot to consider, but I'm happy this is a very good start documentation also of the current consumers.

Made some edits and proof-of-concepted the API implementation. If it all looks in good-enough shape to you Marco I'd like to try to prove out the performance of the system by migrating onVisits and testing it for perf regressions (I'm particularly interested in the cost of consuming the jsvals from C++).

Quick update on this: there seems to be no performance penalty in C++ from the use of jsvals, which is nice. Going to try to clean up the patch for all of this today and make sure all the consumers are working properly.

Requested Ehsan on the interface definition / implementation since it would need a DOM peer, but I would appreciate it if you took a look at that as well, Marco (though it's fairly close to what was in the doc).

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review226306 Code analysis found 8 defects in this patch: - 8 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/modules/engines/history.js:440 (Diff revision 1) > > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:441 (Diff revision 1) > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["visit-added"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:448 (Diff revision 1) > > onStop() { > this._log.info("Removing Places observer."); > PlacesUtils.history.removeObserver(this); > + if (this._placesObserver) { > + PlacesObservers.removeListener(["visit-added"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/PlacesUtils.jsm:321 (Diff revision 1) > TOPIC_VACUUM_STARTING: "places-vacuum-starting", > TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin", > TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success", > TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed", > > + observers: PlacesObservers, Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:18 (Diff revision 1) > > XPCOMUtils.defineLazyGetter(this, "history", function() { > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:20 (Diff revision 1) > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( > + livemarks.handlePlacesEvents.bind(livemarks)); > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:642 (Diff revision 1) > * Must be called before the provider is used. > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:643 (Diff revision 1) > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["visit-added"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef]

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review226308 Code analysis found 31 defects in this patch (only the first 30 are reported here): - 31 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/tests/unit/head_helpers.js:572 (Diff revision 1) > async function promiseVisit(expectedType, expectedURI) { > return new Promise(resolve => { > function done(type, uri) { > - if (uri.equals(expectedURI) && type == expectedType) { > + if (uri == expectedURI.spec && type == expectedType) { > PlacesUtils.history.removeObserver(observer); > + PlacesObservers.removeListener(["visit-added"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/head_helpers.js:596 (Diff revision 1) > onClearHistory() {}, > onPageChanged() {}, > onDeleteVisits() {}, > }; > PlacesUtils.history.addObserver(observer, false); > + PlacesObservers.addListener(["visit-added"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:16 (Diff revision 1) > const TIMESTAMP2 = (Date.now() - 6592903) * 1000; > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > + let listener = new PlacesWeakCallbackWrapper((events) => { Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:17 (Diff revision 1) > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > - onBeginUpdateBatch: function onBeginUpdateBatch() {}, > + let listener = new PlacesWeakCallbackWrapper((events) => { > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:20 (Diff revision 1) > - Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS, > - Ci.nsISupportsWeakReference > - ]) > - }, true); > - }); > + }); > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/jsdownloads/test/unit/head.js:154 (Diff revision 1) > - > - let uri = NetUtil.newURI(aUrl); > - > - PlacesUtils.history.addObserver({ > - QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]), > - onBeginUpdateBatch() {}, > + function listener(aEvents) { > + Assert.equal(aEvents.length, 1); > + Assert.equal(aEvents[0].type, "visit-added"); > + let data = aEvents[0].data; > + if (data.uri == aUrl) { > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/jsdownloads/test/unit/head.js:158 (Diff revision 1) > - if (visitUri.equals(uri)) { > - PlacesUtils.history.removeObserver(this); > - resolve([time, transitionType]); > - } > + } > - }, > - onTitleChanged() {}, > + } > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:325 (Diff revision 1) > waitForNotification(notification, conditionFn = () => true, type = "bookmarks") { > + if (type == "places") { > + return new Promise(resolve => { > + function listener(events) { > + if (conditionFn(events.map(e => e.data))) { > + PlacesObservers.removeListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:329 (Diff revision 1) > + if (conditionFn(events.map(e => e.data))) { > + PlacesObservers.removeListener([notification], listener); > + resolve(); > + } > + } > + PlacesObservers.addListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:33 (Diff revision 1) > > async function promiseLoadedThreeTimes(uri) { > - historyObserver.count = 0; > - historyObserver.expectedURI = Services.io.newURI(uri); > + count = 0; > + expectedURI = uri; > let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["visit-added"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:38 (Diff revision 1) > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["visit-added"], onVisitsListener); > gBrowser.loadURI(uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > - PlacesUtils.history.removeObserver(historyObserver); > + PlacesObservers.removeListener(["visit-added"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:25 (Diff revision 1) > - if (!uri.equals(FINAL_URI)) { > + if (uri != FINAL_URI.spec) { > return; > } > > is(this._notified.length, 4); > - PlacesUtils.history.removeObserver(this); > + PlacesObservers.removeListener(["visit-added"], this.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:61 (Diff revision 1) > - } = visits[0]; > - this.onVisit(uri, visitId, time, referrerId, transitionType); > + } = events[0].data; > + this.onVisit(uri, visitId, visitDate, referrerId, transitionType); > } > + }; > + observer.handleEvents = observer.handleEvents.bind(observer); > + PlacesObservers.addListener(["visit-added"], observer.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:17 (Diff revision 1) > let visitedPromise = new Promise(resolve => { > - let historyObserver = { > - onVisit(aURI, aVisitID, aTime, aReferringID, aTransitionType) { > - PlacesUtils.history.removeObserver(historyObserver); > - info("Received onVisit: " + aURI.spec); > - fieldForUrl(aURI, "frecency", function(aFrecency) { > + function listener(aEvents) { > + is(aEvents.length, 1, "Right number of visits notified"); > + is(aEvents[0].type, "visit-added"); > + let uri = NetUtil.newURI(aEvents[0].data.uri); > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:30 (Diff revision 1) > - resolve(); > + resolve(); > - }); > + }); > - }); > + }); > - }); > + }); > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:17 (Diff revision 1) > - } = aVisits[0]; > - info("onVisits: " + uri.spec); > - if (uri.equals(EXPECTED_URL)) { > + } = aEvents[0].data; > + info("'visit-added': " + uri); > + if (uri == EXPECTED_URL.spec) { > - Assert.equal(lastKnownTitle, null, "Should not have a title"); > + Assert.equal(lastKnownTitle, null, "Should not have a title"); > - } > + } > - }, > + PlacesObservers.removeListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:29 (Diff revision 1) > resolve(); > } > }, > }; > PlacesUtils.history.addObserver(obs); > + PlacesObservers.addListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:24 (Diff revision 1) > // Used to verify errors are not marked as typed. > PlacesUtils.history.markPageAsTyped(NetUtil.newURI(TEST_URL)); > > let promiseVisit = new Promise(resolve => { > - let historyObserver = { > - __proto__: NavHistoryObserver.prototype, > + function onVisits(events) { > + PlacesObservers.removeListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:30 (Diff revision 1) > - PlacesUtils.history.removeObserver(historyObserver); > - is(visits.length, 1, "Right number of visits"); > + is(events[0].type, "visit-added"); > + is(events[0].data.uri, TEST_URL, "Check visited url"); > - is(visits[0].uri.spec, TEST_URL, "Check visited url"); > - resolve(); > + resolve(); > - } > + } > - }; > + PlacesObservers.addListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:88 (Diff revision 1) > - aCallback) { > + aCallback) { > - this.uri = aURI; > + this.uri = aURI; > - this.guid = aGUID; > + this.guid = aGUID; > - this.callback = aCallback; > + this.callback = aCallback; > + this.handlePlacesEvent = this.handlePlacesEvent.bind(this); > + PlacesObservers.addListener(["visit-added"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:117 (Diff revision 1) > - if (!this.uri.equals(uri) || this.guid != guid) { > + if (this.uri.spec != uri || this.guid != guid) { > return; > } > - this.callback(time, transitionType, lastKnownTitle); > - }, > -}; > + this.callback(visitDate, transitionType, lastKnownTitle); > + > + PlacesObservers.removeListener(["visit-added"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:988 (Diff revision 1) > - PlacesUtils.history.addObserver({ > - onVisits(visits) { > - Assert.equal(visits.length, 1, "Should only get notified for one visit."); > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + function onVisits(events) { > + Assert.equal(events.length, 1, "Should only get notified for one visit."); > + Assert.equal(events[0].type, "visit-added"); > + let {uri} = events[0].data; > + Assert.equal(uri, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:991 (Diff revision 1) > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + let {uri} = events[0].data; > + Assert.equal(uri, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["visit-added"], onVisits); > - resolve(); > + resolve(); > - } > + } > + PlacesObservers.addListener(["visit-added"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:38 (Diff revision 1) > - hidden, > + hidden, > - visitCount, > + visitCount, > - typed, > + typed, > - lastKnownTitle, > + lastKnownTitle, > - } = aVisits[0]; > - PlacesUtils.history.removeObserver(this); > + } = aEvents[0].data; > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:44 (Diff revision 1) > - aCallback(uri, visitId, time, 0, referrerId, > + let uriArg = NetUtil.newURI(uri); > + aCallback(uriArg, id, visitDate, 0, referrerId, > - transitionType, guid, hidden, visitCount, > + transitionType, guid, hidden, visitCount, > - typed, lastKnownTitle); > + typed, lastKnownTitle); > - } > + } > - }; > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:45 (Diff revision 1) > + * which resolves a promise on being called. > + */ > +function promiseVisitAdded(callback) { > + return new Promise(resolve => { > + function listener(events) { > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:51 (Diff revision 1) > + Assert.equal(events.length, 1, "Right number of visits notified"); > + Assert.equal(events[0].type, "visit-added"); > + callback(events[0].data); > + resolve(); > + } > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:140 (Diff revision 1) > - Assert.equal(visit.referrerId, 0); > + Assert.equal(visit.referrerId, 0); > - Assert.equal(visit.transitionType, TRANSITION_TYPED); > + Assert.equal(visit.transitionType, TRANSITION_TYPED); > - Assert.equal(visit.visitCount, 3); > + Assert.equal(visit.visitCount, 3); > - Assert.equal(visit.typed, 1); > + Assert.equal(visit.typed, 1); > > - PlacesUtils.history.removeObserver(observer, false); > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:146 (Diff revision 1) > - resolve(); > + resolve(); > - break; > + break; > - } > + } > - } > + } > - }, > - }; > + } > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:27 (Diff revision 1) > - Assert.equal(aTransitionType, gVisits[this._visitCount].transition); > - this._visitCount++; > + Assert.equal(data.transitionType, gVisits[visitCount].transition); > + visitCount++; > > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:30 (Diff revision 1) > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["visit-added"], listener); > - } > + } > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["visit-added"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef]

(In reply to Code Review Bot [:reviewbot] from comment #15) > Error: 'placesobservers' is not defined. [eslint: no-undef] Woops - that's annoying. These are all addressed in the linter changes in commit 4.

Marking as P2 to reflect the current work status - hoping to land at least a large part of the redesign in 61.

Doug sorry for the delay, lots of stuff ongoing. I'll look at this asap, likely these days. There's one thing that may be worth brainstorming, to check how it fits in the current design, that came out yesterday, about batching. Basically we have some cases like the History Sidebar where pages are grouped, for example by Host. So we have mozilla.org ├─ mozilla.org/something ├─ mozilla.org/something_else ... other hundreds entries and the user removes the "mozilla.org" container. That ends up invoking RemovePagesByFilter. The problem for notifications is that the "mozilla.org" container should be able to remove contained urls one by one (for which it needs the normal notification), but it should also be able to tell "all the urls in this domain are going away" and avoid removing entries one by one in that case. I was thinking we could maybe add another dedicated notification to fire BEFORE the next ones, like "host-removed", and then the result could either ignore or skips quicker the single removal notifications, since hopefully everything would come in a single array of notifications. another similar case is "we are removing all the visits between "TIME" and "TIME + N" that a grouping by date could handle more efficiently.

(In reply to Marco Bonardo [::mak] from comment #19) > I was thinking we could maybe add another dedicated notification to fire > BEFORE the next ones, like "host-removed", and then the result could either > ignore or skips quicker the single removal notifications, since hopefully > everything would come in a single array of notifications. Hmm, I don't see a problem with fitting that into the system. I can't think of a cleaner approach, either. But I am trying to figure out how the "host-removed" notification would specify what it covers. If it specifies a count, listeners could just skip over that many notifications in the batch, but it might be tricky for the publisher to accurately count that number when creating the notification, especially if the notifications that follow are of mixed type for any reason. Thoughts?

(In reply to Doug Thayer [:dthayer] from comment #20) > Hmm, I don't see a problem with fitting that into the system. I can't think > of a cleaner approach, either. But I am trying to figure out how the > "host-removed" notification would specify what it covers. If it specifies a > count, listeners could just skip over that many notifications in the batch, > but it might be tricky for the publisher to accurately count that number > when creating the notification, especially if the notifications that follow > are of mixed type for any reason. Thoughts? Right, I was thinking it may more or less work this way: 1. removePagesByHost sends an array of notifications, the first one is "host-removed" with the host as argument. Next notifications are single ones for each related change. 2. the result notices an host-removed notification, it removes the host container, then proceeds analyzing the next notifications, in most (all?) cases it won't care about them since the container has gone, and will just skip them.

this presumes related notifications are in a single array, and different arrays of notifications represent different kind of operations.

Resetting priority to what it was (team switching to whiteboard priority system.)

Looks like now there is a new dom/chrome-webidl/ folder that doesn't require dom peer review.

Yeah, I noticed that. I think I'd still like a DOM peer to take a look though, given it's my first time working in webidl. I'll move it to that folder though. In either case, do you know when you might get around to reviewing this, Marco? I know you've been pretty busy, just figured I'd check in.

Comment on attachment 8951105 [details] Bug 1340498 - Implement new Places Observers interface https://reviewboard.mozilla.org/r/220250/#review233452 Just a few question and naming suggestions, I'm honestly unable to review webidl code at this time. The main point I have that I'd like to discuss/figure out with you, is the mixing of page and visit infos in the same object, maybe we could come up with a more reusable solution for future notifications? See below. ::: dom/webidl/PlacesObservers.webidl:13 (Diff revision 1) > + "none", > + > + /** > + * data: PlacesVisit. Fired whenever a visit is added. > + */ > + "visit-added", considered it's likely in the future we'll have "page-removed", "page-history-cleared", maybe we should keep a common prefix and make this "page-visited" ::: dom/webidl/PlacesObservers.webidl:20 (Diff revision 1) > + > +dictionary PlacesVisit { > + /** > + * URI of the visit. > + */ > + ByteString uri = ""; most of the other webidls seem to use DOMString and url, rather than ByteString and uri ::: dom/webidl/PlacesObservers.webidl:25 (Diff revision 1) > + ByteString uri = ""; > + > + /** > + * Id of the visit. > + */ > + unsigned long long id = 0; visitId, for coherency with WebExt ::: dom/webidl/PlacesObservers.webidl:30 (Diff revision 1) > + unsigned long long id = 0; > + > + /** > + * Date and time of the visit. > + */ > + unsigned long long visitDate = 0; We could either keep coherency with WebExt (https://developer.chrome.com/extensions/history) and name this visitTime, or with History.jsm and name this date. The former is probably more clear. It could be worth for the same reason to also add a lastVisitTime ::: dom/webidl/PlacesObservers.webidl:35 (Diff revision 1) > + unsigned long long visitDate = 0; > + > + /** > + * The id of the visit the user came from, defaults to 0 for no referrer. > + */ > + unsigned long long referrerId = 0; let's name this referringVisitId, for coherency with WebExt ::: dom/webidl/PlacesObservers.webidl:45 (Diff revision 1) > + unsigned long transitionType = 0; > + > + /** > + * The unique id associated with the page. > + */ > + ByteString guid = ""; pageGuid, to clarify it's not the guid of the visit ::: dom/webidl/PlacesObservers.webidl:55 (Diff revision 1) > + boolean hidden = false; > + > + /** > + * Number of visits (including this one) for this URI. > + */ > + unsigned long visitCount = 0; How complex would it be to have a page entry, and inside it a visits entry (that for visit notification may contain just one)? Or have a notification send both a page and a visit object (or an array of visits objects) I'm asking mostly because this PlacesVisit object seems to mix up properties of a page (visitCount) with properties of a single visit (transitionType). Ideally we could have a reusable page ojects also for notifications like page-removed or page-history-cleared, or page-history-expired, and in that case we may not care about a single visit, or not even have any single visit info. I'm mostly trying to understand our options here, I don't know if it's actionable (sorry for my webidl ignorance, I must study it) ::: dom/webidl/PlacesObservers.webidl:62 (Diff revision 1) > + /** > + * Whether the URI has been typed or not. > + * TODO (Bug 1271801): This will become a count, rather than a boolean. > + * For future compatibility, always compare it with "> 0". > + */ > + unsigned long typed = 0; let's name it typedCount, per coherency with WebExt

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review233464 it looks mostly good, clearly the final patch depends on the idl definition. Do you have any measurements of the positive effect of this, even a made up micro benchmark would be nice. ::: browser/components/extensions/ext-history.js:103 (Diff revision 1) > this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]}); > } > - onVisits(visits) { > - for (let visit of visits) { > - let data = { > - id: visit.guid, > + handlePlacesEvents(events) { > + for (let {type, data} of events) { > + switch (type) { > + case "visit-added": { likely we don't need to pay the switch cost, we should have a Places test ensuring that if we register a single event listener, we only get that one. If in the future someone adds more events here, he can then add the switch. This is valid also for the other code points below, at least for now. ::: browser/components/extensions/ext-history.js:108 (Diff revision 1) > - id: visit.guid, > - url: visit.uri.spec, > - title: visit.lastKnownTitle || "", > - lastVisitTime: visit.time / 1000, // time from Places is microseconds, > - visitCount: visit.visitCount, > - typedCount: visit.typed, > + case "visit-added": { > + let visit = { > + id: data.guid, > + url: data.uri, > + title: data.lastKnownTitle || "", > + lastVisitTime: data.visitDate / 1000, // time from Places is microseconds, While here we should modernize the notifications API, and pass out milliseconds.

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review233498 it looks ok, but not worth going through it line by line until we have defined a "final" API

Comment on attachment 8951105 [details] Bug 1340498 - Implement new Places Observers interface https://reviewboard.mozilla.org/r/220250/#review233452 > How complex would it be to have a page entry, and inside it a visits entry (that for visit notification may contain just one)? > Or have a notification send both a page and a visit object (or an array of visits objects) > I'm asking mostly because this PlacesVisit object seems to mix up properties of a page (visitCount) with properties of a single visit (transitionType). > Ideally we could have a reusable page ojects also for notifications like page-removed or page-history-cleared, or page-history-expired, and in that case we may not care about a single visit, or not even have any single visit info. > > I'm mostly trying to understand our options here, I don't know if it's actionable (sorry for my webidl ignorance, I must study it) I think we can do all of the things you're imagining with webidl. However, I'm not sure exactly how best to structure it. Personally I favor the notifications being flat, so consumers don't have to, for example, do this: for (let event of events) { for (let visit of event.data.visits) {...} } I think, pragmatically speaking, what we're trying to avoid is the semantic confusion of having things like "visitCount" be part of a "visit" object, since it's not clear that it's referring to the number of times that page has been visited, not the number of visits that just happened. I feel like the best way to resolve that is to have a PlacesPage dictionary type nested inside the PlacesVisit dictionary (and in future dictionary types we create.) This way it's clear that those properties belong to the page that the visit references, and not to the visit itself, but we don't have to make an array of arrays and consumers don't have to iterate through both. However, a potential issue with having a reusable page object is that we'll feel compelled to fill out all of its properties, even if for the publisher that we're converting it incurs a performance hit to retrieve them. I can't find any examples quickly of where this might have a significant cost, but it's something to think about?

Marco, do you have any thoughts re: comment #29?

(In reply to Doug Thayer [:dthayer] from comment #29) > I think, pragmatically speaking, what we're trying to avoid is the semantic > confusion of having things like "visitCount" be part of a "visit" object, > since it's not clear that it's referring to the number of times that page > has been visited, not the number of visits that just happened. Right. I also agree the double for loop doesn't sound great. > However, a potential issue with having a reusable page object is that we'll > feel compelled to fill out all of its properties, even if for the publisher > that we're converting it incurs a performance hit to retrieve them. I can't > find any examples quickly of where this might have a significant cost, but > it's something to think about? Yes, it may have significant costs in certain cases, indeed even current notifications sometimes omit a few values. The current solution is just to tell in the documentation, for each notification, that certain values are not valid. It's not great but since it's all for internal use we never cared too much. I'm not sure how we could come up with something more freeform here, each notification may want a different "details" object, and incurring a perf penalty just to grab some info that nobody will use doesn't sound great. I guess we'll keep the current flat form that you suggested for now, and just define a different object per notification, hopefully we can reuse some of those.

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review237188 Code analysis found 8 defects in this patch: - 8 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/modules/engines/history.js:484 (Diff revision 2) > > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:485 (Diff revision 2) > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:492 (Diff revision 2) > > onStop() { > this._log.info("Removing Places observer."); > PlacesUtils.history.removeObserver(this); > + if (this._placesObserver) { > + PlacesObservers.removeListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/PlacesUtils.jsm:346 (Diff revision 2) > TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin", > TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success", > TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed", > > ACTION_SCHEME: "moz-action:", > + observers: PlacesObservers, Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:18 (Diff revision 2) > > XPCOMUtils.defineLazyGetter(this, "history", function() { > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:20 (Diff revision 2) > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( > + livemarks.handlePlacesEvents.bind(livemarks)); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:553 (Diff revision 2) > * Must be called before the provider is used. > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:554 (Diff revision 2) > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef]

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review237194 Code analysis found 31 defects in this patch (only the first 30 are reported here): - 31 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/tests/unit/head_helpers.js:573 (Diff revision 2) > async function promiseVisit(expectedType, expectedURI) { > return new Promise(resolve => { > function done(type, uri) { > - if (uri.equals(expectedURI) && type == expectedType) { > + if (uri == expectedURI.spec && type == expectedType) { > PlacesUtils.history.removeObserver(observer); > + PlacesObservers.removeListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/head_helpers.js:597 (Diff revision 2) > onClearHistory() {}, > onPageChanged() {}, > onDeleteVisits() {}, > }; > PlacesUtils.history.addObserver(observer, false); > + PlacesObservers.addListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:16 (Diff revision 2) > const TIMESTAMP2 = (Date.now() - 6592903) * 1000; > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > + let listener = new PlacesWeakCallbackWrapper((events) => { Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:17 (Diff revision 2) > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > - onBeginUpdateBatch: function onBeginUpdateBatch() {}, > + let listener = new PlacesWeakCallbackWrapper((events) => { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:20 (Diff revision 2) > - Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS, > - Ci.nsISupportsWeakReference > - ]) > - }, true); > - }); > + }); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:154 (Diff revision 2) > - > - let uri = NetUtil.newURI(aUrl); > - > - PlacesUtils.history.addObserver({ > - QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]), > - onBeginUpdateBatch() {}, > + function listener(aEvents) { > + Assert.equal(aEvents.length, 1); > + Assert.equal(aEvents[0].type, "page-visited"); > + let data = aEvents[0].data; > + if (data.url == aUrl) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:158 (Diff revision 2) > - if (visitUri.equals(uri)) { > - PlacesUtils.history.removeObserver(this); > - resolve([time, transitionType]); > - } > + } > - }, > - onTitleChanged() {}, > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:325 (Diff revision 2) > waitForNotification(notification, conditionFn = () => true, type = "bookmarks") { > + if (type == "places") { > + return new Promise(resolve => { > + function listener(events) { > + if (conditionFn(events.map(e => e.data))) { > + PlacesObservers.removeListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:329 (Diff revision 2) > + if (conditionFn(events.map(e => e.data))) { > + PlacesObservers.removeListener([notification], listener); > + resolve(); > + } > + } > + PlacesObservers.addListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:33 (Diff revision 2) > > async function promiseLoadedThreeTimes(uri) { > - historyObserver.count = 0; > - historyObserver.expectedURI = Services.io.newURI(uri); > + count = 0; > + expectedURI = uri; > let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:38 (Diff revision 2) > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); > gBrowser.loadURI(uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > - PlacesUtils.history.removeObserver(historyObserver); > + PlacesObservers.removeListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:25 (Diff revision 2) > - if (!uri.equals(FINAL_URI)) { > + if (uri != FINAL_URI.spec) { > return; > } > > is(this._notified.length, 4); > - PlacesUtils.history.removeObserver(this); > + PlacesObservers.removeListener(["page-visited"], this.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:61 (Diff revision 2) > - } = visits[0]; > - this.onVisit(uri, visitId, time, referrerId, transitionType); > + } = events[0].data; > + this.onVisit(url, visitId, visitTime, referringVisitId, transitionType); > } > + }; > + observer.handleEvents = observer.handleEvents.bind(observer); > + PlacesObservers.addListener(["page-visited"], observer.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:17 (Diff revision 2) > let visitedPromise = new Promise(resolve => { > - let historyObserver = { > - onVisit(aURI, aVisitID, aTime, aReferringID, aTransitionType) { > - PlacesUtils.history.removeObserver(historyObserver); > - info("Received onVisit: " + aURI.spec); > - fieldForUrl(aURI, "frecency", function(aFrecency) { > + function listener(aEvents) { > + is(aEvents.length, 1, "Right number of visits notified"); > + is(aEvents[0].type, "page-visited"); > + let uri = NetUtil.newURI(aEvents[0].data.url); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:30 (Diff revision 2) > - resolve(); > + resolve(); > - }); > + }); > - }); > + }); > - }); > + }); > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:17 (Diff revision 2) > - } = aVisits[0]; > - info("onVisits: " + uri.spec); > - if (uri.equals(EXPECTED_URL)) { > + } = aEvents[0].data; > + info("'page-visited': " + url); > + if (url == EXPECTED_URL.spec) { > - Assert.equal(lastKnownTitle, null, "Should not have a title"); > + Assert.equal(lastKnownTitle, null, "Should not have a title"); > - } > + } > - }, > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:29 (Diff revision 2) > resolve(); > } > }, > }; > PlacesUtils.history.addObserver(obs); > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:24 (Diff revision 2) > // Used to verify errors are not marked as typed. > PlacesUtils.history.markPageAsTyped(NetUtil.newURI(TEST_URL)); > > let promiseVisit = new Promise(resolve => { > - let historyObserver = { > - __proto__: NavHistoryObserver.prototype, > + function onVisits(events) { > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:30 (Diff revision 2) > - PlacesUtils.history.removeObserver(historyObserver); > - is(visits.length, 1, "Right number of visits"); > + is(events[0].type, "page-visited"); > + is(events[0].data.url, TEST_URL, "Check visited url"); > - is(visits[0].uri.spec, TEST_URL, "Check visited url"); > - resolve(); > + resolve(); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:88 (Diff revision 2) > - aCallback) { > + aCallback) { > - this.uri = aURI; > + this.uri = aURI; > - this.guid = aGUID; > + this.guid = aGUID; > - this.callback = aCallback; > + this.callback = aCallback; > + this.handlePlacesEvent = this.handlePlacesEvent.bind(this); > + PlacesObservers.addListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:117 (Diff revision 2) > - if (!this.uri.equals(uri) || this.guid != guid) { > + if (this.uri.spec != url || this.guid != pageGuid) { > return; > } > - this.callback(time, transitionType, lastKnownTitle); > - }, > -}; > + this.callback(visitTime, transitionType, lastKnownTitle); > + > + PlacesObservers.removeListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:988 (Diff revision 2) > - PlacesUtils.history.addObserver({ > - onVisits(visits) { > - Assert.equal(visits.length, 1, "Should only get notified for one visit."); > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + function onVisits(events) { > + Assert.equal(events.length, 1, "Should only get notified for one visit."); > + Assert.equal(events[0].type, "page-visited"); > + let {url} = events[0].data; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:991 (Diff revision 2) > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + let {url} = events[0].data; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); > - resolve(); > + resolve(); > - } > + } > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:38 (Diff revision 2) > - hidden, > + hidden, > - visitCount, > + visitCount, > - typed, > + typedCount, > - lastKnownTitle, > + lastKnownTitle, > - } = aVisits[0]; > - PlacesUtils.history.removeObserver(this); > + } = aEvents[0].data; > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:44 (Diff revision 2) > - aCallback(uri, visitId, time, 0, referrerId, > - transitionType, guid, hidden, visitCount, > - typed, lastKnownTitle); > + let uriArg = NetUtil.newURI(url); > + aCallback(uriArg, visitId, visitTime, 0, referringVisitId, > + transitionType, pageGuid, hidden, visitCount, > + typedCount, lastKnownTitle); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:45 (Diff revision 2) > + * which resolves a promise on being called. > + */ > +function promiseVisitAdded(callback) { > + return new Promise(resolve => { > + function listener(events) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:51 (Diff revision 2) > + Assert.equal(events.length, 1, "Right number of visits notified"); > + Assert.equal(events[0].type, "page-visited"); > + callback(events[0].data); > + resolve(); > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:140 (Diff revision 2) > - Assert.equal(visit.referrerId, 0); > + Assert.equal(visit.referringVisitId, 0); > - Assert.equal(visit.transitionType, TRANSITION_TYPED); > + Assert.equal(visit.transitionType, TRANSITION_TYPED); > - Assert.equal(visit.visitCount, 3); > + Assert.equal(visit.visitCount, 3); > - Assert.equal(visit.typed, 1); > + Assert.equal(visit.typedCount, 1); > > - PlacesUtils.history.removeObserver(observer, false); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:146 (Diff revision 2) > - resolve(); > + resolve(); > - break; > + break; > - } > + } > - } > + } > - }, > - }; > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:27 (Diff revision 2) > - Assert.equal(aTransitionType, gVisits[this._visitCount].transition); > - this._visitCount++; > + Assert.equal(data.transitionType, gVisits[visitCount].transition); > + visitCount++; > > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:30 (Diff revision 2) > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); > - } > + } > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef]

(In reply to Code Review Bot [:reviewbot] from comment #36) > Code analysis found 8 defects in this patch: > - 8 defects found by mozlint To any humans: sorry for the noise from this. These are all corrected by the last patch's linting changes. I'll make sure to set that as the first patch in future reviews.

Hi Doug, I'm assuming that :mak was your first choice for reviewer on the first patch here and I was tagged by accident. Let me know if that isn't the case.

(In reply to Blake Kaplan (:mrbkap) from comment #39) > Hi Doug, I'm assuming that :mak was your first choice for reviewer on the > first patch here and I was tagged by accident. Let me know if that isn't the > case. Hey Blake - sorry, I should have clarified in a comment. mconley recommended you as a DOM peer - I would just like someone familiar with webidl to look over my implementation, since I felt a bit out of my depth with this stuff.

Not sure if intended, there is a very long log.tmp file in the idl patch

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review241326 ::: toolkit/components/places/nsLivemarkService.js:710 (Diff revision 2) > * If provided will update nodes having the given uri, > * otherwise any node. > * @param aVisitedStatus > * Whether the nodes should be set as visited. > */ > updateURIVisitedStatus(aURI, aVisitedStatus) { just to avoid confusion, let's rename aURI to aHref, or even just modernize the arguments to (href, visitedStatus) ::: toolkit/components/places/nsLivemarkService.js:831 (Diff revision 2) > } > > this._livemark.children = livemarkChildren; > } catch (ex) { > + dump(ex); > + Cu.reportError("BOB!!!"); some leftovers, I assume ::: toolkit/components/places/nsNavBookmarks.cpp:2092 (Diff revision 2) > > -NS_IMETHODIMP > -nsNavBookmarks::OnVisits(nsIVisitData** aVisits, uint32_t aVisitsCount) > +void > +nsNavBookmarks::HandlePlacesEvent(JSContext* aCx, > + const nsTArray<dom::PlacesEvent>& aEvents) > { > - NS_ENSURE_ARG(aVisits); > + for (const dom::PlacesEvent& event : aEvents) { nit: const auto& ::: toolkit/modules/NewTabUtils.jsm:718 (Diff revision 2) > > /** > * Called by the history service. > */ > onTitleChanged: function PlacesProvider_onTitleChanged(aURI, aNewTitle, aGUID) { > + if (typeof(aURI) != "string") { nit: better to check aURI instanceof Ci.nsIURI

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review241344

Comment on attachment 8951105 [details] Bug 1340498 - Implement new Places Observers interface https://reviewboard.mozilla.org/r/220250/#review239020 I have some questions that I'd like to know more about this before I stamp an r+ on it. Overall, this implementation makes sense, though I'm hoping the need for a GlobalObject doesn't cause too much trouble. ::: commit-message-e43f2:13 (Diff revision 2) > +There were some difficulties with WebIDL though, most of which > +revolved around allowing consumers to be weakly referenced, from > +both C++ and JS. The simplest solution I could come up with was to > +create a simple native interface for the C++ case, and a WebIDL > +wrapper for a JS callback in the JS case. Suggestions for simpler > +alternatives are very welcome though. I think your solution makes sense. I don't know of anything built-in to do this and your implementation seems reasonable. ::: commit-message-e43f2:17 (Diff revision 2) > +wrapper for a JS callback in the JS case. Suggestions for simpler > +alternatives are very welcome though. > + > +The other difficulty which arose when attempting to consume this > +was finding an appropriate JSContext and GlobalObject. The only > +solution I could come up with for consuming this from C++ Do you have an example of this? A cursory glance seems to indicate that ProcessGlobal is only intended to be used from child processes. We might need to create a service or global GlobalObject in the parent process. Or something. smaug might have a better solution. ::: dom/base/PlacesEventAccumulator.h:65 (Diff revision 2) > +protected: > + ~PlacesEventAccumulator(); > + > + nsCOMPtr<nsISupports> mParent; > + nsTArray<PlacesEventType> mEventTypes; > + nsTArray<JS::Heap<JS::Value>> mEvents; Out of curiosity: why not make this an array of `Tuple<PlacesEventType, JS::Heap<JS::Value>>`? ::: dom/base/PlacesObservers.cpp:86 (Diff revision 2) > + return 0; > + } > + return 1 << ((uint32_t)aEventType - 1); > +} > + > +uint32_t GetFlagsForEventTypes(const nsTArray<PlacesEventType>& aEventTypes) { Nit (here and below): the first brace gets its own line. ::: dom/chrome-webidl/PlacesObservers.webidl:73 (Diff revision 2) > + DOMString? lastKnownTitle = null; > +}; > + > +dictionary PlacesEvent { > + PlacesEventType type = "none"; > + any data = null; Would it be possible to get rid of these `any`s in favor of `enum`s? That might be nice for documenting exactly what types of data can pass through. ::: tmp.log:1 (Diff revision 2) > + 0:00.99 Clobber not needed. This shouldn't be checked in.

(In reply to Blake Kaplan (:mrbkap) from comment #44) > Would it be possible to get rid of these `any`s in favor of `enum`s? That > might be nice for documenting exactly what types of data can pass through. I'm not sure I understand. Is it possible to use enums to create sum types? My initial attempt created sum types with "or", but that didn't work with webidl dictionaries (since we can't distinguish them when passed from JS), and defining an interface for each type seemed like too much boilerplate to be worth it.

(In reply to Blake Kaplan (:mrbkap) from comment #44) > ::: commit-message-e43f2:17 > (Diff revision 2) > > +wrapper for a JS callback in the JS case. Suggestions for simpler > > +alternatives are very welcome though. > > + > > +The other difficulty which arose when attempting to consume this > > +was finding an appropriate JSContext and GlobalObject. The only > > +solution I could come up with for consuming this from C++ > > Do you have an example of this? A cursory glance seems to indicate that > ProcessGlobal is only intended to be used from child processes. > > We might need to create a service or global GlobalObject in the parent > process. Or something. smaug might have a better solution. Olli, do you know if this is accurate / do you have any suggestions for getting a JSContext/GlobalObject to call JS from C++ asynchronously like this? (I.e., without a promise or something else obvious to pull a JSContext from.)

Woops, forgot needinfo. See comment #51

Not quite sure what is being asked here, but if there is a need for kind of temporary small js global, SimpleGlobalObject is one option. No idea though yet whether it would work here.

(In reply to Doug Thayer [:dthayer] from comment #45) > I'm not sure I understand. Is it possible to use enums to create sum types? > My initial attempt created sum types with "or", but that didn't work with > webidl dictionaries (since we can't distinguish them when passed from JS), > and defining an interface for each type seemed like too much boilerplate to > be worth it. Sorry, I had a brain fart.

Comment on attachment 8967828 [details] Bug 1340498 - Fix unified sources build errors https://reviewboard.mozilla.org/r/236512/#review242722

Is there still a patch to review here?

(In reply to Andrew Overholt [:overholt] from comment #56) > Is there still a patch to review here? Yeah - sorry, it's fallen under a bit. I chatted with smaug and I still need to investigate a little bit whether we can avoid needing a GlobalObject from C++ to consume this.

(In reply to Doug Thayer [:dthayer] (PTO on May 17) from comment #57) > Yeah - sorry, it's fallen under a bit. I chatted with smaug and I still need > to investigate a little bit whether we can avoid needing a GlobalObject from > C++ to consume this. Hi Doug! I've been watching the progress here with much anticipation :-) Were you able to get a bit further with the sans-GlobalObject investigation? If there's a way I can help out by getting the right people on an etherpad or a call, let me know!

Hey Mike - I haven't gotten any further with that. The initial conversation with Olli and I on IRC kind of fizzled. Olli, could you maybe just help me figure out the minimum I need from the jsapi with the following requirements: - Ability to publish from C++ and from JS to chrome-only subscribers in C++ and JS, without a JSContext or similar in the stack - Ability to publish payloads which consist of an array of non-homogeneous struct-like objects, something like: [{'type': 'page-visited', {'url': 'http://foo.com', ...}, {'type': 'bookmark-added', {'url': 'http://foo.com'}}, ...] - Ideally jank-free when publishing arrays of hundreds or thousands of events. The existing system has a high per-item overhead, so we'd like to reduce that within reason. Let me know if you need more information - further elaboration of what we're going for is here: https://docs.google.com/document/d/1G45vfd6RXFXwNz7i4FV40lDCU0ao-JX_bZdgJV4tLjk/edit

That looks rather homogeneous array. Does jank-free mean that reading from array is asynchronous - or I mean that partial arrays are published and then consumer may ask more data? (it sounds like that is the case, but not sure.) So the API in the doc feels quite a bit like DOM events API. Why PlacesObservers couldn't be an EventTarget object (implemented by extending DOMEventTargetHelper). EventListeners can be implemented in JS or in C++, and events dispatched using JS or C++. One thing unclear to me what kind of payload the event may have. The document doesn't explain that. Array of some structs, but what kind of structs? Could we have [Frozen, Cached, Pure] sequence<PlacesData> data; and then implement PlacesData as an interface and each different type would extend from the base interface?

(In reply to Olli Pettay [:smaug] from comment #60) > That looks rather homogeneous array. Sorry, you're right it does, but that's just me being absentminded. In reality the struct-like object after the type fields would be different. Amended: [{'type': 'page-visited', {'url': 'http://foo.com', ...}, {'type': 'bookmark-added', {'index': 5, ...}}, ...] > Does jank-free mean that reading from array is asynchronous - or I mean that > partial arrays are published and then consumer may ask more data? (it sounds > like that is the case, but not sure.) However we want to achieve being jank-free, but I was thinking more along the lines of just ensuring a low interop overhead per event. Before bug 1421703 we spent a large chunk of time on the main thread in XPConnect when moving visit events from C++ -land to JS-land, which was resolved simply by doing it in aggregate. Other event types still have this problem. I just want to make sure that whatever solution we come up with extends that advantage to all event types. > So the API in the doc feels quite a bit like DOM events API. Why > PlacesObservers couldn't be an EventTarget object (implemented by extending > DOMEventTargetHelper). Could you help me understand what setting this up would look like? I notice there's a constructor that doesn't require an nsIGlobalObject but it's only used in one place which later calls BindToOwner, so I assume the nsIGlobalObject is required before publishing events? If so, where would I want to initialize this and how would I make it accessible from anywhere to create a singleton for the process? > EventListeners can be implemented in JS or in C++, > and events dispatched using JS or C++. > One thing unclear to me what kind of payload the event may have. The > document doesn't explain that. > Array of some structs, but what kind of structs? > Could we have > [Frozen, Cached, Pure] > sequence<PlacesData> data; > and then implement PlacesData as an interface and each different type would > extend from the base interface? Just flat POD structs containing strings, integers, booleans, etc. To be clear, data will be one of these structs, not an array of structs. The array would be an array of PlacesEvents, but I think that doesn't change what you're asking for. In that case, each event type would have its own interface extending PlacesData? That's fine, it's just unfortunate that there's so much boilerplate implementing interfaces for POD types - I was hoping to avoid that by using dictionaries, but if that's what we need to do I can manage.

(In reply to Doug Thayer [:dthayer] (PTO on May 17) from comment #61) > > So the API in the doc feels quite a bit like DOM events API. Why > > PlacesObservers couldn't be an EventTarget object (implemented by extending > > DOMEventTargetHelper). > > Could you help me understand what setting this up would look like? I notice > there's a constructor that doesn't require an nsIGlobalObject but it's only > used in one place which later calls BindToOwner, so I assume the > nsIGlobalObject is required before publishing events? Just create one and keep it alive? Like SimpleGlobalObject? > If so, where would I > want to initialize this and how would I make it accessible from anywhere to > create a singleton for the process? Make the object a service, that is after all the normal way to have singleton, well, services. > > Just flat POD structs containing strings, integers, booleans, etc. To be > clear, data will be one of these structs, not an array of structs. The array > would be an array of PlacesEvents, but I think that doesn't change what > you're asking for. In that case, each event type would have its own > interface extending PlacesData? That's fine, it's just unfortunate that > there's so much boilerplate implementing interfaces for POD types - I was > hoping to avoid that by using dictionaries, but if that's what we need to do > I can manage. Well, what kind of data is there, exactly? If there is a need for several "events" with some different payloads (where payload is always something simple), just use code generator to generate the events? See examples from https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/dom/webidl/moz.build#1023 Those events are implemented by having just .webidl file. C++ is generated from it.

Comment on attachment 8951105 [details] Bug 1340498 - Implement new Places Observers interface https://reviewboard.mozilla.org/r/220250/#review253776 Clearing this for now while waiting for a new patch.

I'm putting up the updated patches for this shortly. I ended up implementing it with manually coded interfaces, and not via the DOMEventTargetHelper with generated event objects for the following reasons: - We want to have the API publish events as arrays, and have subscribers subscribe to some set of types and only be notified with arrays containing mixes of those types. This does not conform with a DOM events API. Blake suggested having an event which could be queried with a mask of types. This is fine, but the array elements at that point would no longer be proper events, and I believe as such we wouldn't be able to generate them anyway with something like GENERATED_EVENTS_WEBIDL_FILES. - We want to have the option to subscribe to the API with a weak reference. I couldn't find anything like this in the DOM events infrastructure or any examples of consumers with this behavior. Please correct me if I'm wrong in this. - It seems like even if this could be overcome, it would be a bit unnatural and misleading for it to masquerade as a typical DOM events API when it behaves in such a peculiar way. I'd rather have it keep its own semantics to not mislead or confuse anyone. Because of the use of interfaces though I was able to avoid any dependency on a JSContext or similar from C++, so that's good.

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review254882 Code analysis found 8 defects in this patch: - 8 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/modules/engines/history.js:487 (Diff revision 4) > > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:488 (Diff revision 4) > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:495 (Diff revision 4) > > onStop() { > this._log.info("Removing Places observer."); > PlacesUtils.history.removeObserver(this); > + if (this._placesObserver) { > + PlacesObservers.removeListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/PlacesUtils.jsm:343 (Diff revision 4) > TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin", > TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success", > TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed", > > ACTION_SCHEME: "moz-action:", > + observers: PlacesObservers, Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:18 (Diff revision 4) > > XPCOMUtils.defineLazyGetter(this, "history", function() { > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:20 (Diff revision 4) > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( > + livemarks.handlePlacesEvents.bind(livemarks)); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:553 (Diff revision 4) > * Must be called before the provider is used. > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:554 (Diff revision 4) > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef]

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review254884 Code analysis found 39 defects in this patch (only the first 30 are reported here): - 39 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/tests/unit/head_helpers.js:554 (Diff revision 4) > async function promiseVisit(expectedType, expectedURI) { > return new Promise(resolve => { > function done(type, uri) { > - if (uri.equals(expectedURI) && type == expectedType) { > + if (uri == expectedURI.spec && type == expectedType) { > PlacesUtils.history.removeObserver(observer); > + PlacesObservers.removeListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/head_helpers.js:578 (Diff revision 4) > onClearHistory() {}, > onPageChanged() {}, > onDeleteVisits() {}, > }; > PlacesUtils.history.addObserver(observer, false); > + PlacesObservers.addListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:16 (Diff revision 4) > const TIMESTAMP2 = (Date.now() - 6592903) * 1000; > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > + let listener = new PlacesWeakCallbackWrapper((events) => { Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:17 (Diff revision 4) > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > - onBeginUpdateBatch: function onBeginUpdateBatch() {}, > + let listener = new PlacesWeakCallbackWrapper((events) => { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:20 (Diff revision 4) > - Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS, > - Ci.nsISupportsWeakReference > - ]) > - }, true); > - }); > + }); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:154 (Diff revision 4) > - > - let uri = NetUtil.newURI(aUrl); > - > - PlacesUtils.history.addObserver({ > - QueryInterface: ChromeUtils.generateQI([Ci.nsINavHistoryObserver]), > - onBeginUpdateBatch() {}, > + function listener(aEvents) { > + Assert.equal(aEvents.length, 1); > + let event = aEvents[0]; > + Assert.equal(event.type, "page-visited"); > + if (event.url == aUrl) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:158 (Diff revision 4) > - if (visitUri.equals(uri)) { > - PlacesUtils.history.removeObserver(this); > - resolve([time, transitionType]); > - } > + } > - }, > - onTitleChanged() {}, > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:325 (Diff revision 4) > waitForNotification(notification, conditionFn = () => true, type = "bookmarks") { > + if (type == "places") { > + return new Promise(resolve => { > + function listener(events) { > + if (conditionFn(events)) { > + PlacesObservers.removeListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:329 (Diff revision 4) > + if (conditionFn(events)) { > + PlacesObservers.removeListener([notification], listener); > + resolve(); > + } > + } > + PlacesObservers.addListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:33 (Diff revision 4) > > async function promiseLoadedThreeTimes(uri) { > - historyObserver.count = 0; > - historyObserver.expectedURI = Services.io.newURI(uri); > + count = 0; > + expectedURI = uri; > let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:38 (Diff revision 4) > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); > gBrowser.loadURI(uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > - PlacesUtils.history.removeObserver(historyObserver); > + PlacesObservers.removeListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:25 (Diff revision 4) > - if (!uri.equals(FINAL_URI)) { > + if (uri != FINAL_URI.spec) { > return; > } > > is(this._notified.length, 4); > - PlacesUtils.history.removeObserver(this); > + PlacesObservers.removeListener(["page-visited"], this.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:61 (Diff revision 4) > - } = visits[0]; > - this.onVisit(uri, visitId, time, referrerId, transitionType); > + } = events[0]; > + this.onVisit(url, visitId, visitTime, referringVisitId, transitionType); > } > + }; > + observer.handleEvents = observer.handleEvents.bind(observer); > + PlacesObservers.addListener(["page-visited"], observer.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:17 (Diff revision 4) > let visitedPromise = new Promise(resolve => { > - let historyObserver = { > - onVisit(aURI, aVisitID, aTime, aReferringID, aTransitionType) { > - PlacesUtils.history.removeObserver(historyObserver); > - info("Received onVisit: " + aURI.spec); > - fieldForUrl(aURI, "frecency", function(aFrecency) { > + function listener(aEvents) { > + is(aEvents.length, 1, "Right number of visits notified"); > + is(aEvents[0].type, "page-visited"); > + let uri = NetUtil.newURI(aEvents[0].url); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:30 (Diff revision 4) > - resolve(); > + resolve(); > - }); > + }); > - }); > + }); > - }); > + }); > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:17 (Diff revision 4) > - } = aVisits[0]; > - info("onVisits: " + uri.spec); > - if (uri.equals(EXPECTED_URL)) { > + } = aEvents[0]; > + info("'page-visited': " + url); > + if (url == EXPECTED_URL.spec) { > - Assert.equal(lastKnownTitle, null, "Should not have a title"); > + Assert.equal(lastKnownTitle, null, "Should not have a title"); > - } > + } > - }, > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:29 (Diff revision 4) > resolve(); > } > }, > }; > PlacesUtils.history.addObserver(obs); > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:24 (Diff revision 4) > // Used to verify errors are not marked as typed. > PlacesUtils.history.markPageAsTyped(NetUtil.newURI(TEST_URL)); > > let promiseVisit = new Promise(resolve => { > - let historyObserver = { > - __proto__: NavHistoryObserver.prototype, > + function onVisits(events) { > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:30 (Diff revision 4) > - PlacesUtils.history.removeObserver(historyObserver); > - is(visits.length, 1, "Right number of visits"); > + is(events[0].type, "page-visited"); > + is(events[0].url, TEST_URL, "Check visited url"); > - is(visits[0].uri.spec, TEST_URL, "Check visited url"); > - resolve(); > + resolve(); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:92 (Diff revision 4) > - aCallback) { > + aCallback) { > - this.uri = aURI; > + this.uri = aURI; > - this.guid = aGUID; > + this.guid = aGUID; > - this.callback = aCallback; > + this.callback = aCallback; > + this.handlePlacesEvent = this.handlePlacesEvent.bind(this); > + PlacesObservers.addListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:121 (Diff revision 4) > - if (!this.uri.equals(uri) || this.guid != guid) { > + if (this.uri.spec != url || this.guid != pageGuid) { > return; > } > - this.callback(time, transitionType, lastKnownTitle); > - }, > -}; > + this.callback(visitTime * 1000, transitionType, lastKnownTitle); > + > + PlacesObservers.removeListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:991 (Diff revision 4) > - PlacesUtils.history.addObserver({ > - onVisits(visits) { > - Assert.equal(visits.length, 1, "Should only get notified for one visit."); > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + function onVisits(events) { > + Assert.equal(events.length, 1, "Should only get notified for one visit."); > + Assert.equal(events[0].type, "page-visited"); > + let {url} = events[0]; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:994 (Diff revision 4) > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + let {url} = events[0]; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); > - resolve(); > + resolve(); > - } > + } > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:38 (Diff revision 4) > - hidden, > + hidden, > - visitCount, > + visitCount, > - typed, > + typedCount, > - lastKnownTitle, > + lastKnownTitle, > - } = aVisits[0]; > - PlacesUtils.history.removeObserver(this); > + } = aEvents[0]; > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:44 (Diff revision 4) > - aCallback(uri, visitId, time, 0, referrerId, > - transitionType, guid, hidden, visitCount, > - typed, lastKnownTitle); > + let uriArg = NetUtil.newURI(url); > + aCallback(uriArg, visitId, visitTime, 0, referringVisitId, > + transitionType, pageGuid, hidden, visitCount, > + typedCount, lastKnownTitle); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:45 (Diff revision 4) > + * which resolves a promise on being called. > + */ > +function promiseVisitAdded(callback) { > + return new Promise(resolve => { > + function listener(events) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:51 (Diff revision 4) > + Assert.equal(events.length, 1, "Right number of visits notified"); > + Assert.equal(events[0].type, "page-visited"); > + callback(events[0]); > + resolve(); > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:140 (Diff revision 4) > - Assert.equal(visit.referrerId, 0); > + Assert.equal(visit.referringVisitId, 0); > - Assert.equal(visit.transitionType, TRANSITION_TYPED); > + Assert.equal(visit.transitionType, TRANSITION_TYPED); > - Assert.equal(visit.visitCount, 3); > + Assert.equal(visit.visitCount, 3); > - Assert.equal(visit.typed, 1); > + Assert.equal(visit.typedCount, 1); > > - PlacesUtils.history.removeObserver(observer, false); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:146 (Diff revision 4) > - resolve(); > + resolve(); > - break; > + break; > - } > + } > - } > + } > - }, > - }; > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:27 (Diff revision 4) > - Assert.equal(aTransitionType, gVisits[this._visitCount].transition); > - this._visitCount++; > + Assert.equal(event.transitionType, gVisits[visitCount].transition); > + visitCount++; > > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:30 (Diff revision 4) > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); > - } > + } > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef]

Comment on attachment 8951105 [details] Bug 1340498 - Implement new Places Observers interface https://reviewboard.mozilla.org/r/220250/#review256040 ::: dom/base/PlacesObservers.cpp:39 (Diff revision 4) > +using FlaggedArray = nsTArray<Flagged<T>>; > + > +template <class T> > +struct ListenerCollection > +{ > +static StaticAutoPtr<FlaggedArray<T>> gListeners; This should all be indented since it's in a struct. ::: dom/base/PlacesObservers.cpp:72 (Diff revision 4) > +typedef ListenerCollection<WeakPtr<PlacesWeakCallbackWrapper>> WeakJSListeners; > +typedef ListenerCollection<WeakPtr<places::INativePlacesEventCallback>> WeakNativeListeners; > + > +static bool gCallingListeners = false; > + > +uint32_t GetEventTypeFlag(PlacesEventType aEventType) Nit (here and below): Please put these function names on their own lines as you do for `CallListeners`. ::: dom/base/PlacesVisit.h:54 (Diff revision 4) > + nsString mUrl; > + uint64_t mVisitId; > + uint64_t mVisitTime; > + uint64_t mReferringVisitId; > + uint32_t mTransitionType; > + nsTString<char> mPageGuid; nsString

Comment on attachment 8982762 [details] Bug 1340498 - Fix unified sources build errors https://reviewboard.mozilla.org/r/248656/#review258286

Comment on attachment 8951108 [details] Bug 1340498 - Add new globals to lint config https://reviewboard.mozilla.org/r/220256/#review258688

I assume we'll wait for the merge.

(In reply to Marco Bonardo [::mak] from comment #75) > I assume we'll wait for the merge. Yup!

Comment on attachment 8951106 [details] Bug 1340498 - Update onVisits uses to 'page-visited' https://reviewboard.mozilla.org/r/220252/#review259472 Code analysis found 8 defects in this patch: - 8 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/modules/engines/history.js:487 (Diff revision 5) > > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:488 (Diff revision 5) > onStart() { > this._log.info("Adding Places observer."); > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/modules/engines/history.js:495 (Diff revision 5) > > onStop() { > this._log.info("Removing Places observer."); > PlacesUtils.history.removeObserver(this); > + if (this._placesObserver) { > + PlacesObservers.removeListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/PlacesUtils.jsm:343 (Diff revision 5) > TOPIC_BOOKMARKS_RESTORE_BEGIN: "bookmarks-restore-begin", > TOPIC_BOOKMARKS_RESTORE_SUCCESS: "bookmarks-restore-success", > TOPIC_BOOKMARKS_RESTORE_FAILED: "bookmarks-restore-failed", > > ACTION_SCHEME: "moz-action:", > + observers: PlacesObservers, Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:18 (Diff revision 5) > > XPCOMUtils.defineLazyGetter(this, "history", function() { > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/components/places/nsLivemarkService.js:20 (Diff revision 5) > + let livemarks = PlacesUtils.livemarks; > // Lazily add an history observer when it's actually needed. > - PlacesUtils.history.addObserver(PlacesUtils.livemarks, true); > + PlacesUtils.history.addObserver(livemarks, true); > + let listener = new PlacesWeakCallbackWrapper( > + livemarks.handlePlacesEvents.bind(livemarks)); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:553 (Diff revision 5) > * Must be called before the provider is used. > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: toolkit/modules/NewTabUtils.jsm:554 (Diff revision 5) > */ > init: function PlacesProvider_init() { > PlacesUtils.history.addObserver(this, true); > + this._placesObserver = > + new PlacesWeakCallbackWrapper(this.handlePlacesEvents.bind(this)); > + PlacesObservers.addListener(["page-visited"], this._placesObserver); Error: 'placesobservers' is not defined. [eslint: no-undef]

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6465310b3de9 Implement new Places Observers interface r=mrbkap https://hg.mozilla.org/integration/autoland/rev/e2ac49ef2034 Update onVisits uses to 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/a444ab9cefde Update onVisits tests to use 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/2adde1d1742a Add new globals to lint config r=standard8 https://hg.mozilla.org/integration/autoland/rev/fae677707059 Fix unified sources build errors r=mrbkap

Comment on attachment 8951107 [details] Bug 1340498 - Update onVisits tests to use 'page-visited' https://reviewboard.mozilla.org/r/220254/#review259480 Code analysis found 39 defects in this patch (only the first 30 are reported here): - 39 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: services/sync/tests/unit/head_helpers.js:554 (Diff revision 5) > async function promiseVisit(expectedType, expectedURI) { > return new Promise(resolve => { > function done(type, uri) { > - if (uri.equals(expectedURI) && type == expectedType) { > + if (uri == expectedURI.spec && type == expectedType) { > PlacesUtils.history.removeObserver(observer); > + PlacesObservers.removeListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/head_helpers.js:578 (Diff revision 5) > onClearHistory() {}, > onPageChanged() {}, > onDeleteVisits() {}, > }; > PlacesUtils.history.addObserver(observer, false); > + PlacesObservers.addListener(["page-visited"], Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:16 (Diff revision 5) > const TIMESTAMP2 = (Date.now() - 6592903) * 1000; > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > + let listener = new PlacesWeakCallbackWrapper((events) => { Error: 'placesweakcallbackwrapper' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:17 (Diff revision 5) > const TIMESTAMP3 = (Date.now() - 123894) * 1000; > > function promiseOnVisitObserved() { > return new Promise(res => { > - PlacesUtils.history.addObserver({ > - onBeginUpdateBatch: function onBeginUpdateBatch() {}, > + let listener = new PlacesWeakCallbackWrapper((events) => { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: services/sync/tests/unit/test_history_store.js:20 (Diff revision 5) > - Ci.nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS, > - Ci.nsISupportsWeakReference > - ]) > - }, true); > - }); > + }); > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:154 (Diff revision 5) > - > - let uri = NetUtil.newURI(aUrl); > - > - PlacesUtils.history.addObserver({ > - QueryInterface: ChromeUtils.generateQI([Ci.nsINavHistoryObserver]), > - onBeginUpdateBatch() {}, > + function listener(aEvents) { > + Assert.equal(aEvents.length, 1); > + let event = aEvents[0]; > + Assert.equal(event.type, "page-visited"); > + if (event.url == aUrl) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/downloads/test/unit/head.js:158 (Diff revision 5) > - if (visitUri.equals(uri)) { > - PlacesUtils.history.removeObserver(this); > - resolve([time, transitionType]); > - } > + } > - }, > - onTitleChanged() {}, > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:325 (Diff revision 5) > waitForNotification(notification, conditionFn = () => true, type = "bookmarks") { > + if (type == "places") { > + return new Promise(resolve => { > + function listener(events) { > + if (conditionFn(events)) { > + PlacesObservers.removeListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/PlacesTestUtils.jsm:329 (Diff revision 5) > + if (conditionFn(events)) { > + PlacesObservers.removeListener([notification], listener); > + resolve(); > + } > + } > + PlacesObservers.addListener([notification], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:33 (Diff revision 5) > > async function promiseLoadedThreeTimes(uri) { > - historyObserver.count = 0; > - historyObserver.expectedURI = Services.io.newURI(uri); > + count = 0; > + expectedURI = uri; > let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_bug399606.js:38 (Diff revision 5) > - PlacesUtils.history.addObserver(historyObserver); > + PlacesObservers.addListener(["page-visited"], onVisitsListener); > gBrowser.loadURI(uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri); > - PlacesUtils.history.removeObserver(historyObserver); > + PlacesObservers.removeListener(["page-visited"], onVisitsListener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:25 (Diff revision 5) > - if (!uri.equals(FINAL_URI)) { > + if (uri != FINAL_URI.spec) { > return; > } > > is(this._notified.length, 4); > - PlacesUtils.history.removeObserver(this); > + PlacesObservers.removeListener(["page-visited"], this.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_double_redirect.js:61 (Diff revision 5) > - } = visits[0]; > - this.onVisit(uri, visitId, time, referrerId, transitionType); > + } = events[0]; > + this.onVisit(url, visitId, visitTime, referringVisitId, transitionType); > } > + }; > + observer.handleEvents = observer.handleEvents.bind(observer); > + PlacesObservers.addListener(["page-visited"], observer.handleEvents); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:17 (Diff revision 5) > let visitedPromise = new Promise(resolve => { > - let historyObserver = { > - onVisit(aURI, aVisitID, aTime, aReferringID, aTransitionType) { > - PlacesUtils.history.removeObserver(historyObserver); > - info("Received onVisit: " + aURI.spec); > - fieldForUrl(aURI, "frecency", function(aFrecency) { > + function listener(aEvents) { > + is(aEvents.length, 1, "Right number of visits notified"); > + is(aEvents[0].type, "page-visited"); > + let uri = NetUtil.newURI(aEvents[0].url); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_notfound.js:30 (Diff revision 5) > - resolve(); > + resolve(); > - }); > + }); > - }); > + }); > - }); > + }); > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:17 (Diff revision 5) > - } = aVisits[0]; > - info("onVisits: " + uri.spec); > - if (uri.equals(EXPECTED_URL)) { > + } = aEvents[0]; > + info("'page-visited': " + url); > + if (url == EXPECTED_URL.spec) { > - Assert.equal(lastKnownTitle, null, "Should not have a title"); > + Assert.equal(lastKnownTitle, null, "Should not have a title"); > - } > + } > - }, > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_onvisit_title_null_for_navigation.js:29 (Diff revision 5) > resolve(); > } > }, > }; > PlacesUtils.history.addObserver(obs); > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:24 (Diff revision 5) > // Used to verify errors are not marked as typed. > PlacesUtils.history.markPageAsTyped(NetUtil.newURI(TEST_URL)); > > let promiseVisit = new Promise(resolve => { > - let historyObserver = { > - __proto__: NavHistoryObserver.prototype, > + function onVisits(events) { > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/browser/browser_visited_notfound.js:30 (Diff revision 5) > - PlacesUtils.history.removeObserver(historyObserver); > - is(visits.length, 1, "Right number of visits"); > + is(events[0].type, "page-visited"); > + is(events[0].url, TEST_URL, "Check visited url"); > - is(visits[0].uri.spec, TEST_URL, "Check visited url"); > - resolve(); > + resolve(); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:92 (Diff revision 5) > - aCallback) { > + aCallback) { > - this.uri = aURI; > + this.uri = aURI; > - this.guid = aGUID; > + this.guid = aGUID; > - this.callback = aCallback; > + this.callback = aCallback; > + this.handlePlacesEvent = this.handlePlacesEvent.bind(this); > + PlacesObservers.addListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:121 (Diff revision 5) > - if (!this.uri.equals(uri) || this.guid != guid) { > + if (this.uri.spec != url || this.guid != pageGuid) { > return; > } > - this.callback(time, transitionType, lastKnownTitle); > - }, > -}; > + this.callback(visitTime * 1000, transitionType, lastKnownTitle); > + > + PlacesObservers.removeListener(["page-visited"], this.handlePlacesEvent); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:991 (Diff revision 5) > - PlacesUtils.history.addObserver({ > - onVisits(visits) { > - Assert.equal(visits.length, 1, "Should only get notified for one visit."); > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + function onVisits(events) { > + Assert.equal(events.length, 1, "Should only get notified for one visit."); > + Assert.equal(events[0].type, "page-visited"); > + let {url} = events[0]; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/history/test_async_history_api.js:994 (Diff revision 5) > - let {uri} = visits[0]; > - Assert.equal(uri.spec, place.uri.spec, "Should get notified for visiting the new URI."); > - PlacesUtils.history.removeObserver(this); > + let {url} = events[0]; > + Assert.equal(url, place.uri.spec, "Should get notified for visiting the new URI."); > + PlacesObservers.removeListener(["page-visited"], onVisits); > - resolve(); > + resolve(); > - } > + } > + PlacesObservers.addListener(["page-visited"], onVisits); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:38 (Diff revision 5) > - hidden, > + hidden, > - visitCount, > + visitCount, > - typed, > + typedCount, > - lastKnownTitle, > + lastKnownTitle, > - } = aVisits[0]; > - PlacesUtils.history.removeObserver(this); > + } = aEvents[0]; > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_download_history.js:44 (Diff revision 5) > - aCallback(uri, visitId, time, 0, referrerId, > - transitionType, guid, hidden, visitCount, > - typed, lastKnownTitle); > + let uriArg = NetUtil.newURI(url); > + aCallback(uriArg, visitId, visitTime, 0, referringVisitId, > + transitionType, pageGuid, hidden, visitCount, > + typedCount, lastKnownTitle); > - } > + } > - }; > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:45 (Diff revision 5) > + * which resolves a promise on being called. > + */ > +function promiseVisitAdded(callback) { > + return new Promise(resolve => { > + function listener(events) { > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:51 (Diff revision 5) > + Assert.equal(events.length, 1, "Right number of visits notified"); > + Assert.equal(events[0].type, "page-visited"); > + callback(events[0]); > + resolve(); > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:140 (Diff revision 5) > - Assert.equal(visit.referrerId, 0); > + Assert.equal(visit.referringVisitId, 0); > - Assert.equal(visit.transitionType, TRANSITION_TYPED); > + Assert.equal(visit.transitionType, TRANSITION_TYPED); > - Assert.equal(visit.visitCount, 3); > + Assert.equal(visit.visitCount, 3); > - Assert.equal(visit.typed, 1); > + Assert.equal(visit.typedCount, 1); > > - PlacesUtils.history.removeObserver(observer, false); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_history_observer.js:146 (Diff revision 5) > - resolve(); > + resolve(); > - break; > + break; > - } > + } > - } > + } > - }, > - }; > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:27 (Diff revision 5) > - Assert.equal(aTransitionType, gVisits[this._visitCount].transition); > - this._visitCount++; > + Assert.equal(event.transitionType, gVisits[visitCount].transition); > + visitCount++; > > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef] ::: toolkit/components/places/tests/unit/test_markpageas.js:30 (Diff revision 5) > - if (this._visitCount == gVisits.length) { > + if (visitCount == gVisits.length) { > - resolveCompletionPromise(); > + resolveCompletionPromise(); > + PlacesObservers.removeListener(["page-visited"], listener); > - } > + } > - }, > - onVisits(aVisits) { > + } > + PlacesObservers.addListener(["page-visited"], listener); Error: 'placesobservers' is not defined. [eslint: no-undef]

Backed out 5 changesets (bug 1340498) for build bustages on SelectionChangeListener.h on a CLOSED TREE Backout revision https://hg.mozilla.org/integration/autoland/rev/dafeb03fc8c7ddc104eabdb2255ec689066a1ef3 Failed push:https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fae67770705996f4871eabaf2f56f6a9884407de&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Link to a recent log:https://treeherder.mozilla.org/logviewer.html#?job_id=184823799&repo=autoland Part of that log:[task 2018-06-26T00:36:28.015Z] 00:36:28 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_dom_clients_manager1.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/clients/manager -I/builds/worker/workspace/build/src/obj-firefox/dom/clients/manager -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -MD -MP -MF .deps/Unified_cpp_dom_clients_manager1.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/clients/manager/Unified_cpp_dom_clients_manager1.cpp [task 2018-06-26T00:36:28.016Z] 00:36:28 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/clients/manager' [task 2018-06-26T00:36:28.019Z] 00:36:28 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:28.020Z] 00:36:28 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:28.042Z] 00:36:28 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:28.042Z] 00:36:28 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.041Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.043Z] 00:36:31 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_gfx_layers1.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DOS_POSIX=1 -DOS_LINUX=1 -DD3D_DEBUG_INFO -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/gfx/layers -I/builds/worker/workspace/build/src/obj-firefox/gfx/layers -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/media/libyuv/libyuv/include -I/builds/worker/workspace/build/src/gfx/skia -I/builds/worker/workspace/build/src/gfx/skia/skia/include/config -I/builds/worker/workspace/build/src/gfx/skia/skia/include/core -I/builds/worker/workspace/build/src/gfx/skia/skia/include/gpu -I/builds/worker/workspace/build/src/gfx/skia/skia/include/utils -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/obj-firefox/dist/include/cairo -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -Wno-maybe-uninitialized -MD -MP -MF .deps/Unified_cpp_gfx_layers1.o.pp /builds/worker/workspace/build/src/obj-firefox/gfx/layers/Unified_cpp_gfx_layers1.cpp [task 2018-06-26T00:36:31.044Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.060Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:31.060Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:31.060Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:31.060Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:31.400Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.401Z] 00:36:31 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_gfx_layers10.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DOS_POSIX=1 -DOS_LINUX=1 -DD3D_DEBUG_INFO -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/gfx/layers -I/builds/worker/workspace/build/src/obj-firefox/gfx/layers -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/media/libyuv/libyuv/include -I/builds/worker/workspace/build/src/gfx/skia -I/builds/worker/workspace/build/src/gfx/skia/skia/include/config -I/builds/worker/workspace/build/src/gfx/skia/skia/include/core -I/builds/worker/workspace/build/src/gfx/skia/skia/include/gpu -I/builds/worker/workspace/build/src/gfx/skia/skia/include/utils -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/obj-firefox/dist/include/cairo -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -Wno-maybe-uninitialized -MD -MP -MF .deps/Unified_cpp_gfx_layers10.o.pp /builds/worker/workspace/build/src/obj-firefox/gfx/layers/Unified_cpp_gfx_layers10.cpp [task 2018-06-26T00:36:31.401Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.417Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.417Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.417Z] 00:36:31 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:31.417Z] 00:36:31 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/layers' [task 2018-06-26T00:36:32.589Z] 00:36:32 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:32.591Z] 00:36:32 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_dom_base0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base0.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base0.cpp [task 2018-06-26T00:36:32.591Z] 00:36:32 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:32.608Z] 00:36:32 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:32.608Z] 00:36:32 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:32.608Z] 00:36:32 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:32.608Z] 00:36:32 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:33.755Z] 00:36:33 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/base' [task 2018-06-26T00:36:33.756Z] 00:36:33 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_dom_base4.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DHAVE_SIDEBAR -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/obj-firefox/dom/base -I/builds/worker/workspace/build/src/dom/battery -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/media -I/builds/worker/workspace/build/src/dom/network -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/file -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/html -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/dom/svg -I/builds/worker/workspace/build/src/dom/u2f -I/builds/worker/workspace/build/src/dom/xbl -I/builds/worker/workspace/build/src/dom/xml -I/builds/worker/workspace/build/src/dom/xslt/xpath -I/builds/worker/workspace/build/src/dom/xul -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/image -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/js/xpconnect/wrappers -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/layout/forms -I/builds/worker/workspace/build/src/layout/generic -I/builds/worker/workspace/build/src/layout/style -I/builds/worker/workspace/build/src/layout/svg -I/builds/worker/workspace/build/src/layout/xul -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/security/manager/ssl -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/netwerk/sctp/datachannel -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_base4.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp [task 2018-06-26T00:36:33.756Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:11:0, [task 2018-06-26T00:36:33.756Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.757Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:22:57: error: ISO C++ forbids declaration of 'NS_DECL_CYCLE_COLLECTION_CLASS' with no type [-fpermissive] [task 2018-06-26T00:36:33.757Z] 00:36:33 INFO - NS_DECL_CYCLE_COLLECTION_CLASS(SelectionChangeListener) [task 2018-06-26T00:36:33.757Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.757Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:22:57: error: expected ';' at end of member declaration [task 2018-06-26T00:36:33.759Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:36:5: error: 'nsCOMPtr' does not name a type [task 2018-06-26T00:36:33.760Z] 00:36:33 INFO - nsCOMPtr<nsINode> mStartContainer; [task 2018-06-26T00:36:33.760Z] 00:36:33 INFO - ^~~~~~~~ [task 2018-06-26T00:36:33.760Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:37:5: error: 'nsCOMPtr' does not name a type [task 2018-06-26T00:36:33.760Z] 00:36:33 INFO - nsCOMPtr<nsINode> mEndContainer; [task 2018-06-26T00:36:33.760Z] 00:36:33 INFO - ^~~~~~~~ [task 2018-06-26T00:36:33.761Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:44:33: error: 'nsRange' does not name a type [task 2018-06-26T00:36:33.761Z] 00:36:33 INFO - explicit RawRangeData(const nsRange* aRange); [task 2018-06-26T00:36:33.761Z] 00:36:33 INFO - ^~~~~~~ [task 2018-06-26T00:36:33.761Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:45:23: error: 'nsRange' does not name a type [task 2018-06-26T00:36:33.762Z] 00:36:33 INFO - bool Equals(const nsRange* aRange); [task 2018-06-26T00:36:33.762Z] 00:36:33 INFO - ^~~~~~~ [task 2018-06-26T00:36:33.762Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:49:3: error: 'nsTArray' does not name a type [task 2018-06-26T00:36:33.762Z] 00:36:33 INFO - nsTArray<RawRangeData> mOldRanges; [task 2018-06-26T00:36:33.763Z] 00:36:33 INFO - ^~~~~~~~ [task 2018-06-26T00:36:33.763Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:0: [task 2018-06-26T00:36:33.763Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:24:1: error: prototype for 'mozilla::dom::SelectionChangeListener::RawRangeData::RawRangeData(const nsRange*)' does not match any in class 'mozilla::dom::SelectionChangeListener::RawRangeData' [task 2018-06-26T00:36:33.763Z] 00:36:33 INFO - SelectionChangeListener::RawRangeData::RawRangeData(const nsRange* aRange) [task 2018-06-26T00:36:33.764Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.764Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:11:0, [task 2018-06-26T00:36:33.765Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.766Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:29:10: error: candidates are: constexpr mozilla::dom::SelectionChangeListener::RawRangeData::RawRangeData(mozilla::dom::SelectionChangeListener::RawRangeData&&) [task 2018-06-26T00:36:33.767Z] 00:36:33 INFO - struct RawRangeData [task 2018-06-26T00:36:33.768Z] 00:36:33 INFO - ^~~~~~~~~~~~ [task 2018-06-26T00:36:33.769Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:29:10: error: constexpr mozilla::dom::SelectionChangeListener::RawRangeData::RawRangeData(const mozilla::dom::SelectionChangeListener::RawRangeData&) [task 2018-06-26T00:36:33.773Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:44:14: error: mozilla::dom::SelectionChangeListener::RawRangeData::RawRangeData(const int*) [task 2018-06-26T00:36:33.774Z] 00:36:33 INFO - explicit RawRangeData(const nsRange* aRange); [task 2018-06-26T00:36:33.775Z] 00:36:33 INFO - ^~~~~~~~~~~~ [task 2018-06-26T00:36:33.775Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:0: [task 2018-06-26T00:36:33.776Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:38:1: error: prototype for 'bool mozilla::dom::SelectionChangeListener::RawRangeData::Equals(const nsRange*)' does not match any in class 'mozilla::dom::SelectionChangeListener::RawRangeData' [task 2018-06-26T00:36:33.777Z] 00:36:33 INFO - SelectionChangeListener::RawRangeData::Equals(const nsRange* aRange) [task 2018-06-26T00:36:33.778Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.778Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:11:0, [task 2018-06-26T00:36:33.779Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.780Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:45:10: error: candidate is: bool mozilla::dom::SelectionChangeListener::RawRangeData::Equals(const int*) [task 2018-06-26T00:36:33.780Z] 00:36:33 INFO - bool Equals(const nsRange* aRange); [task 2018-06-26T00:36:33.781Z] 00:36:33 INFO - ^~~~~~ [task 2018-06-26T00:36:33.782Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:0: [task 2018-06-26T00:36:33.783Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp: In function 'void ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback&, mozilla::dom::SelectionChangeListener::RawRangeData&, const char*, uint32_t)': [task 2018-06-26T00:36:33.790Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:58:49: error: 'struct mozilla::dom::SelectionChangeListener::RawRangeData' has no member named 'mStartContainer' [task 2018-06-26T00:36:33.791Z] 00:36:33 INFO - ImplCycleCollectionTraverse(aCallback, aField.mStartContainer, [task 2018-06-26T00:36:33.794Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.794Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:60:49: error: 'struct mozilla::dom::SelectionChangeListener::RawRangeData' has no member named 'mEndContainer' [task 2018-06-26T00:36:33.795Z] 00:36:33 INFO - ImplCycleCollectionTraverse(aCallback, aField.mEndContainer, [task 2018-06-26T00:36:33.795Z] 00:36:33 INFO - ^~~~~~~~~~~~~ [task 2018-06-26T00:36:33.795Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:33:0, [task 2018-06-26T00:36:33.795Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:10, [task 2018-06-26T00:36:33.796Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/OwningNonNull.h:12, [task 2018-06-26T00:36:33.796Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RootedOwningNonNull.h:20, [task 2018-06-26T00:36:33.796Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:20, [task 2018-06-26T00:36:33.796Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventTarget.h:10, [task 2018-06-26T00:36:33.797Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/BasicEvents.h:11, [task 2018-06-26T00:36:33.797Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Event.h:11, [task 2018-06-26T00:36:33.797Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AsyncEventDispatcher.h:12, [task 2018-06-26T00:36:33.797Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:13, [task 2018-06-26T00:36:33.798Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.798Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp: At global scope: [task 2018-06-26T00:36:33.798Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionNoteChild.h:40:9: error: 'cycleCollection' in 'class mozilla::dom::SelectionChangeListener' does not name a type [task 2018-06-26T00:36:33.798Z] 00:36:33 INFO - cycleCollection [task 2018-06-26T00:36:33.799Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.799Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:920:10: note: in expansion of macro 'NS_CYCLE_COLLECTION_INNERCLASS' [task 2018-06-26T00:36:33.799Z] 00:36:33 INFO - _class::NS_CYCLE_COLLECTION_INNERCLASS _class::NS_CYCLE_COLLECTION_INNERNAME; [task 2018-06-26T00:36:33.799Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.800Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:64:1: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_CLASS' [task 2018-06-26T00:36:33.800Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_CLASS(SelectionChangeListener) [task 2018-06-26T00:36:33.800Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.800Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionNoteChild.h:40:9: error: 'mozilla::dom::SelectionChangeListener::cycleCollection' has not been declared [task 2018-06-26T00:36:33.801Z] 00:36:33 INFO - cycleCollection [task 2018-06-26T00:36:33.801Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.801Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:287:17: note: in expansion of macro 'NS_CYCLE_COLLECTION_INNERCLASS' [task 2018-06-26T00:36:33.801Z] 00:36:33 INFO - _class::NS_CYCLE_COLLECTION_INNERCLASS [task 2018-06-26T00:36:33.802Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.818Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:450:3: note: in expansion of macro 'NS_CYCLE_COLLECTION_CLASSNAME' [task 2018-06-26T00:36:33.818Z] 00:36:33 INFO - NS_CYCLE_COLLECTION_CLASSNAME(_class)::Unlink(void *p) \ [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:66:1: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN' [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(SelectionChangeListener) [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:0: [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp: In function 'void Unlink(void*)': [task 2018-06-26T00:36:33.819Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:67:8: error: 'class mozilla::dom::SelectionChangeListener' has no member named 'mOldRanges' [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - tmp->mOldRanges.Clear(); [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - ^~~~~~~~~~ [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:33:0, [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:10, [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/OwningNonNull.h:12, [task 2018-06-26T00:36:33.820Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RootedOwningNonNull.h:20, [task 2018-06-26T00:36:33.821Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:20, [task 2018-06-26T00:36:33.821Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventTarget.h:10, [task 2018-06-26T00:36:33.821Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/BasicEvents.h:11, [task 2018-06-26T00:36:33.822Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Event.h:11, [task 2018-06-26T00:36:33.822Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AsyncEventDispatcher.h:12, [task 2018-06-26T00:36:33.822Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:13, [task 2018-06-26T00:36:33.822Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.823Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp: At global scope: [task 2018-06-26T00:36:33.823Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionNoteChild.h:40:9: error: 'mozilla::dom::SelectionChangeListener::cycleCollection' has not been declared [task 2018-06-26T00:36:33.823Z] 00:36:33 INFO - cycleCollection [task 2018-06-26T00:36:33.823Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.823Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:287:17: note: in expansion of macro 'NS_CYCLE_COLLECTION_INNERCLASS' [task 2018-06-26T00:36:33.824Z] 00:36:33 INFO - _class::NS_CYCLE_COLLECTION_INNERCLASS [task 2018-06-26T00:36:33.824Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.824Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:489:3: note: in expansion of macro 'NS_CYCLE_COLLECTION_CLASSNAME' [task 2018-06-26T00:36:33.824Z] 00:36:33 INFO - NS_CYCLE_COLLECTION_CLASSNAME(_class)::TraverseNative \ [task 2018-06-26T00:36:33.825Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.825Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:495:3: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL' [task 2018-06-26T00:36:33.825Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(_class) \ [task 2018-06-26T00:36:33.826Z] 00:36:33 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [task 2018-06-26T00:36:33.826Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:70:1: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN' [task 2018-06-26T00:36:33.826Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(SelectionChangeListener) [task 2018-06-26T00:36:33.838Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.839Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/dom/base/nsWrapperCache.h:10:0, [task 2018-06-26T00:36:33.839Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventTarget.h:13, [task 2018-06-26T00:36:33.839Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/BasicEvents.h:11, [task 2018-06-26T00:36:33.840Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Event.h:11, [task 2018-06-26T00:36:33.840Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AsyncEventDispatcher.h:12, [task 2018-06-26T00:36:33.841Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:13, [task 2018-06-26T00:36:33.841Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.841Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp: In function 'nsresult TraverseNative(void*, nsCycleCollectionTraversalCallback&)': [task 2018-06-26T00:36:33.842Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:496:50: error: 'nsCycleCollectingAutoRefCnt mozilla::dom::SelectionChangeListener::mRefCnt' is protected within this context [task 2018-06-26T00:36:33.843Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_DESCRIBE(_class, tmp->mRefCnt.get()) [task 2018-06-26T00:36:33.844Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.846Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:485:31: note: in definition of macro 'NS_IMPL_CYCLE_COLLECTION_DESCRIBE' [task 2018-06-26T00:36:33.849Z] 00:36:33 INFO - cb.DescribeRefCountedNode(_refcnt, #_class); [task 2018-06-26T00:36:33.849Z] 00:36:33 INFO - ^~~~~~~ [task 2018-06-26T00:36:33.850Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:70:1: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN' [task 2018-06-26T00:36:33.851Z] 00:36:33 INFO - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(SelectionChangeListener) [task 2018-06-26T00:36:33.852Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.852Z] 00:36:33 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsUtils.h:14:0, [task 2018-06-26T00:36:33.853Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupports.h:77, [task 2018-06-26T00:36:33.854Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISelectionListener.h:10, [task 2018-06-26T00:36:33.855Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:10, [task 2018-06-26T00:36:33.856Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.cpp:11, [task 2018-06-26T00:36:33.856Z] 00:36:33 INFO - from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2: [task 2018-06-26T00:36:33.856Z] 00:36:33 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:422:31: note: declared protected here [task 2018-06-26T00:36:33.857Z] 00:36:33 INFO - nsCycleCollectingAutoRefCnt mRefCnt; \ [task 2018-06-26T00:36:33.858Z] 00:36:33 INFO - ^ [task 2018-06-26T00:36:33.858Z] 00:36:33 INFO - /builds/worker/workspace/build/src/dom/base/SelectionChangeListener.h:21:3: note: in expansion of macro 'NS_DECL_CYCLE_COLLECTING_ISUPPORTS' [task 2018-06-26T00:36:33.859Z] 00:36:33 INFO - NS_DECL_CYCLE_COLLECTING_ISUPPORTS

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a5977e0708ea Add new globals to lint config r=standard8 https://hg.mozilla.org/integration/autoland/rev/df9a67c58183 Implement new Places Observers interface r=mrbkap https://hg.mozilla.org/integration/autoland/rev/8ed32495b46f Update onVisits uses to 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/b270d4a01986 Update onVisits tests to use 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/c89b86622d38 Fix unified sources build errors r=mrbkap

Backed out 5 changesets (bug 1340498) for build bustages on SelectionChangeListener.h on a CLOSED TREE Backout revision https://hg.mozilla.org/integration/autoland/rev/83d286f5134fd374fc4526093afd1be183bd868f Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c89b86622d3885cb731dcc52274a35a7d626159b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Link to a recent log https://treeherder.mozilla.org/logviewer.html#?job_id=184846364&repo=autoland

Okay - this should actually be good to land now *crosses fingers*. Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5be1e19b27c71d4bd0632e0a1cd07cac7c4c89f&selectedJob=350636build-linux64-asan-fuzzing%2Fopt

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa40c179eb0d Add new globals to lint config r=standard8 https://hg.mozilla.org/integration/autoland/rev/21d8c1fbbbd1 Implement new Places Observers interface r=mrbkap https://hg.mozilla.org/integration/autoland/rev/f8c799971f81 Update onVisits uses to 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/63321093bb70 Update onVisits tests to use 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/9ebcdb66ceff Fix unified sources build errors r=mrbkap

Backed out 5 changesets (bug 1340498) for build bustages on nsDOMCSSAttrDeclaration.h Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ebcdb66ceff32ce486e66c285919b7b43dc3480 Backout link: https://hg.mozilla.org/integration/autoland/rev/84b4c46d0042ba903636c9f63aeaba74d0ed7049 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=184991979&repo=autoland&lineNumber=19487

Ugh, the header cargo culting situation in dom/base is just awful. My change knocked a cargo culted "using namespace mozilla" out of a file in Olli's change today (https://hg.mozilla.org/integration/autoland/rev/e8c0ffefb34f). Trying again.

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/79a8619bd3e2 Add new globals to lint config r=standard8 https://hg.mozilla.org/integration/autoland/rev/515bb5e24dd7 Implement new Places Observers interface r=mrbkap https://hg.mozilla.org/integration/autoland/rev/5fcd31c65fe0 Update onVisits uses to 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/f950a2310e26 Update onVisits tests to use 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/28bedb658af4 Fix unified sources build errors r=mrbkap

FYI This caused some perf regressions only for the *debug* builds: == Change summary for alert #14038 (as of Tue, 26 Jun 2018 19:09:51 GMT) == Regressions: 1% compiler_metrics num_static_constructors windows2012-32 debug 861.00 -> 867.00 1% compiler_metrics num_static_constructors windows2012-64 debug 872.00 -> 878.00 1% compiler_metrics num_static_constructors osx-cross debug 196.00 -> 197.00 0% compiler_metrics num_static_constructors linux32 debug 241.00 -> 242.00 0% compiler_metrics num_static_constructors linux64 debug 241.00 -> 242.00 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14038

https://hg.mozilla.org/mozilla-central/rev/79a8619bd3e2 https://hg.mozilla.org/mozilla-central/rev/515bb5e24dd7 https://hg.mozilla.org/mozilla-central/rev/5fcd31c65fe0 https://hg.mozilla.org/mozilla-central/rev/f950a2310e26 https://hg.mozilla.org/mozilla-central/rev/28bedb658af4

Backed out 5 changesets (bug 1340498) for build bustage due to conflicts with bug 1470325. a=backout Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=e5fd22d489871a43a2ba79b90691db5777241f9a&selectedJob=185121419 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185121455&repo=mozilla-central Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=9c7bb8874337c2d40aef3d9945b10490a5115188

Pushed by dothayer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4a9b4cc6cb5 Add new globals to lint config r=standard8 https://hg.mozilla.org/integration/autoland/rev/43edbd931929 Implement new Places Observers interface r=mrbkap https://hg.mozilla.org/integration/autoland/rev/b6f047709e8e Update onVisits uses to 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/07d953128a21 Update onVisits tests to use 'page-visited' r=mak https://hg.mozilla.org/integration/autoland/rev/c78fe269ab6e Fix unified sources build errors r=mrbkap

The relanding brought back the same regressions mentioned in comment 104: == Change summary for alert #14061 (as of Wed, 27 Jun 2018 15:17:15 GMT) == Regressions: 1% compiler_metrics num_static_constructors windows2012-32 debug 861.00 -> 867.00 1% compiler_metrics num_static_constructors windows2012-64 debug 872.00 -> 878.00 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14061

https://hg.mozilla.org/mozilla-central/rev/a4a9b4cc6cb5 https://hg.mozilla.org/mozilla-central/rev/43edbd931929 https://hg.mozilla.org/mozilla-central/rev/b6f047709e8e https://hg.mozilla.org/mozilla-central/rev/07d953128a21 https://hg.mozilla.org/mozilla-central/rev/c78fe269ab6e

Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/c207dd88511dc6d725abe61062ee6638710aa942 chore(mc): Port Bug 1340498 - Remove onVisits from PlacesFeed (#4221)