1265845 - Implement browser.history.onVisited

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bob Silverberg [:bsilverberg]]

As per https://developer.chrome.com/extensions/history#event-onVisited

Comment 2

[Bob Silverberg [:bsilverberg]]

This will also cover  https://developer.chrome.com/extensions/history#event-onVisitRemoved

Comment 3

[Bob Silverberg [:bsilverberg]]

It looks like nsINavHistoryService does not currently have an event that we can use for `onVisitRemoved` [1]. There is an event for `onDeleteVisits` [2], but it looks like it is only called when a visit expires from history, not when a visit is manually removed. My testing verifies that this is the case. There is also `onDeleteURI` [3], but this is only called when a *page* is removed which can be because all of the visits for that page are removed. This method is not called when only some visits for a page are removed, so again, it won't give us what we need for `onVisitRemoved`. 

We can use `onClearHistory` [4] to deal with part of the requirements for `onVisitRemoved`, which wants to be notified when all history is removed, but it doesn't look like there is currently support for an observer which would fire whenever an individual visit is removed.

Marco, do you have any thoughts or comments about this?

[1] https://developer.chrome.com/extensions/history#event-onVisitRemoved
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#787
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#734
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#741

Comment 4

[Bob Silverberg [:bsilverberg]]

There's another issue for onVisited as well. The Chrome API expects a fully populated `HistoryItem` object [1], which means that we need to have the title, visitCount and typedCount. When the `onVisit` [2] observer event is fired it does not provide us with any of that info, so we either need to:

1) Enhance the onVisit event to provide that information, or
2) After we receive the event retrieve the info ourselves, which would mean retrieving a history entry for the uri in question.

If we are going to need to do #2, then perhaps we should look at implementing `fetch` [3] in History.jsm, as we've done for `insert`.

What do you think, Marco?

[1] https://developer.chrome.com/extensions/history#type-HistoryItem
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#644
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.jsm#130

Comment 5

[Bob Silverberg [:bsilverberg]]

Marco, do you have any thoughts about what we can/should do about supporting browser.history.onVisited and onVisitRemoved, based on my observations above?

Comment 6

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #6)

> I'd notify on both onDeleteURI and onDeleteVisits and onClearHistory for now
> and file a bug to move to onDeletePages when it will be available.
>

That makes sense if we only care about URLs, but I'm still not sure about that. See my other comment below.

> > This method is not called when only some visits for a page are removed, so
> > again, it won't give us what we need for `onVisitRemoved`. 
> 
> But the spec doesn't state it is invoked when only some visits are removed,
> it states it is fired when the URL is removed (and thus ALL of its visits).

I find the wording of the spec confusing. It does say "Fired when one or more URLs are removed from the history service", which suggests it should only be fired when a URL is removed, and not when just visits are removed but the URL remains. However, the next part says "When all visits have been removed the URL is purged from history." Why would it bother to say that if it doesn't mean to differentiate between visits being removed and URLs being removed? I will do some checking in Chrome itself to see if I can determine whether it fires for individual visit removal or only when an entire URL's history is removed.

Comment 7

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #7)
> 
> The problem is that when onVisit fires, we don't yet have a title. Indeed
> the docshell first registers the visit with global history (and we fire
> onVisit) and then only later goes through the DOM to find (and register) the
> title.
> We could likely add title, visitCount and typedCount to onVisit, but the
> title would be the old one, if there is one. Otherwise you should wait for
> an onTitleChanged notification, but it may even never come if the page has
> no title... Though, if done with a meaningful timeout it may work.
>
> I was just thinking this morning it would be far more efficient if the
> docshell would do a single call to global history including both the visit
> and the title, but I feel like that kind of refactoring is quite an
> expensive one.
> 
> 
> When we insert a visit, in general, we need to read the url info from the DB
> already (look at History.cpp) so it would be more efficient to improve
> onVisit than to read the same info later.
> I'm about to start working on bug 1209027, that touches History.cpp, and
> while there I could look if it would be hard to add typed and visitCount to
> the notification. Title is a different story, as I explained above.

Thanks Marco. I look forward to seeing if the missing data issue can be resolved via bug 1209027. I understand that if we want an accurate title we'll need to do something extra on top of what we'll get from onVisit.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> I find the wording of the spec confusing. It does say "Fired when one or
> more URLs are removed from the history service", which suggests it should
> only be fired when a URL is removed, and not when just visits are removed
> but the URL remains.

It depends on the history concept. I think in chrome history and bookmarks are 2 completely separated entities, thus an url being removed means an url being removed from history. When all visits are removed, the url is removed from history, the fact it is not removed from the db is an implementation detail.

