Bug 176496

Summary: Modernize Geolocation code
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, sam, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch aestes: review+

Description Alex Christensen 2017-09-07 00:00:19 PDT
Modernize Geolocation code
Comment 1 Alex Christensen 2017-09-07 00:02:16 PDT
Created attachment 320098 [details]
Patch
Comment 2 Alex Christensen 2017-09-07 00:17:04 PDT
Created attachment 320099 [details]
Patch
Comment 3 Alex Christensen 2017-09-07 01:15:17 PDT
Created attachment 320102 [details]
Patch
Comment 4 Sam Weinig 2017-09-07 09:44:10 PDT
Comment on attachment 320102 [details]
Patch

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

> Source/WebCore/Modules/geolocation/GeolocationController.cpp:-114
> -    copyToVector(m_observers, observersVector);

Seems like we should have an overload of copyToVector that looks like:

    template<typename T, typename U, typename V>
    inline Vector<T> copyToVector(const HashSet<T, U, V>& collection)
    {
        Vector<T> result;
        result.reserveInitialCapacity(collection.size());
        for (auto& value : collection)
            result.uncheckedAppend(WTFCopy(value));
    }

Where  WTFCopy is a new function that usually just assigns, but is overloaded for Ref<> to call copyRef().

Maybe a big enough bit of infrastructure that you don't want to do it here.

> Source/WebKit/ChangeLog:10
> +        * NetworkProcess/CustomProtocols/LegacyCustomProtocolManager.cpp:
> +        (WebKit::LegacyCustomProtocolManager::LegacyCustomProtocolManager):
> +        (WebKit::LegacyCustomProtocolManager::startLoading):

ChangeLog could use some additional information.  Much of this looks un-Geolocation related.
Comment 5 Sam Weinig 2017-09-07 09:46:36 PDT
The biggest cleanup / improvement in this code that you could do would be to find a way to ensure that GeoNotifier::runSuccessCallback() always took a reference. Largely this revolves around figuring out what to do with Geolocation::lastPosition().
Comment 6 Alex Christensen 2017-09-07 10:45:58 PDT
Created attachment 320134 [details]
Patch
Comment 7 Alex Christensen 2017-09-07 10:53:44 PDT
I'm trying to make this have no change in behavior and that would be a change.  We can also do the vector cleanup later.
Comment 8 Alex Christensen 2017-09-07 12:02:02 PDT
http://trac.webkit.org/r221743
Comment 9 Radar WebKit Bug Importer 2017-09-27 12:38:28 PDT
<rdar://problem/34693666>