1340498 - Redesign the Places observers system

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

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).

Comment 1

[Alex Thayer [:alexical] (she/her)]

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.

Comment 2

[Marco Bonardo [:mak]]

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 :)

Comment 3

[Alex Thayer [:alexical] (she/her)]

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?

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Alex Thayer [:alexical] (she/her)]

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

Comment 6

[Alex Thayer [:alexical] (she/her)]

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

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Alex Thayer [:alexical] (she/her)]

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++).

Comment 9

[Alex Thayer [:alexical] (she/her)]

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.

Comment 10

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Implement new Places Observers interface

See the design doc[1] for further info. We would like to redesign
the places observer system to be more performant and more friendly
to consume. WebIDL was recommended as it simplifies creating simple
dictionary payloads while allowing dynamic typing with `any`.

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.

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++
was to use ProcessGlobal::Get(). My reasoning was that this would
be okay since it is only consumed from chrome code, but I do not
know well enough to say for certain. The exact details of the
consumption of this can be seen from the second commit in the
series.


[1] https://docs.google.com/document/d/1G45vfd6RXFXwNz7i4FV40lDCU0ao-JX_bZdgJV4tLjk/edit?usp=sharing

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

Comment 11

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Update onVisits uses to 'page-visited'

Consuming the new 'visit-added' notification was fairly trivial,
since it was already brought over to onVisits. There's not much to
say about this other than that I'm a little bit uncertain about
all the hoops we have to jump through to get a JSContext and
GlobalObject from History.cpp (which is discussed in the earlier
commit in the series).

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

Comment 12

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Update onVisits tests to use 'page-visited'

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

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Add new globals to lint config

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

Comment 14

[Alex Thayer [:alexical] (she/her)]

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 15

[Code Review Bot [:reviewbot]]

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 16

[Code Review Bot [:reviewbot]]

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]

Comment 17

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 18

[Alex Thayer [:alexical] (she/her)]

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

Comment 19

[Marco Bonardo [:mak]]

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.

Comment 20

[Alex Thayer [:alexical] (she/her)]

