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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Places
P2
normal
RESOLVED FIXED
3 months ago
3 months 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

3 months 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 2

3 months ago
mozreview-review
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

3 months 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

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

Comment 6

3 months 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

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

Comment 8

3 months 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.