Bug 173150 - Add API::GeolocationProvider
Summary: Add API::GeolocationProvider
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 173151
  Show dependency treegraph
 
Reported: 2017-06-09 04:22 PDT by Carlos Garcia Campos
Modified: 2017-06-12 23:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch (15.75 KB, patch)
2017-06-09 04:23 PDT, Carlos Garcia Campos
achristensen: review-
Details | Formatted Diff | Diff
Rebased patch (15.75 KB, patch)
2017-06-12 10:21 PDT, Carlos Garcia Campos
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (16.86 KB, patch)
2017-06-12 22:54 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>