However, the next part says "When all visits have been
> removed the URL is purged from history." Why would it bother to say that if
> it doesn't mean to differentiate between visits being removed and URLs being
> removed?

I think it's a poor wording for "if you/we remove the last visit for an url, the url is removed and this is notified.

Btw, testing the behavior will be better than relying on confusing wording.

Comment 9

[Bob Silverberg [:bsilverberg]]

I checked Chrome's behaviour and your assumption was correct: `onVisitRemoved` is fired when a URL is removed, not when individual visits for a URL are removed.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #3)
> It looks like nsINavHistoryService does not currently have an event that we
> can use for `onVisitRemoved` [1]. There is an event for `onDeleteVisits`
> [2], but it looks like it is only called when a visit expires from history,
> not when a visit is manually removed. My testing verifies that this is the
> case. There is also `onDeleteURI` [3], but this is only called when a *page*
> is removed which can be because all of the visits for that page are removed.

You are right, onDeleteVisits is invoked when ALL the visits to a page are removed, but the page was not, cause maybe it was bookmarked, while onDeleteURI is invoked when a page is removed (maybe cause all visits have been removed).
I agree the situation is not optimal.
In bug 937560 we started redesigning a new onDeletePages API, but it was mostly a "batched" notification, so that instead of getting hundreds or thousands of notifications, one could get just one. But it was not completed, cause along the way we found it was still ugly, we really would like to coalesce both onDeleteURI and onDeleteVisits into this new notification.

Btw, from reading the specs, onVisitRemoved states:
Fired when one or more URLs are removed from the history service. When all visits have been removed the URL is purged from history. 

For that we could already use the existing notifications, onClearHistory can indeed be used for allHistory, while we could (temporarily) send an onVisitRemoved notification for each onDeleteVisits.
The problematic part is onDeleteURI, this API indeed just stares the URI has been removed from the database, but not whether that happened due to its history being removed.

On the other side, it's likely consumers of these notifications need to be notified of removals to stop tracking urls they are following, so I don't think over-notifying would be a big deal.

I'd notify on both onDeleteURI and onDeleteVisits and onClearHistory for now and file a bug to move to onDeletePages when it will be available.

> This method is not called when only some visits for a page are removed, so
> again, it won't give us what we need for `onVisitRemoved`. 

But the spec doesn't state it is invoked when only some visits are removed, it states it is fired when the URL is removed (and thus ALL of its visits).

Comment 11

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> There's another issue for onVisited as well. The Chrome API expects a fully
> populated `HistoryItem` object [1], which means that we need to have the
> title, visitCount and typedCount. When the `onVisit` [2] observer event is
> fired it does not provide us with any of that info, so we either need to:
> 
> 1) Enhance the onVisit event to provide that information, or
> 2) After we receive the event retrieve the info ourselves, which would mean
> retrieving a history entry for the uri in question.

The problem is that when onVisit fires, we don't yet have a title. Indeed the docshell first registers the visit with global history (and we fire onVisit) and then only later goes through the DOM to find (and register) the title.
We could likely add title, visitCount and typedCount to onVisit, but the title would be the old one, if there is one. Otherwise you should wait for an onTitleChanged notification, but it may even never come if the page has no title... Though, if done with a meaningful timeout it may work.

I was just thinking this morning it would be far more efficient if the docshell would do a single call to global history including both the visit and the title, but I feel like that kind of refactoring is quite an expensive one.

> If we are going to need to do #2, then perhaps we should look at
> implementing `fetch` [3] in History.jsm, as we've done for `insert`.
> 
> What do you think, Marco?

When we insert a visit, in general, we need to read the url info from the DB already (look at History.cpp) so it would be more efficient to improve onVisit than to read the same info later.
I'm about to start working on bug 1209027, that touches History.cpp, and while there I could look if it would be hard to add typed and visitCount to the notification. Title is a different story, as I explained above.

Comment 12

[Bob Silverberg [:bsilverberg]]

This bug is now just for onVisited. I reopened the other bug that was going to be for onVisitRemoved.

Comment 13

[Bob Silverberg [:bsilverberg]]

Thanks for adding `visitCount` and typed to `onVisit`, Marco. I'm not sure what I am to do about `title` though. Earlier in the bug you mentioned that you could add `title` to onVisit as well [1], but that wasn't included in bug 1277235. Are you still planning to do that? You mentioned that I could listen for `onTitleChanged`, but I'm not sure how to do that in conjunction with listening for onVisit.

I guess what I'm asking here is, given the current code that exists, how would you suggest I go about figuring out the correct title when I want to emit the onVisited event after receiving the onVisit notification?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1265845#c7

