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

Implement CefGeolocationHandler::OnRequestGeolocationPermission #768

Merged
merged 3 commits into from
Jan 28, 2015
Merged

Implement CefGeolocationHandler::OnRequestGeolocationPermission #768

merged 3 commits into from
Jan 28, 2015

Conversation

pushplay
Copy link
Contributor

delegating the request to a CefSharp.IGeolocationHandler implementation. This allows CefSharp to permit geolocation requests.

This implements #332 .

…ating the request to a CefSharp.IGeolocationHandler implementation. This allows CefSharp to permit geolocation requests.
@pushplay
Copy link
Contributor Author

Geolocation requests with the option {enableHighAccuracy: false} work as expected. A call to the google api domain is made which fails if the google api key isn't set. There's information about setting the google api key here: http://www.chromium.org/developers/how-tos/api-keys . I can add a wiki entry on the subject once (if?) the PR is accepted.

I haven't yet confirmed {enableHighAccuracy: true} working as expected on a device with GPS as I don't have one available to test with. I might be able to get one in the future.

@amaitland amaitland added this to the 39.0.0 milestone Jan 27, 2015
@amaitland
Copy link
Member

I can add a wiki entry on the subject once (if?) the PR is accepted.

@pushplay Looks great, let's merge this before the 39 release 👍 A wiki entry would be great 👍

return false;
}

if (handler->OnRequestGeolocationPermission(_browserControl, StringUtils::ToClr(requesting_url), request_id)) {
Copy link
Member

Choose a reason for hiding this comment

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

Brace on new line please 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amaitland done 👍

@amaitland
Copy link
Member

Thanks, will take it for a spin when I get a chance 👍

@jornh
Copy link
Contributor

jornh commented Jan 27, 2015

@pushplay excellent looking (first?) PR, 👍 really happy to have people doing stuff like that in the community 😄

@pushplay
Copy link
Contributor Author

It was totally selfish. I needed it to work, so I did it. :)

@jornh
Copy link
Contributor

jornh commented Jan 27, 2015

😄 I like that kind of selfishness

@jankurianski
Copy link
Member

I was able to get this going in so far as seeing my CefSharp geolocation API requests showing as 4xx client errors in the Google dev console... looks like I have to "contact sales" to get actual quota for that API 😄

  • I think you could add this as a commented-out part of CefExample.Init() to show how to set the keys:
// Set Google API keys, used for Geolocation requests.  See http://www.chromium.org/developers/how-tos/api-keys
// Environment.SetEnvironmentVariable("GOOGLE_API_KEY", "");
// Environment.SetEnvironmentVariable("GOOGLE_DEFAULT_CLIENT_ID", "");
// Environment.SetEnvironmentVariable("GOOGLE_DEFAULT_CLIENT_SECRET", "");
  • The example GeolocationHandler permission message could have a reference to setting API keys in CefExample.Init(), that way it is self-documenting. For example,

    {0} wants to use your computer's location. Allow?
    ** You must set your Google API key in CefExample.Init() for this to work. **

  • Is there any reason to have 2 separate copies of GeolocationHandler.cs instead of 1 in the CefSharp.Example project?

@jankurianski
Copy link
Member

BTW, really good PR! 👍

@amaitland
Copy link
Member

I think you could add this as a commented-out part of CefExample.Init() to show how to set the keys:

Nice touch, I like it 👍

Is there any reason to have 2 separate copies of GeolocationHandler.cs instead of 1 in the CefSharp.Example project?

Can you use the WinForms messagebox from a WPF app? Can't say that I've ever tried it.

@pushplay
Copy link
Contributor Author

Can you use the WinForms messagebox from a WPF app?

I haven't used UI elements from both in a project, but I have had references to both in a project and it leads to some confusing Intellisense results.

@jankurianski
Copy link
Member

Sorry, I misread the WPF GeolocationHandler as using the WinForms MessageBox already. Disregard that point 🙊 😄

@amaitland
Copy link
Member

@pushplay Do you mind making the first two changes @jankurianski suggested? After that I think this can be merged 👍

@pushplay
Copy link
Contributor Author

@amaitland Sure I'll look into it tomorrow.

@amaitland
Copy link
Member

@pushplay Fantastic thanks! 👍

@jankurianski When your happy feel free to test out that shiny new merge button 😄

jankurianski added a commit that referenced this pull request Jan 28, 2015
Implement CefGeolocationHandler::OnRequestGeolocationPermission
@jankurianski jankurianski merged commit 0f23b66 into cefsharp:master Jan 28, 2015
@jankurianski
Copy link
Member

Thanks a lot for your contribution @pushplay 👍
Also added your feature to the ChangeLog for the 39 release.

@amaitland
Copy link
Member

@pushplay Greatly appreciated 😄

@jankurianski excellent suggestions 👍

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.

4 participants