1295301 - history.onVisitRemoved argument is wrong after a call to history.deleteAll

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Will Bamberg [:wbamberg]]

If you add a listener to chrome.history.onVisitRemoved, it's passed a callback object containing a boolean `allHistory` and an array `urls`.

If the event fired because the history was all cleared, `allHistory` is true and `urls` should be an empty array (according to the Chrome docs[1] and Chrome behavior).

In Firefox, though, `urls` is undefined in this situation.

[1] https://developer.chrome.com/extensions/history#event-onVisitRemoved (although now I see that `urls` is also marked as optional, which makes no sense to me).

Comment 1

[Bob Silverberg [:bsilverberg]]

I think I implemented it this way precisely because `urls` is defined as optional. However, the comment in the Chrome docs about `allHistory`: "True if all history was removed. If true, then urls will be empty.", plus the fact that Chrome does in fact return an empty array, suggests that this is a bug that should be fixed. I will change the `urls` property to not be optional and will return an empty array, as per Chrome.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1295301 - history.onVisitRemoved argument is wrong after a call to history.deleteAll,

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8781549 [details]
Bug 1295301 - history.onVisitRemoved argument is wrong after a call to history.deleteAll,

https://reviewboard.mozilla.org/r/71966/#review69606

Comment 4

[Bob Silverberg [:bsilverberg]]

Thanks Andrew. Try run is looking good as well.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/366b3ac46777
history.onVisitRemoved argument is wrong after a call to history.deleteAll, r=aswan

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/366b3ac46777

Comment 7

[Bob Silverberg [:bsilverberg]]

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

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8781549 [details]
Bug 1295301 - history.onVisitRemoved argument is wrong after a call to history.deleteAll,

Approval Request Comment
[Feature/regressing bug #]: 1295301
[User impact if declined]: There is an incompatibility between the initial implementation of this method and how it is implemented in Chrome. This fixes that incompatibility.
[Describe test coverage new/current, TreeHerder]: This patch includes a test for this feature. A try run against Aurora has been submitted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c212f31a55c
[Risks and why]: This only affects the history API and is a very small change, therefore the risk is low.
[String/UUID change made/needed]: None.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Will, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8781549 [details]
Bug 1295301 - history.onVisitRemoved argument is wrong after a call to history.deleteAll,

Patch improves browser parity, includes a new test (yay!), Aurora50+

Comment 11

[Will Bamberg [:wbamberg]]

(In reply to Ritu Kothari (:ritu) from comment #9)
> Hello Will, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

Yes! Verified as fixed on 51.0a1 (2016-08-22).

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

> Yes! Verified as fixed on 51.0a1 (2016-08-22).

Great Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35a1c9a9d263

Comment 14

[Bob Silverberg [:bsilverberg]]

Thanks Ryan.