1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Bob Silverberg [:bsilverberg]]

It was suggested that it is too expensive/degrading of performance to call PlacesUtils.promisePlaceInfo for every visit, so the code in the observer should be changed to not call PlacesUtils.promisePlaceInfo but rather attempt to wait for a notification of `onTitleChanged` and use the title from that to provide data for onVisited.

Comment 1

[Kris Maglione [:kmag]]

Waiting for `onTitleChanged` is not helpful. Chrome does not provide the title in cases where it isn't already known, so we shouldn't either.

Comment 2

[Bob Silverberg [:bsilverberg]]

That's true. So we should still track titles of known pages via onTitleChanged, and then use those values if they are known when onVisit fires?

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo,

Removing the call to PlacesUtils.promisePlaceInfo and just returning an empty title.

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

Comment 4

[Kris Maglione [:kmag]]

Let's leave this as-is until we decide what we're going to do next. We just need to make sure to deal with it before the next merge.

Comment 5

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ce0bae50787

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8763188 [details]
Bug 1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo,

https://reviewboard.mozilla.org/r/59486/#review56564

::: browser/components/extensions/ext-history.js:105
(Diff revision 1)
>        onVisit: function(uri, visitId, time, sessionId, referringId, transitionType, guid, hidden, visitCount, typed) {
> -        PlacesUtils.promisePlaceInfo(guid).then(placeInfo => {
> -          let data = {
> +        let data = {
> -            id: guid,
> +          id: guid,
> -            url: uri.spec,
> +          url: uri.spec,
> -            title: placeInfo.title,
> +          title: "",

Chrome documents title as an optional property, why don't we just omit it here?

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8763188 [details]
Bug 1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo,

We're going to approach this differently.

Comment 8

[Bob Silverberg [:bsilverberg]]

Kris, I'm only here for the next few days, and then off until Aug. 8th. We said that we didn't want this code to make it into 50, so here are some suggestions:

1. I can just add back in a patch which removes the call to promisePlaceInfo and returns "" for the title, and then we can fix the title issue after bug 1280601 lands.
2. If you can land bug 1280601 in the next couple of days I can update this to use the title from the event.
3. I can do nothing and leave it in your hands to both land bug 1280601 and update this code before the merge.

I think 1 is safest, and least demanding of your time, right now, but let me know what you think we should do.

Comment 9

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo,

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

Comment 10

[Bob Silverberg [:bsilverberg]]

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6214a17a0e43 looks good.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8772575 [details]
Bug 1280357 - Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo,

https://reviewboard.mozilla.org/r/65332/#review62648

Comment 12

[Bob Silverberg [:bsilverberg]]

Thanks Kris.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/17409ee90c86
Update browser.history.onVisited to not call PlacesUtils.promisePlaceInfo, r=kmag

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17409ee90c86