1278716 - Remove wakelocks in geo

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Comment 1

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

Attachment: Bug 1278716 - Remove wakelocks from geo.

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

Comment 2

[Doug Turner (:dougt)]

This is fine -- but maybe lets not remove the visibility test.  What we should do is collect telemetry when Geo is used in the background.  iirc, that was being done in another patch?

Comment 3

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

(In reply to Doug Turner (:dougt) from comment #2)

> This is fine -- but maybe lets not remove the visibility test.  What we
> should do is collect telemetry when Geo is used in the background.  iirc,
> that was being done in another patch?

Yes, we are already accumulating that: https://dxr.mozilla.org/mozilla-central/source/dom/geolocation/nsGeolocation.cpp?from=nsGeolocation.cpp%3A480#480
P

Comment 4

[Doug Turner (:dougt)]

I am asking a different question. I am suggesting that we not remove the code that tracks visibility changes and use this as an opportunity to understand how Geo is used in background tabs.

Comment 5

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

(In reply to Doug Turner (:dougt) from comment #4)

> I am asking a different question. I am suggesting that we not remove the
> code that tracks visibility changes and use this as an opportunity to
> understand how Geo is used in background tabs.

Makes sense.
If you don't mind I'm gonna go over that in a different bug.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 8760990 [details]
Bug 1278716 - Remove wakelocks from geo.

clearing flag -- if the gonk stuff is removed, this will be lgtm.

Comment 7

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

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

Comment 8

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

Comment on attachment 8760990 [details]
Bug 1278716 - Remove wakelocks from geo.

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

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 8760990 [details]
Bug 1278716 - Remove wakelocks from geo.

https://reviewboard.mozilla.org/r/58368/#review82930

It took me longer than I anticipated to figure out why removing the check for ContainsRequest is correct, but I've convinced myself that now requests will only have Allow called on them once, while previously they could have been suspended by the wakelock after Allow was called.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/503f80f3f2c2
Remove wakelocks from geo. r=jdm

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/503f80f3f2c2