(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?

Comment 21

[Marco Bonardo [:mak]]

(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.

Comment 22

[Marco Bonardo [:mak]]

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

Comment 23

[Alex Thayer [:alexical] (she/her)]

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

Comment 24

[Marco Bonardo [:mak]]

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

Comment 25

[Alex Thayer [:alexical] (she/her)]

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 26

[Marco Bonardo [:mak]]

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 27

[Marco Bonardo [:mak]]

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 28

[Marco Bonardo [:mak]]

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 29

[Alex Thayer [:alexical] (she/her)]

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?

Comment 30

[Alex Thayer [:alexical] (she/her)]

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

Comment 31

[Marco Bonardo [:mak]]

(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 32

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

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

Comment 33

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

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

Comment 34

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

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

Comment 35

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

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

Comment 36

[Code Review Bot [:reviewbot]]

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 37

[Code Review Bot [:reviewbot]]

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]

Comment 38

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 40

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 41

[Marco Bonardo [:mak]]

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

Comment 42

[Marco Bonardo [:mak]]

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 43

[Marco Bonardo [:mak]]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

https://reviewboard.mozilla.org/r/220254/#review241344

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 45

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 46

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Fix unified sources build errors

Adding the Places* files into unified sources pushed the
unified sources into a situation that exposed a strangely
large number of errors. This seems to be the minimum set of
changes I could make to resolve all of the issues.

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

Comment 47

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

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

Comment 48

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

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

Comment 49

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

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

Comment 50

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

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

Comment 51

[Alex Thayer [:alexical] (she/her)]

(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.)

Comment 52

[Alex Thayer [:alexical] (she/her)]

Woops, forgot needinfo. See comment #51

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 54

[Blake Kaplan (:mrbkap) (inactive)]

(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 55

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8967828 [details]
Bug 1340498 - Fix unified sources build errors

https://reviewboard.mozilla.org/r/236512/#review242722

Comment 56

[Andrew Overholt [:overholt]]

Is there still a patch to review here?

Comment 57

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 58

[Mike de Boer [:mikedeboer]]

(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!

Comment 59

[Alex Thayer [:alexical] (she/her)]

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

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 61

[Alex Thayer [:alexical] (she/her)]

(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.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

(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 63

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 64

[Alex Thayer [:alexical] (she/her)]

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 65

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1340498 - Fix unified sources build errors

Adding the Places* files into unified sources pushed the
unified sources into a situation that exposed a strangely
large number of errors. This seems to be the minimum set of
changes I could make to resolve all of the issues.

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

Comment 66

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

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

Comment 67

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

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

Comment 68

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

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

Comment 69

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

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

Comment 70

[Code Review Bot [:reviewbot]]

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 71

[Code Review Bot [:reviewbot]]

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 72

[Blake Kaplan (:mrbkap) (inactive)]

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 73

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

https://reviewboard.mozilla.org/r/248656/#review258286

Comment 74

[Mark Banner (:standard8)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

https://reviewboard.mozilla.org/r/220256/#review258688

Comment 75

[Marco Bonardo [:mak]]

I assume we'll wait for the merge.

Comment 76

[Alex Thayer [:alexical] (she/her)]

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

Yup!

Comment 77

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

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

Comment 78

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

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

Comment 79

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

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

Comment 80

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

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

Comment 81

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

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

Comment 82

[Code Review Bot [:reviewbot]]

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]

Comment 83

[Pulsebot]

Pushed by [e-mail]:
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 84

[Code Review Bot [:reviewbot]]

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]

Comment 85

[Arthur Iakab [arthur_iakab]]

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

Comment 86

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220256/diff/5-6/

Comment 87

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220250/diff/5-6/

Comment 88

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220252/diff/5-6/

Comment 89

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220254/diff/5-6/

Comment 90

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

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

Comment 91

[Pulsebot]

Pushed by [e-mail]:
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

Comment 92

[Arthur Iakab [arthur_iakab]]

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

Comment 93

[Alex Thayer [:alexical] (she/her)]

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

Comment 94

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

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

Comment 95

[Pulsebot]

Pushed by [e-mail]:
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

Comment 96

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 97

[Alex Thayer [:alexical] (she/her)]

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.

Comment 98

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220256/diff/6-7/

Comment 99

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220250/diff/6-7/

Comment 100

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220252/diff/6-7/

Comment 101

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220254/diff/6-7/

Comment 102

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

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

Comment 103

[Pulsebot]

Pushed by [e-mail]:
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

Comment 104

[Ionuț Goldan [:igoldan]]

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

Comment 105

[Dorel Luca [:dluca]]

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

Comment 106

[Eliza Balazs [:ebalazs_]]

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

Comment 107

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951108 [details]
Bug 1340498 - Add new globals to lint config

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220256/diff/7-8/

Comment 108

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220250/diff/7-8/

Comment 109

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220252/diff/7-8/

Comment 110

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220254/diff/7-8/

Comment 111

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248656/diff/5-6/

Comment 112

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951105 [details]
Bug 1340498 - Implement new Places Observers interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220250/diff/8-9/

Comment 113

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951106 [details]
Bug 1340498 - Update onVisits uses to 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220252/diff/8-9/

Comment 114

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8951107 [details]
Bug 1340498 - Update onVisits tests to use 'page-visited'

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220254/diff/8-9/

Comment 115

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8982762 [details]
Bug 1340498 - Fix unified sources build errors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248656/diff/6-7/

Comment 116

[Pulsebot]

Pushed by [e-mail]:
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

Comment 117

[Ionuț Goldan [:igoldan]]

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

Comment 118

[Cosmin Sabou [:CosminS]]

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

Comment 119

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4221

Comment 120

[[github robot]]

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)