Closed Bug 1353783 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

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 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+
(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.
(fwiw, the current Android build should not even build the places/ folder at all)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/64f302609496
updatePlaces doesn't correctly notify and count embed visits. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/64f302609496
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.