Bug 236566 – Do preliminary work to pass domain names to CoreLocation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Do preliminary work to pass domain names to CoreLocation.

[Chris Dumez]

<rdar://88761413>

[Chris Dumez]

Created attachment 451829 [details]
Patch

[Chris Dumez]

Created attachment 451832 [details]
Patch

[Chris Dumez]

Created attachment 451838 [details]
Patch

[Chris Dumez]

Created attachment 451839 [details]
Patch

[Chris Dumez]

Created attachment 451844 [details]
Patch

[Chris Dumez]

Created attachment 451851 [details]
Patch

[Chris Dumez]

Created attachment 451853 [details]
Patch

[Chris Dumez]

Created attachment 451920 [details]
Patch

[Chris Dumez]

Created attachment 451976 [details]
Patch

[Chris Dumez]

Created attachment 451984 [details]
Patch

[Darin Adler]

Comment on attachment 451984 [details]
Patch

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

> Source/WebCore/Modules/geolocation/cocoa/GeolocationPositionDataCocoa.mm:33
> +GeolocationPositionData::GeolocationPositionData(CLLocation* location)

CLLocation * should have a space given our Objective-C class coding style.

> Source/WebCore/SourcesCocoa.txt:311
> +platform/graphics/avfoundation/objc/AVAssetTrackUtilities.mm @no-unify

I’d love to avoid adding new no-unify. Why were these required? Do you know any workaround?

> Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.h:46
> +        virtual void geolocationAuthorizationGranted(const String& /*websiteIdentifier*/) { }

Can any of these arguments be StringView instead of const String&?

> Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:50
> +static bool isAuthorizationGranted(CLAuthorizationStatus authorizationStatus)

