Bug 176496 - Modernize Geolocation code
Summary: Modernize Geolocation code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-07 00:00 PDT by Alex Christensen
Modified: 2017-09-27 12:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (59.13 KB, patch)
2017-09-07 00:02 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (61.75 KB, patch)
2017-09-07 00:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (66.79 KB, patch)
2017-09-07 01:15 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (67.13 KB, patch)
2017-09-07 10:45 PDT, Alex Christensen
aestes: review+
Details | Formatted Diff | Diff

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