[webkit-reviews] review granted: [Bug 236566] Do preliminary work to pass domain names to CoreLocation : [Attachment 451984] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 236566: Do preliminary work to pass domain names to CoreLocation
https://bugs.webkit.org/show_bug.cgi?id=236566

Attachment 451984: Patch

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 451984
  --> https://bugs.webkit.org/attachment.cgi?id=451984
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.