Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: opt into location services once device service has been started #14253

Merged
merged 3 commits into from
Aug 23, 2018

Conversation

deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Aug 22, 2018

Description of Change

The global permission for opting into geolocation services was trying to get initialized before the device service was started, this patch fixes it by initializing it as a part of the permission request as every geolocation api call is guaranteed to hit the permission manager before further processing.

Fixes #14111
Depends on electron/libchromiumcontent#659

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
Release Notes

Notes: opt into location services once device service has been started

Copy link
Contributor

@alexeykuzmin alexeykuzmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test of the geolocation API?

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could add a test, but sadly I'm 99% sure new payment models for API keys mean that we currently can't :(

Let's add a follow-up issue to see if we can circle back to and address that but for now if it's locally tested to work i'd rather get it out in the next beta

@deepak1556
Copy link
Member Author

@codebytere its possible to mock the geolocation response for our tests, will update the PR shortly.

@deepak1556 deepak1556 force-pushed the geolocation_mojo_patch branch 2 times, most recently from 85a0553 to 8141cd5 Compare August 22, 2018 21:52
@deepak1556
Copy link
Member Author

deepak1556 commented Aug 22, 2018

@alexeykuzmin have added the specs, #14263 needs to be verified and merged before this PR since the specs and implementation rely on it.

@codebytere codebytere merged commit bce5bd8 into master Aug 23, 2018
@release-clerk
Copy link

release-clerk bot commented Aug 23, 2018

Release Notes Persisted

opt into location services once device service has been started

@trop
Copy link
Contributor

trop bot commented Aug 23, 2018

An error occurred while attempting to backport this PR to "3-0-x",
you will need to perform this backport manually

@deepak1556 deepak1556 deleted the geolocation_mojo_patch branch August 23, 2018 15:53
codebytere pushed a commit that referenced this pull request Aug 24, 2018
…14253)

* fix: opt into location services once device service has been started

* refactor: provide fake location provider to mock geolocation reponses

* chore: add spec for navigator.geolocation api using fake location provider
codebytere pushed a commit that referenced this pull request Aug 24, 2018
…14253)

* fix: opt into location services once device service has been started

* refactor: provide fake location provider to mock geolocation reponses

* chore: add spec for navigator.geolocation api using fake location provider
codebytere added a commit that referenced this pull request Aug 24, 2018
…(backport: 3-0-x) (#14289)

* fix: opt into location services once device service has been started (#14253)

* fix: opt into location services once device service has been started

* refactor: provide fake location provider to mock geolocation reponses

* chore: add spec for navigator.geolocation api using fake location provider

* fix conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

geolocation.getCurrentPosition callback isn't called
4 participants