updatePlaces handleCompletion doesn't return embed visits in its updatedCount argument

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
24 days ago
22 days ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

24 days ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1343182#c28

UpdatePlaces has this updatedCount that returns in handleCompletion, this doesn't account for embed visits, so it looks like no visit was added, when instead some were added.
And embed visits should not notify onError and onResult if those are ignored by the callback.
Comment hidden (mozreview-request)
Comment on attachment 8855290 [details]
Bug 1353783 - updatePlaces doesn't correctly notify and count embed visits.

https://reviewboard.mozilla.org/r/127166/#review129936

Code LGTM but I don't know about all the android stuff...

::: toolkit/components/places/tests/favicons/xpcshell.ini
(Diff revision 1)
> -# Bug 676989: test fails consistently on Android
> -fail-if = os == "android"

As noted on IRC, not sure what these removals are about. Are these just dead test annotations?
Attachment #8855290 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 3

23 days ago
(In reply to :Gijs from comment #2)
> ::: toolkit/components/places/tests/favicons/xpcshell.ini
> (Diff revision 1)
> > -# Bug 676989: test fails consistently on Android
> > -fail-if = os == "android"
> 
> As noted on IRC, not sure what these removals are about. Are these just dead
> test annotations?

My fault, I should have annotated that in the commit message.

Basically we can remove those for 2 reasons:
1. These are from xul fennec, the current Firefox for Android doesn't use Places at all.
2. in any case the same is repeated in the head part of the ini file, so no reason to have single annotations.
(Assignee)

Comment 4

23 days ago
(fwiw, the current Android build should not even build the places/ folder at all)
Comment hidden (mozreview-request)

Comment 6

22 days ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/64f302609496
updatePlaces doesn't correctly notify and count embed visits. r=Gijs

Comment 7

22 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64f302609496
Status: ASSIGNED → RESOLVED
Last Resolved: 22 days ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 8

22 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64f302609496
You need to log in before you can comment on or make changes to this bug.