Bug 173150

Summary: Add API::GeolocationProvider
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 173151    
Attachments:
Description Flags
Patch
achristensen: review-
Rebased patch
achristensen: review+
Patch for landing none

Description Carlos Garcia Campos 2017-06-09 04:22:01 PDT
We will use this instead of the C API in GTK+.
Comment 1 Carlos Garcia Campos 2017-06-09 04:23:45 PDT
Created attachment 312423 [details]
Patch
Comment 2 Alex Christensen 2017-06-09 10:23:37 PDT
Comment on attachment 312423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312423&action=review

> Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:84
> +    std::unique_ptr<API::GeolocationProvider> m_provider;

I think this is wrong.  Usually we have the API object own the WebKit namespace object.
Comment 3 Carlos Garcia Campos 2017-06-12 10:21:59 PDT
Created attachment 312677 [details]
Rebased patch

It should apply now
Comment 4 Alex Christensen 2017-06-12 10:27:34 PDT
Comment on attachment 312677 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312677&action=review

> Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:50
> +    void setProvider(std::unique_ptr<API::GeolocationProvider>);

&&

> Source/WebKit2/UIProcess/WebGeolocationProvider.h:44
> +class WebGeolocationProvider : public API::GeolocationProvider,  API::Client<WKGeolocationProviderBase> {

extra space
Comment 5 Carlos Garcia Campos 2017-06-12 10:33:50 PDT
(In reply to Alex Christensen from comment #4)
> Comment on attachment 312677 [details]
> Rebased patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312677&action=review
> 
> > Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:50
> > +    void setProvider(std::unique_ptr<API::GeolocationProvider>);
> 
> &&

That was also my idea at first, but then again I followed what we do for all other clients. In this particular case, I think that what we have is equivalent to && because we always pass a std::make_unique<> (or nullptr) to the function, so there aren't copies anyway.

> > Source/WebKit2/UIProcess/WebGeolocationProvider.h:44
> > +class WebGeolocationProvider : public API::GeolocationProvider,  API::Client<WKGeolocationProviderBase> {
> 
> extra space

Good catch!
Comment 6 Darin Adler 2017-06-12 14:57:25 PDT
(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Alex Christensen from comment #4)
> > > Source/WebKit2/UIProcess/WebGeolocationManagerProxy.h:50
> > > +    void setProvider(std::unique_ptr<API::GeolocationProvider>);
> > 
> > &&
> 
> That was also my idea at first, but then again I followed what we do for all
> other clients. In this particular case, I think that what we have is
> equivalent to && because we always pass a std::make_unique<> (or nullptr) to
> the function, so there aren't copies anyway.

I don’t know if we ever resolved this. Here is what Scott Meyers has to say:

http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-passed.html
Comment 7 Carlos Garcia Campos 2017-06-12 22:54:01 PDT
Created attachment 312751 [details]
Patch for landing
Comment 8 Carlos Garcia Campos 2017-06-12 23:59:34 PDT
Committed r218165: <http://trac.webkit.org/changeset/218165>