1218627 - Intermittent test_mozsettings.html | Geolocation didn't work after setting geolocation.enabled to true.

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/logviewer.html#?job_id=16261318&repo=mozilla-inbound

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Thanks Phil. If the issue persists, please reopen this.

Comment 2

[Phil Ringnalda (:philor)]

Impressively low-frequency intermittent, https://treeherder.mozilla.org/logviewer.html#?job_id=217344&repo=autoland

Comment 3

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

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

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 8768205 [details]
Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

https://reviewboard.mozilla.org/r/62510/#review59256

lgtm.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8fcd144a50
Fixing randomly failing test(s) in Geo. r=dougt

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef8fcd144a50

Comment 7

[Michelangelo De Simone [Offline] [:mds]]

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

Comment 8

[:gerard-majax]

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

Comment 9

[:gerard-majax]

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

Comment 10

[Ben Francis [:benfrancis]]

Hi, please back out this patch as it has caused a regression of an important feature of B2G OS.

Removing this entire feature is clearly not necessary in order to fix this incredibly intermittent test failure.

I think long term we can work together to remove the entire settings API, but this requires a proper discussion with stakeholders. Please don't simply remove code without asking the people who are using it.

Comment 11

[Doug Turner (:dougt)]

No backout.

B2G OS is tier 3.  The announcement is https://lists.mozilla.org/pipermail/dev-fxos/2016-January/002410.html.

Tier 3 platforms have a maintainer (you?) or community (you?) which attempt to keep the platform working.  As the module owner, I have no intention of supporting the Settings API as it adds untested complexity to Geolocation.  I suggest you fork if you want to continue supporting B2G -- which I thought you already did at the end of may?

Happy to discuss further -- but this bug isn't the right place.

Comment 12

[[:fabrice] Fabrice Desré]

It's great to see that we spend engineering time to fix intermittent failures that happen once every 9 months. I guess that means that this is one of our worst cases?

I'm gonna back this out, and let's see it that fails again before we have an acceptable solution for b2g.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0138788c4e
backout ef8fcd144a50

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b0138788c4e

Comment 15

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8768205 [details]
Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62510/diff/1-2/

Comment 16

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8768205 [details]
Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62510/diff/2-3/

Comment 17

[Michelangelo De Simone [Offline] [:mds]]

This was r+'ed and landed few months ago; later on it was backed out as for comment #12.

In light of [1], the patch can be landed. It has therefore been rebased; the original r+ from Doug stands.

[1] https://groups.google.com/forum/#!msg/mozilla.dev.fxos/FoAwifahNPY/Lppm0VHVBAAJ

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Patch doesn't apply cleanly anymore. Please rebase.

Comment 19

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8768205 [details]
Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62510/diff/3-4/

Comment 20

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)

> Patch doesn't apply cleanly anymore. Please rebase.

Rebased on top of b1d60f2f68c7. Thanks.

Comment 21

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8768205 [details]
Bug 1218627 - Removing MozSettings API from Geo (and fixing randomly failing tests).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62510/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d03efada4985
Remove MozSettings API from Geo (and fix randomly failing tests). r=dougt

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d03efada4985

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Should we uplift this anywhere or can it ride the trains?

Comment 25

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)

> Should we uplift this anywhere or can it ride the trains?

There's no urgency, it can ride with the trains, yes.:)
Thanks for asking, though.