Comment 14

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> Thanks for adding `visitCount` and typed to `onVisit`, Marco. I'm not sure
> what I am to do about `title` though. Earlier in the bug you mentioned that
> you could add `title` to onVisit as well [1], but that wasn't included in
> bug 1277235. Are you still planning to do that?

No, as I said we don't yet know the title at that time.
The only thing you could do is start a timer when you get onVisit, wait enough time (a few seconds?) so that an eventual title notification could come, and associate the two.
Otherwise, even if you'd fetch title when you get onVisit, you'd fetch the old title, or no title at all since it was not set yet.

Comment 15

[Bob Silverberg [:bsilverberg]]

Thanks Marco. Based on the hoops we'd have to jump through to make that work, and the questionability of the value of `title` to the `onVisited` event, we'd decided to just not provide a title at this time.

Comment 16

[Bob Silverberg [:bsilverberg]]

It seems that we're still discussing this. I checked Chrome's behaviour and it looks like it delivers the last known title to onVisited, so when you first visit a page it is often an empty title, but when you revisit a page, or reload, you get a title. Marco, knowing this, would you consider delivering the last known title, if one exists, to onVisit?

Comment 17

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1265845 - Implement browser.history.onVisited,

Note that this version just delivers an empty string for the title to onVisited. That may get revisited, but for now I'm treating this as a candidate patch.

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

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8761393 [details]
Bug 1265845 - Implement browser.history.onVisited,

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

Comment 19

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58606/#review55688