I’d name the argument just status.

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:245
> +    for (auto& perDomainData : m_perDomainData.values()) {

Same name for the loop variable as this function’s argument. Could sidestep this by using a shorter name for one or both. Just “data” will do give the scope of a short function or loop.

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:265
> +    if (!m_clientProvider)

This code should go into the #else, or we could move the #if so it’s only around the provider creation and start call.

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:280
> +    UNUSED_PARAM(perDomainData);

Same refactoring as above would be good. Put #if around the stop call.

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:293
> +    UNUSED_PARAM(perDomainData);

And here. Add the early return?

> Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:104
> +    struct PerDomainData {

Should just name the struct here and define it inside the .cop file.

> Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:47
> +    return RegistrableDomain { document->url() };

Can probably use the braces without repeating the class name.

> Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:69
> +    auto& pageSets = m_pageSets.ensure(registrableDomain, [] {

This ensure should just be an add. It’s probably sufficiently efficient to make an empty PageSets; just two null pointers. Generally speaking new empty hash tables are just a null pointer, amd even though they sound expensive to allocate they’re not.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Comment on attachment 451984 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451984&action=review
> 
> > Source/WebCore/Modules/geolocation/cocoa/GeolocationPositionDataCocoa.mm:33
> > +GeolocationPositionData::GeolocationPositionData(CLLocation* location)
> 
> CLLocation * should have a space given our Objective-C class coding style.

OK.

> 
> > Source/WebCore/SourcesCocoa.txt:311
> > +platform/graphics/avfoundation/objc/AVAssetTrackUtilities.mm @no-unify
> 
> I’d love to avoid adding new no-unify. Why were these required? Do you know
> any workaround?

I consulted with Jer on this and this was his suggestion. This is happening because of unified build and soft-linking macros.
I'll double check to see if I still need both or if I can remove one.

> > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.h:46
> > +        virtual void geolocationAuthorizationGranted(const String& /*websiteIdentifier*/) { }
> 
> Can any of these arguments be StringView instead of const String&?

What's the benefit here though. I am holding a String as a data member and passing it as parameter. I think it makes sense to use the same type.

> 
> > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:50
> > +static bool isAuthorizationGranted(CLAuthorizationStatus authorizationStatus)
> 
> I’d name the argument just status.

OK.

> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:245
> > +    for (auto& perDomainData : m_perDomainData.values()) {
> 
> Same name for the loop variable as this function’s argument. Could sidestep
> this by using a shorter name for one or both. Just “data” will do give the
> scope of a short function or loop.

OK.

> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:265
> > +    if (!m_clientProvider)
> 
> This code should go into the #else, or we could move the #if so it’s only
> around the provider creation and start call.

I think there is probably a misunderstanding here. The code cannot be in the #else because it is still useful on IOS_FAMILY in those 2 cases:
1. The client sets its own provider via the API to pass a dummy provider (for testing, used by WKTR and API tests for e.g.)
2. The client sets its own provider via the API and passes a provider which rejects all authorization as a way to disable the geolocation API (used on watchOS).

Eventually, we should be able to get rid of both cases but it will require some additional work for provide alternatives for these clients.

> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:280
> > +    UNUSED_PARAM(perDomainData);
> 
> Same refactoring as above would be good. Put #if around the stop call.
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:293
> > +    UNUSED_PARAM(perDomainData);
> 
> And here. Add the early return?
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:104
> > +    struct PerDomainData {
> 
> Should just name the struct here and define it inside the .cop file.

Oh, last time I tried that it didn't work because I was also using the struct from WebGeolocationManagerCocoa.cpp. However, in this latest iteration, it doesn't seem to be the case anymore so I should be able to do this indeed.

> 
> > Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:47
> > +    return RegistrableDomain { document->url() };
> 
> Can probably use the braces without repeating the class name.

OK, will try.

> 
> > Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:69
> > +    auto& pageSets = m_pageSets.ensure(registrableDomain, [] {
> 
> This ensure should just be an add. It’s probably sufficiently efficient to
> make an empty PageSets; just two null pointers. Generally speaking new empty
> hash tables are just a null pointer, amd even though they sound expensive to
> allocate they’re not.

Ok.

[Chris Dumez]

Created attachment 452026 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Darin Adler from comment #12)
> > Comment on attachment 451984 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=451984&action=review
> > 
> > > Source/WebCore/Modules/geolocation/cocoa/GeolocationPositionDataCocoa.mm:33
> > > +GeolocationPositionData::GeolocationPositionData(CLLocation* location)
> > 
> > CLLocation * should have a space given our Objective-C class coding style.
> 
> OK.
> 
> > 
> > > Source/WebCore/SourcesCocoa.txt:311
> > > +platform/graphics/avfoundation/objc/AVAssetTrackUtilities.mm @no-unify
> > 
> > I’d love to avoid adding new no-unify. Why were these required? Do you know
> > any workaround?
> 
> I consulted with Jer on this and this was his suggestion. This is happening
> because of unified build and soft-linking macros.
> I'll double check to see if I still need both or if I can remove one.
> 
> > > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.h:46
> > > +        virtual void geolocationAuthorizationGranted(const String& /*websiteIdentifier*/) { }
> > 
> > Can any of these arguments be StringView instead of const String&?
> 
> What's the benefit here though. I am holding a String as a data member and
> passing it as parameter. I think it makes sense to use the same type.
> 
> > 
> > > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:50
> > > +static bool isAuthorizationGranted(CLAuthorizationStatus authorizationStatus)
> > 
> > I’d name the argument just status.
> 
> OK.
> 
> > 
> > > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:245
> > > +    for (auto& perDomainData : m_perDomainData.values()) {
> > 
> > Same name for the loop variable as this function’s argument. Could sidestep
> > this by using a shorter name for one or both. Just “data” will do give the
> > scope of a short function or loop.
> 
> OK.
> 
> > 
> > > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:265
> > > +    if (!m_clientProvider)
> > 
> > This code should go into the #else, or we could move the #if so it’s only
> > around the provider creation and start call.
> 
> I think there is probably a misunderstanding here. The code cannot be in the
> #else because it is still useful on IOS_FAMILY in those 2 cases:
> 1. The client sets its own provider via the API to pass a dummy provider
> (for testing, used by WKTR and API tests for e.g.)
> 2. The client sets its own provider via the API and passes a provider which
> rejects all authorization as a way to disable the geolocation API (used on
> watchOS).
> 
> Eventually, we should be able to get rid of both cases but it will require
> some additional work for provide alternatives for these clients.
> 
> > 
> > > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:280
> > > +    UNUSED_PARAM(perDomainData);
> > 
> > Same refactoring as above would be good. Put #if around the stop call.
> > 
> > > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:293
> > > +    UNUSED_PARAM(perDomainData);
> > 
> > And here. Add the early return?
> > 
> > > Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:104
> > > +    struct PerDomainData {
> > 
> > Should just name the struct here and define it inside the .cop file.
> 
> Oh, last time I tried that it didn't work because I was also using the
> struct from WebGeolocationManagerCocoa.cpp. However, in this latest
> iteration, it doesn't seem to be the case anymore so I should be able to do
> this indeed.

No actually, WebGeolocationManagerCocoa.cpp still uses PerDomainData so I still cannot do what you suggested it seems.

> > 
> > > Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:47
> > > +    return RegistrableDomain { document->url() };
> > 
> > Can probably use the braces without repeating the class name.
> 
> OK, will try.
> 
> > 
> > > Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:69
> > > +    auto& pageSets = m_pageSets.ensure(registrableDomain, [] {
> > 
> > This ensure should just be an add. It’s probably sufficiently efficient to
> > make an empty PageSets; just two null pointers. Generally speaking new empty
> > hash tables are just a null pointer, amd even though they sound expensive to
> > allocate they’re not.
> 
> Ok.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Comment on attachment 451984 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451984&action=review
> 
> > Source/WebCore/Modules/geolocation/cocoa/GeolocationPositionDataCocoa.mm:33
> > +GeolocationPositionData::GeolocationPositionData(CLLocation* location)
> 
> CLLocation * should have a space given our Objective-C class coding style.
> 
> > Source/WebCore/SourcesCocoa.txt:311
> > +platform/graphics/avfoundation/objc/AVAssetTrackUtilities.mm @no-unify
> 
> I’d love to avoid adding new no-unify. Why were these required? Do you know
> any workaround?
> 
> > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.h:46
> > +        virtual void geolocationAuthorizationGranted(const String& /*websiteIdentifier*/) { }
> 
> Can any of these arguments be StringView instead of const String&?
> 
> > Source/WebCore/platform/cocoa/CoreLocationGeolocationProvider.mm:50
> > +static bool isAuthorizationGranted(CLAuthorizationStatus authorizationStatus)
> 
> I’d name the argument just status.
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:245
> > +    for (auto& perDomainData : m_perDomainData.values()) {
> 
> Same name for the loop variable as this function’s argument. Could sidestep
> this by using a shorter name for one or both. Just “data” will do give the
> scope of a short function or loop.
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:265
> > +    if (!m_clientProvider)
> 
> This code should go into the #else, or we could move the #if so it’s only
> around the provider creation and start call.
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:280
> > +    UNUSED_PARAM(perDomainData);
> 
> Same refactoring as above would be good. Put #if around the stop call.
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.cpp:293
> > +    UNUSED_PARAM(perDomainData);
> 
> And here. Add the early return?
> 
> > Source/WebKit/UIProcess/WebGeolocationManagerProxy.h:104
> > +    struct PerDomainData {
> 
> Should just name the struct here and define it inside the .cop file.
> 
> > Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:47
> > +    return RegistrableDomain { document->url() };
> 
> Can probably use the braces without repeating the class name.

Hmm, this didn't work:
/Volumes/Data/WebKit/OpenSource/Source/WebKit/WebProcess/Geolocation/WebGeolocationManager.cpp:47:12: error: chosen constructor is explicit in copy-initialization
    return { document->url() };

[Chris Dumez]

Created attachment 452028 [details]
Patch

[EWS]

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

[Chris Dumez]

Created attachment 452044 [details]
Patch

[EWS]

Patch 452044 does not build

[Chris Dumez]

Created attachment 452055 [details]
Patch

[Chris Dumez]

Created attachment 452062 [details]
Patch

[EWS]

Patch 452062 does not build

[Chris Dumez]

Created attachment 452079 [details]
Patch

[Chris Dumez]

Created attachment 452081 [details]
Patch

[Chris Dumez]

Created attachment 452104 [details]
Patch

[Chris Dumez]

Created attachment 452105 [details]
Patch

[EWS]

Patch 452081 does not build

[Chris Dumez]

Created attachment 452106 [details]
Patch

[EWS]

Committed r289872 (247312@main): <https://commits.webkit.org/247312@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452106 [details].

[Myles C. Maxfield]

Reopening to attach new patch.

[Myles C. Maxfield]

Created attachment 452135 [details]
Patch

[Myles C. Maxfield]

Created attachment 452136 [details]
Patch

[Myles C. Maxfield]

Whoops, sorry, webkit-patch upload messed up