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

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: wbamberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla51
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 verified)

Details

(Whiteboard: [history]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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).
(Assignee)

Comment 1

a year ago
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.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 51.2 - Aug 29
Priority: -- → P1
Whiteboard: [history]triaged
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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
Attachment #8781549 - Flags: review?(aswan) → review+
(Assignee)

Comment 4

a year ago
Thanks Andrew. Try run is looking good as well.
Keywords: checkin-needed

Comment 5

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/366b3ac46777
history.onVisitRemoved argument is wrong after a call to history.deleteAll, r=aswan
Keywords: checkin-needed

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/366b3ac46777
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 8

a year ago
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.
Attachment #8781549 - Flags: approval-mozilla-aurora?
Hello Will, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(wbamberg)
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+
Attachment #8781549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

a year ago
status-firefox50: --- → affected
(Reporter)

Comment 11

a year ago
(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).
Flags: needinfo?(wbamberg)
(Reporter)

Updated

a year ago
Status: RESOLVED → VERIFIED
> Yes! Verified as fixed on 51.0a1 (2016-08-22).

Great Thanks!
status-firefox51: fixed → verified

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/35a1c9a9d263
status-firefox50: affected → fixed
Flags: in-testsuite+
(Assignee)

Comment 14

a year ago
Thanks Ryan.
You need to log in before you can comment on or make changes to this bug.