::: browser/components/extensions/ext-history.js:102
(Diff revision 2)
>      _observer = {
>        onDeleteURI: function(uri, guid, reason) {
>          this.emit("visitRemoved", {allHistory: false, urls: [uri.spec]});
>        },
> -      onVisit: function() {},
> +      onVisit: function(uri, visitId, time, sessionId, referringId, transitionType, guid, hidden, visitCount, typed) {
> +        PlacesUtils.promisePlaceInfo(guid).then(placeInfo => {

When I was poking around Places I found `PlacesUtils.promisePlaceInfo` which does the trick, so I've added that. As discussed, it might return an empty title, but that's no different from what Chrome does.

Comment 20

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/58606/#review56136

I'd like to understand the title handling before r+ing.  What you're doing seems fine, but given all the issues we discussed earlier is the test of it going to be reliable?

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

You're probably tired of this topic, but the last message I recall reading said title would be an empty string.  I assume that since you're doing this, it returns something other than an empty string?  What exactly does it return?

::: browser/components/extensions/test/browser/browser_ext_history.js:393
(Diff revision 2)
> +  function background() {
> +    let onVisitedData = [];
> +
> +    browser.history.onVisited.addListener(data => {
> +      onVisitedData.push(data);
> +      if (onVisitedData.length === 3) {

== is the standard here

::: browser/components/extensions/test/browser/browser_ext_history.js:415
(Diff revision 2)
> +  yield extension.awaitMessage("ready");
> +
> +  yield PlacesUtils.history.insertMany(PAGE_INFOS);
> +
> +  let onVisitedData = yield extension.awaitMessage("on-visited-data");
> +  is(onVisitedData.length, 3, "onVisited was called the expected number of times");

the way the background script is written, this test can never fail

Comment 21

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58606/#review56136

I do believe it will be reliable. It seems that calling `insertMany` sets the title appropriately, so I'm not worried about the reliability of the test.

> You're probably tired of this topic, but the last message I recall reading said title would be an empty string.  I assume that since you're doing this, it returns something other than an empty string?  What exactly does it return?

This will set the `title` to be whatever was returned as the title when the `PlacesUtils.promisePlaceInfo` call resolves. That may or may not be an empty string depending on whether history has been populated with a title yet. This seems to be the same behaviour as Chrome, which sometimes returns an empty string and sometimes does not.

> == is the standard here

Why is that? Wouldn't we always expect the `length` property to return a number, and therefore wouldn't `===` be appropriate? It seems to me that I see `===` used far more often in our code, so why is `==` the standard in this context?

> the way the background script is written, this test can never fail

Is it not possible that there could be a race condition which would allow `onVisitedData.length` to be other than `3`? I realize the likelihood of that is almost nil, but I think it may be worth keeping this one line of code as a safeguard.

Comment 22

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/58606/#review56136

> Why is that? Wouldn't we always expect the `length` property to return a number, and therefore wouldn't `===` be appropriate? It seems to me that I see `===` used far more often in our code, so why is `==` the standard in this context?

I can't really answer the "why" but see a few lines in here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
A quick count of `toolkit/components/extensions/*.jsm` finds more than 100 instances of == and fewer than 20 of ===

> Is it not possible that there could be a race condition which would allow `onVisitedData.length` to be other than `3`? I realize the likelihood of that is almost nil, but I think it may be worth keeping this one line of code as a safeguard.

what is the race?  the code in the background script that sends the message is conditional on `onVisistedData.length == 3`

Comment 23

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8761393 [details]
Bug 1265845 - Implement browser.history.onVisited,

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

Comment 24

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58606/#review56136

> I can't really answer the "why" but see a few lines in here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
> A quick count of `toolkit/components/extensions/*.jsm` finds more than 100 instances of == and fewer than 20 of ===

Fair enough. I thought I noticed it being the other way around, but I guess I was mistaken.

> what is the race?  the code in the background script that sends the message is conditional on `onVisistedData.length == 3`

I might just be being overly paranoid, and honestly, if I hadn't accidentally left the assertion in there from an earlier version of the test when it was needed I probably wouldn't have noticed it, but I do think there is a possible race condition that could happen if the `onVisited` listener were to fire between the time that the test for `onVisitedData.length == 3` passes and the message is sent back to the test. It's possible, although highly unlikely, that an extra item could be added to the `onVisitedData` array before the message is sent, resulting in an array in the test with a length that is greater than 3. Like I said, I probably wouldn't add the assertion if it weren't already there to deal with this unlikely race condition, but looking at the code it does seem like a reasonable safeguard with no real cost. If you feel strongly I will remove it - I don't really mind either way, but I wanted to explain why I think it could possibly be useful.

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8761393 [details]
Bug 1265845 - Implement browser.history.onVisited,

https://reviewboard.mozilla.org/r/58606/#review56228

Comment 26

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/58606/#review56136

> I might just be being overly paranoid, and honestly, if I hadn't accidentally left the assertion in there from an earlier version of the test when it was needed I probably wouldn't have noticed it, but I do think there is a possible race condition that could happen if the `onVisited` listener were to fire between the time that the test for `onVisitedData.length == 3` passes and the message is sent back to the test. It's possible, although highly unlikely, that an extra item could be added to the `onVisitedData` array before the message is sent, resulting in an array in the test with a length that is greater than 3. Like I said, I probably wouldn't add the assertion if it weren't already there to deal with this unlikely race condition, but looking at the code it does seem like a reasonable safeguard with no real cost. If you feel strongly I will remove it - I don't really mind either way, but I wanted to explain why I think it could possibly be useful.

But the code that checks the length and the code that sends the message is all synchronous, there's no way for some other code to run and modify the array in between those steps.
I don't feel strongly about taking out the test, just wanted to make sure we're in agreement about how this all works.

Comment 27

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8761393 [details]
Bug 1265845 - Implement browser.history.onVisited,

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

Comment 28

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58606/#review56136

> But the code that checks the length and the code that sends the message is all synchronous, there's no way for some other code to run and modify the array in between those steps.
> I don't feel strongly about taking out the test, just wanted to make sure we're in agreement about how this all works.

Thanks for the explanation. I have removed the unnecessary assertion.

Comment 29

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78ed658d1257

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8ea9433ab777
Implement browser.history.onVisited, r=aswan

Comment 31

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9979228&repo=fx-team

Comment 32

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/161aea49941f
Backed out changeset 8ea9433ab777 for test failures in browser_ext_history.js

Comment 33

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8761393 [details]
Bug 1265845 - Implement browser.history.onVisited,

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

Comment 34

[Bob Silverberg [:bsilverberg]]

Thanks Tomcat. I believe I have addressed the failure in my latest commit.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4258cf59c62f
Implement browser.history.onVisited, r=aswan

Comment 36

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/58606/#review55688

> When I was poking around Places I found `PlacesUtils.promisePlaceInfo` which does the trick, so I've added that. As discussed, it might return an empty title, but that's no different from what Chrome does.

Note that this way you are doing an I/O access on every visit notification, and I don't think it's a good idea cause you are slowing down all visits insertions.

Comment 37

[Marco Bonardo [:mak]]

that means you should remove that code, I'm sorry... in general you should avoid as much I/O as you can in notifications, especially the ones that directly affect the user browsing experience.

Comment 38

[Bob Silverberg [:bsilverberg]]

(In reply to Marco Bonardo [::mak] from comment #37)
> that means you should remove that code, I'm sorry... in general you should
> avoid as much I/O as you can in notifications, especially the ones that
> directly affect the user browsing experience.

Ok,then where does that leave us? What can I do to deliver a title as part of onVisited? Are we back to having to listen for both onVisit and onTitleChanged events, and synchonizing the two? Is it possible for onVisit to be changed to deliver the last known title without incurring this extra I/O? Or perhaps I could change our observer so that it only calls promisePlaceInfo if there is a listener for the onVisited event, which would prevent the extra I/O except in those cases where it is needed for the API. What do you think?

Comment 39

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #38)
> Ok,then where does that leave us? What can I do to deliver a title as part
> of onVisited? Are we back to having to listen for both onVisit and
> onTitleChanged events, and synchonizing the two?

That's still the best solution imo. You could (probably) do that easily using a Map (url => {title, addedTime}) and a timeout (you could assume the title may have been set 1s after visit notification).
On timeout you could check if a title has been received for the given url, then expire that url and any entries older than it (since all notifications are serialized). Maybe an array would make expiration easier, but that's an implementation detail.
There should be no problem in delaying a notification by 1s. 

Is it possible for onVisit
> to be changed to deliver the last known title without incurring this extra
> I/O?

Yes, but I don't see what's the use-case for that and why that would be better than always returning an empty title or a random string. Why should one care about which title a page had BEFORE being visited?

> Or perhaps I could change our observer so that it only calls
> promisePlaceInfo if there is a listener for the onVisited event, which would
> prevent the extra I/O except in those cases where it is needed for the API.

It would still slowdown page loads for any user with an add-on using this API, that is unlikely what we want.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4258cf59c62f

Comment 41

[Marco Bonardo [:mak]]

Even if you don't have a patch ready, I'd like to have at least a bug filed for the problem in comment 36, that we can track (since I'd not want to release the code as-is). Luckily we have enough time for that :)

Comment 42

[Bob Silverberg [:bsilverberg]]

I've already filed bug 1280357 for this. :)

Comment 43

[Kris Maglione [:kmag]]

(In reply to Marco Bonardo [::mak] from comment #39)
> (In reply to Bob Silverberg [:bsilverberg] from comment #38)
> > Ok,then where does that leave us? What can I do to deliver a title as part
> > of onVisited? Are we back to having to listen for both onVisit and
> > onTitleChanged events, and synchonizing the two?
>
> That's still the best solution imo.

There's no reason to do this. The Chrome version of this API only includes the
title when we already know it. And I don't expect that we'd get an
onTitleChanged event if the page's title doesn't change during the visit, so
we'd actually wind up doing the exact opposite of Chrome.

> > Is it possible for onVisit to be changed to deliver the last known title
> > without incurring this extra I/O?
>
> Yes, but I don't see what's the use-case for that and why that would be
> better than always returning an empty title or a random string. Why should
> one care about which title a page had BEFORE being visited?

They may have a use for the last-known title of a page. In any case, the
listener's contract is to return the full place info, as it's currently known.
Since we're already querying for it, and can return that without much
overhead, I think that we should.

Comment 44

[Bob Silverberg [:bsilverberg]]

(In reply to Kris Maglione [:kmag] from comment #43)
> (In reply to Marco Bonardo [::mak] from comment #39)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #38)
> > > Is it possible for onVisit to be changed to deliver the last known title
> > > without incurring this extra I/O?
> >
> > Yes, but I don't see what's the use-case for that and why that would be
> > better than always returning an empty title or a random string. Why should
> > one care about which title a page had BEFORE being visited?
> 
> They may have a use for the last-known title of a page. In any case, the
> listener's contract is to return the full place info, as it's currently
> known.
> Since we're already querying for it, and can return that without much
> overhead, I think that we should.

Kris, are you advocating changing the onVisit notification to provide the last known title? I'm just trying to keep track of what everyone is suggesting.

Comment 45

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #44)
> Kris, are you advocating changing the onVisit notification to provide the
> last known title? I'm just trying to keep track of what everyone is
> suggesting.

Yes.

Comment 46

[Bob Silverberg [:bsilverberg]]

Ok, sorry to keep bugging you with the same request, but, given the further discussion above, what do you think about implementing that, Marco?

Comment 47

[Marco Bonardo [:mak]]

if you are 110% sure Chrome is giving out the OLD title, let's just add title to onVisited. I still think that's a dumb API choice, but the cost is small and not worth blocking this. Shouldn't be hard to do that, if you need help ping me in the other bug, or in London before I escape :D

Comment 48

[Bob Silverberg [:bsilverberg]]

Thanks Marco. Kris suggested that in onVisit we could call it `lastKnownTitle`, as opposed to just `title`, to be clear in our API at least. I'm not able to make the change myself. Would you be able to do it?

Comment 49

[Kris Maglione [:kmag]]

I can do it. Please file a Places bug and assign it to me.

Comment 50

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/history/onVisited