[WK2] Gather resource load statistics

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Second step of capturing resource load statistics, this time targeting WebKit2.

[Brent Fulgham]

Created attachment 272172 [details]
Patch

[Brent Fulgham]

Created attachment 272173 [details]
Patch

[Brent Fulgham]

Created attachment 272177 [details]
Patch

[Brent Fulgham]

This patch allows me to run the statistics load code in WK2 Minibrowser. I need help with a few things:

1. I'm probably not handling the preferences stuff properly for WK2 since I had to hack a couple of methods on to ProcessPool. How do you get to the WKContextRef in something like MiniBrowser?

2. Note that proper long-term storage will be addressed in Bug 154642.

3. Should I be using a RuntimeEnabledFeature flag instead of the preference?

[Brent Fulgham]

Created attachment 272208 [details]
Patch (Fixed EFL Build)

[Brent Fulgham]

Created attachment 272253 [details]
Patch (Revised per Anders)

[Brent Fulgham]

Comment on attachment 272253 [details]
Patch (Revised per Anders)

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

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsState.cpp:43
> +}

This file is unused. I'll remove.

> Source/WebKit2/UIProcess/API/C/WKResourceLoadStatisticsState.h:43
> +#endif /* WKResourceLoadStatisticsState_h */

This whole file is unused. I'll remove.

> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:32
> +#import <WebKit/WKContext.h>

This isn't needed.

[Brent Fulgham]

Created attachment 272257 [details]
Patch (Revised per Anders)

[Radar WebKit Bug Importer]

<rdar://problem/24851116>

[Brent Fulgham]

<rdar://problem/24702892>

[Sam Weinig]

Comment on attachment 272257 [details]
Patch (Revised per Anders)

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

Anders wants everyone to know that he reviewed this with Sam.

> Source/WebCore/loader/ResourceLoadObserver.cpp:393
> +    Vector<ResourceLoadStatistics> statistics;

This should be using reserveInitialCapacity based on m_resourceStatisticsMap.size()

> Source/WebCore/loader/ResourceLoadObserver.cpp:395
> +        statistics.append(statistic);

This should move the values into the Vector and use uncheckedAppend()

> Source/WebCore/loader/ResourceLoadObserver.h:56
> +    WEBCORE_EXPORT bool isEmpty() const { return m_resourceStatisticsMap.isEmpty(); }

Inline functions don't need to be exported.

> Source/WebCore/loader/ResourceLoadObserver.h:64
> +    static String defaultFilename() { return ASCIILiteral("full_browsing_session_resourceLog.plist"); }

This should not be inline.  It's also really weird to be naming a file like this.  I am also confused why there is file writing going on at all in WebCore.

> Source/WebCore/loader/ResourceLoadStatistics.h:40
> +    { }

This should be 
{
}

> Source/WebCore/loader/ResourceLoadStatistics.h:43
> +    { }

Same here

> Source/WebKit2/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +

Can you add a high level description of what is going on here?

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2056
> +static void encodeHashCountedSet(ArgumentEncoder& encoder, const HashCountedSet<String>& hashCountedSet)
> +{
> +    encoder << hashCountedSet.size();
> +
> +    for (auto entry : hashCountedSet) {
> +        encoder << entry.key;
> +        encoder << entry.value;
> +    }
> +}

This needs to go into ArgumentCoders.h and be generalized on HashCountedSet<T>.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2115
> +static bool decodeHashCountedSet(ArgumentDecoder& decoder, HashCountedSet<String>& hashCountedSet)
> +{
> +    unsigned entries;
> +    if (!decoder.decode(entries))
> +        return false;
> +
> +    for (size_t i = 0; i < entries; ++i) {
> +        String origin;
> +        if (!decoder.decode(origin))
> +            return false;
> +        
> +        unsigned count;
> +        if (!decoder.decode(count))
> +            return false;
> +
> +        hashCountedSet.add(origin, count);
> +    }
> +
> +    return true;
> +}

This too needs to move to ArgumentCoders.h

> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.cpp:37
> +PassRefPtr<ResourceLoadStatisticsState> ResourceLoadStatisticsState::create(WebProcessPool* processPool)

This should return a Ref<ResourceLoadStatisticsState> and take a WebProcessPool&.

> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.cpp:45
> +    m_processPool.addMessageReceiver(Messages::ResourceLoadStatisticsState::messageReceiverName(), *this);

If you add yourself as a MessageReceiver, you should also remove yourself somewhere.

> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.h:43
> +class ResourceLoadStatisticsState : public API::ObjectImpl<API::Object::Type::ResourceLoadStatisticsState>, private IPC::MessageReceiver {

We tend to think of State objects as immutable things. This doesn't seem to fall into that category.

> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.h:62
> +    WebProcessPool& m_processPool;

It's not clear to me that this should be on the WebProcessPool. I think we need to have a conversation about this.

> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.h:64
> +    std::unique_ptr<WebCore::ResourceLoadObserver> m_resourceLoadObserver;

It seems wrong to use a WebCore::ResourceLoadObserver in the UIProcess as that class uses many WebProcess specific classes, such as Document and Settings. Even if this happens to work, it is the wrong abstraction for it.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:62
> +- (void)setResourceLoadStatisticsEnabled:(BOOL)value;

This needs to start with an underscore and have WK_AVAILABLE. It should also probably be a @property. It should also probably be on the _WKProcessPoolConfiguration.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:52
> +@property (nonatomic, setter=_setResourceLoadStatisticsEnabled:) BOOL _resourceLoadStatisticsEnabled;

I don't get why this is needed on both the configuration and the process pool.

> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:441
> +    NSString *storageDirectory = [[NSUserDefaults standardUserDefaults] objectForKey:WebKitResourceLoadStatisticsDirectoryDefaultsKey];

Please don't use NSUserDefaults for this.  If you need to provide a custom location, please pass it in explicitly to the API, probably on the _WKProcessPoolConfiguration.  It would be good if you could reuse WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation() for this.

> Source/WebKit2/WebProcess/WebProcess.cpp:369
> +    m_statisticsChangedTimer.startRepeating(std::chrono::seconds(5));

This is not a good idea.  We try very hard to not have repeating timers like this.  I assume this is for coalescing updates, so I will recommend using an approach that has changes in the resource load observer trigger the coalesced timer. That way, if no resources are being loaded, the timer won't fire and wake up the process.

[Brent Fulgham]

I'm back in the office tomorrow and will try to get a few minutes of your time to hammer out the details of your comments. Thank your for taking time to look this over.

[Brent Fulgham]

Comment on attachment 272257 [details]
Patch (Revised per Anders)

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

>> Source/WebCore/loader/ResourceLoadObserver.cpp:393
>> +    Vector<ResourceLoadStatistics> statistics;
> 
> This should be using reserveInitialCapacity based on m_resourceStatisticsMap.size()

OK!

>> Source/WebCore/loader/ResourceLoadObserver.cpp:395
>> +        statistics.append(statistic);
> 
> This should move the values into the Vector and use uncheckedAppend()

Would it be better to do:

Vector<ResourceLoadStatistics> statistics = WTFMove(m_resourceStatisticsMap.values());

... instead?

To answer my own question: No. It looks like 'values()' returns a set of iterators that the Vector constructor doesn't provide an overload for.

>> Source/WebCore/loader/ResourceLoadObserver.h:56
>> +    WEBCORE_EXPORT bool isEmpty() const { return m_resourceStatisticsMap.isEmpty(); }
> 
> Inline functions don't need to be exported.

OK!

>> Source/WebCore/loader/ResourceLoadObserver.h:64
>> +    static String defaultFilename() { return ASCIILiteral("full_browsing_session_resourceLog.plist"); }
> 
> This should not be inline.  It's also really weird to be naming a file like this.  I am also confused why there is file writing going on at all in WebCore.

This is left over from the early experimental stages. I had hoped to address the longer-term storage aspects in Bug 154642 in one step, moving file writing out of WebCore and into the WK1 and WK2 layers. Do you want me to attack that in this same step? I was hoping to keep each step simple.

>> Source/WebCore/loader/ResourceLoadStatistics.h:40
>> +    { }
> 
> This should be 
> {
> }

OK!

>> Source/WebCore/loader/ResourceLoadStatistics.h:43
>> +    { }
> 
> Same here

OK! We should probably have a check-webkit-style for this.

>> Source/WebKit2/ChangeLog:8
>> +
> 
> Can you add a high level description of what is going on here?

Will do!

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2056
>> +}
> 
> This needs to go into ArgumentCoders.h and be generalized on HashCountedSet<T>.

Oh, cool! I didn't know about this file. That's much cleaner.

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2115
>> +}
> 
> This too needs to move to ArgumentCoders.h

Will do!

>> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.cpp:37
>> +PassRefPtr<ResourceLoadStatisticsState> ResourceLoadStatisticsState::create(WebProcessPool* processPool)
> 
> This should return a Ref<ResourceLoadStatisticsState> and take a WebProcessPool&.

OK!

>> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.cpp:45
>> +    m_processPool.addMessageReceiver(Messages::ResourceLoadStatisticsState::messageReceiverName(), *this);
> 
> If you add yourself as a MessageReceiver, you should also remove yourself somewhere.

That should have been obvious to me. :-(

[Brent Fulgham]

Comment on attachment 272257 [details]
Patch (Revised per Anders)

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

>> Source/WebKit2/UIProcess/ResourceLoadStatisticsState.h:64
>> +    std::unique_ptr<WebCore::ResourceLoadObserver> m_resourceLoadObserver;
> 
> It seems wrong to use a WebCore::ResourceLoadObserver in the UIProcess as that class uses many WebProcess specific classes, such as Document and Settings. Even if this happens to work, it is the wrong abstraction for it.

It seems like I should split the ResourceLoadObserver class up. The only part of this I'm using in the UIProcess is the "thing that holds a collection of ResourceLoadStatistics", not the part that looks at documents and settings. I'll make a new class, maybe "ResourceLoadStatisticsCollection" that can be used by ResourceLoadObserver.

[Brent Fulgham]

Comment on attachment 272257 [details]
Patch (Revised per Anders)

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

>> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:441
>> +    NSString *storageDirectory = [[NSUserDefaults standardUserDefaults] objectForKey:WebKitResourceLoadStatisticsDirectoryDefaultsKey];
> 
> Please don't use NSUserDefaults for this.  If you need to provide a custom location, please pass it in explicitly to the API, probably on the _WKProcessPoolConfiguration.  It would be good if you could reuse WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation() for this.

Yes! I will do this!

[Brent Fulgham]

Created attachment 272773 [details]
Patch

[Brent Fulgham]

Created attachment 272774 [details]
Patch (v2)

[Brent Fulgham]

Comment on attachment 272774 [details]
Patch (v2)

This includes fixes for Sam/Anders review, plus a fix for EFL/GTK builds

[Brent Fulgham]

1) iOS Failure:

/Volumes/Data/EWS/WebKit/Source/WebKit2/WebProcess/WebProcess.cpp:173:7: error: field 'm_resourceLoadStatisticsStorage' will be initialized after field 'm_webSQLiteDatabaseTracker' [-Werror,-Wreorder]
    , m_resourceLoadStatisticsStorage(WebCore::ResourceLoadStatisticsStore::create())
      ^
2) GTK Failure:

API::ProcessPoolConfiguration::ProcessPoolConfiguration(): error: undefined reference to 'API::WebsiteDataStore::defaultResourceLoadStatisticsDirectory()'

3) EFK Failure:

Missing API::WebsiteDataStore::defaultResourceLoadStatisticsDirectory()

[Andy Estes]

Comment on attachment 272774 [details]
Patch (v2)

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

I'd like to see a version of this with a simplified SPI for enabling the feature.

> Source/WebCore/loader/ResourceLoadObserver.h:59
> +    RefPtr<ResourceLoadStatisticsStore> m_statistics;

Can you call this m_store or m_statisticsStore instead? m_statistics sounds like an instance of ResourceLoadStatistics.

> Source/WebCore/loader/ResourceLoadStatistics.h:45
> +    ResourceLoadStatistics()
> +    {
> +    }

I slightly prefer to write this as ResourceLoadStatistics() = default

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:75
> +    if (!m_resourceStatisticsMap.contains(primaryDomain)) {
> +        ResourceLoadStatistics newStatistics(primaryDomain);
> +        m_resourceStatisticsMap.set(primaryDomain, newStatistics);
> +    }
> +    
> +    return m_resourceStatisticsMap.find(primaryDomain)->value;

This looks like a perfect case for HashMap::ensure():

    auto addResult =  m_resourceStatisticsMap.ensure(primaryDomain, [primaryDomain] { return ResourceLoadStatistics(primaryDomain); });
    return addResult.iterator->value;

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:83
> +    encoder->encodeUInt32("originsVisited", m_resourceStatisticsMap.size());

I don't see why you need to encode this value.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:105
> +void ResourceLoadStatisticsStore::writeDataToDisk(const String& origin, const ResourceLoadStatistics& resourceStatistics) const
> +{
> +    auto encoder = KeyedEncoder::encoder();
> +    encoder->encodeUInt32("originsVisited", 1);
> +    
> +    encoder->encodeObject(origin, resourceStatistics, [this](KeyedEncoder& encoder, const ResourceLoadStatistics& resourceStatistics) {
> +        resourceStatistics.encode(encoder);
> +    });
> +    
> +    String label = origin;
> +    label.replace('/', '_');
> +    label.replace(':', '+');
> +    writeEncoderToDisk(*encoder.get(), label);
> +}

I don't see this function being called anywhere.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:132
> +    decoder->decodeUInt32("originsVisited", visitedOrigins);

Previous comment about the utility of encoding this value notwithstanding, you need to check the return value of decodeUInt32().

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:135
> +    decoder->decodeObjects("browsingStatistics", loadedOrigins, [this](KeyedDecoder& decoderInner, String& origin) {

You need to check this return value, too. If it returns false, then m_resourceStatisticsMap should be left unmodified (or at least cleared). The file on disk should maybe also be deleted, although I guess that's unnecessary since it'll just be overwritten with valid data the next time we save.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:137
> +        if (!decoderInner.decodeString("PrevalentResourceOrigin", origin))
> +            return false;

This seems wrong. Won't ResourceLoadStatistics::decode() also try to decode "PrevalentResourceOrigin"? Is it OK to decode the same key twice?

Even if it were ok, I'd still be bothered by the lack of symmetry between this lambda and the one passed to encodeObjects() above. I think you should pass a Vector<ResourceLoadStatistics> to decodeObjects() and then move the values from the vector to the map once you know that decoding was successful.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:148
> +    ASSERT(visitedOrigins == static_cast<size_t>(loadedOrigins.size()));

This isn't a good assertion, since most causes of decoding failure are outside of your control. I'd just make sure you handle decoding failure properly at runtime.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:198
> +    if (!m_resourceStatisticsMap.contains(origin))
> +        return emptyString();
> +    
> +    auto& statistics = m_resourceStatisticsMap.find(origin)->value;

Why call both contains() and find()? Can't you check the iterator returned from find() against m_resourceStatisticsMap::end()?

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:211
> +    Vector<ResourceLoadStatistics> statistics;
> +    statistics.reserveInitialCapacity(m_resourceStatisticsMap.size());
> +    for (auto& statistic : m_resourceStatisticsMap.values())
> +        statistics.uncheckedAppend(WTFMove(statistic));
> +
> +    m_resourceStatisticsMap.clear();
> +
> +    return statistics;

This could just be:

    return WTFMove(m_resourceStatisticsMap);

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:223
> +        auto iter = m_resourceStatisticsMap.find(statistic.highLevelDomain);
> +        if (iter == m_resourceStatisticsMap.end()) {
> +            m_resourceStatisticsMap.set(statistic.highLevelDomain, statistic);
> +            continue;
> +        }
> +        
> +        iter->value.merge(statistic);

You could use HashMap::ensure() to store a default-constructed value when the key is missing, then you could call merge unconditionally.

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:51
> +    bool isEmpty() const { return m_resourceStatisticsMap.isEmpty(); }
> +    size_t size() const { return m_resourceStatisticsMap.size(); }

Do we need isEmpty() if we have size()?

> Source/WebCore/loader/ResourceLoadStatisticsStore.h:67
> +    ResourceLoadStatisticsStore()
> +    {
> +    }

I slightly prefer to write this as ResourceLoadStatisticsStore() = default

> Source/WebKit2/Platform/IPC/ArgumentCoders.h:2
> + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved.

I think we can write a year range (e.g. 2010-2016) these days.

> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2
> + * Copyright (C) 2011, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/Shared/WebCoreArgumentCoders.h:2
> + * Copyright (C) 2010-2011, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:2
> + * Copyright (C) 2010-2012, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/Shared/WebProcessCreationParameters.h:2
> + * Copyright (C) 2010-2012, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:2
> + * Copyright (C) 2010-2013, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/UIProcess/WebProcessPool.h:2
> + * Copyright (C) 2010-2013, 2016 Apple Inc. All rights reserved.

Ditto.

> Source/WebKit2/UIProcess/WebProcessPool.h:234
> +    WebResourceLoadStatisticsStore* resourceLoadStatisticsStore() const { return m_resourceLoadStatistics.get(); }

I don't think you need this anymore.

> Source/WebKit2/UIProcess/WebProcessPool.h:480
> +    RefPtr<WebResourceLoadStatisticsStore> m_resourceLoadStatistics;

I don't think you need this anymore.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:44
> +class WebResourceLoadStatisticsStore : public IPC::Connection::WorkQueueMessageReceiver {

Seems like this should be a subclass of WebCore::ResourceLoadStatisticsStore.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:40
> +@property (nonatomic, setter=_setResourceLoadStatisticsEnabled:) BOOL _resourceLoadStatisticsEnabled WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);

Since the load statistics store is owned by the website data store, this seems like the right place for the SPI that enables the feature. But if you do it here, you should remove *all* the other places with enabled properties/logic that aren't WebsiteDataStore-related. You could also move the code that notifies web processes about state changes to WebsiteDataStore.

This patch would seem a lot clearer without the proliferation of enable code.

> Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:181
> +    parameters.resourceLoadStatisticsEnabled = [defaults boolForKey:WebKitResourceLoadStatisticsEnabledDefaultsKey];

I don't think you need to do this. It seems fine to default to false and rely on the application to enable this at runtime.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:81
> +    , m_resourceLoadStatisticsDirectory(WTFMove(configuration.resourceLoadStatisticsDirectory))

Do you need to store the directory path separately? You only use it to construct m_resourceLoadStatistics.

> Source/WebKit2/WebProcess/WebProcess.cpp:207
> +    ResourceLoadObserver::sharedObserver().setStatisticsStore(m_resourceLoadStatisticsStorage.copyRef());

Not sure I see why WebProcess has to keep a strong reference to the statistics store. All these objects are essentially singletons. It'd seem cleaner to have the store created and owned by ResourceLoadObserver, which could have public functions for setting the notification callback and taking the statistics vector.

> Source/WebKit2/WebProcess/WebProcess.cpp:1306
> +void WebProcess::notifyNewStatisticsAdded()
> +{
> +    if (m_statisticsChangedTimer.isActive())
> +        return;
> +
> +    m_statisticsChangedTimer.startOneShot(std::chrono::seconds(5));
> +}

Is this called anywhere?

[Brent Fulgham]

Comment on attachment 272774 [details]
Patch (v2)

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

>> Source/WebCore/loader/ResourceLoadObserver.h:59
>> +    RefPtr<ResourceLoadStatisticsStore> m_statistics;
> 
> Can you call this m_store or m_statisticsStore instead? m_statistics sounds like an instance of ResourceLoadStatistics.

m_store seems fine.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:75
>> +    return m_resourceStatisticsMap.find(primaryDomain)->value;
> 
> This looks like a perfect case for HashMap::ensure():
> 
>     auto addResult =  m_resourceStatisticsMap.ensure(primaryDomain, [primaryDomain] { return ResourceLoadStatistics(primaryDomain); });
>     return addResult.iterator->value;

Ooh! Wow! I didn't know that existed! Will do!

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:83
>> +    encoder->encodeUInt32("originsVisited", m_resourceStatisticsMap.size());
> 
> I don't see why you need to encode this value.

I stuck that in there so I could compare the count I read back to the count I wrote out originally to find serialization errors. But it was never a problem, and certainly isn't needed now. I'll remove it.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:105
>> +}
> 
> I don't see this function being called anywhere.

I'll get rid of it. It was used in some earlier debugging logic that is now gone.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:132
>> +    decoder->decodeUInt32("originsVisited", visitedOrigins);
> 
> Previous comment about the utility of encoding this value notwithstanding, you need to check the return value of decodeUInt32().

I got rid of it, but you point is noted (and I hang my head in shame)!

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:135
>> +    decoder->decodeObjects("browsingStatistics", loadedOrigins, [this](KeyedDecoder& decoderInner, String& origin) {
> 
> You need to check this return value, too. If it returns false, then m_resourceStatisticsMap should be left unmodified (or at least cleared). The file on disk should maybe also be deleted, although I guess that's unnecessary since it'll just be overwritten with valid data the next time we save.

I'll clear out the HashMap, but I won't touch the file system here. We'll be revisiting storage in Bug 154642.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:137
>> +            return false;
> 
> This seems wrong. Won't ResourceLoadStatistics::decode() also try to decode "PrevalentResourceOrigin"? Is it OK to decode the same key twice?
> 
> Even if it were ok, I'd still be bothered by the lack of symmetry between this lambda and the one passed to encodeObjects() above. I think you should pass a Vector<ResourceLoadStatistics> to decodeObjects() and then move the values from the vector to the map once you know that decoding was successful.

You are right -- that wasn't necessary. Happily, it worked because the plist retrieval logic reads the plist into an NSDictionary, which is perfectly happy to answer repeated requests for the same keys.

However, I'll fix that.

I'll also revise this to match the encodeObjects behavior.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:148
>> +    ASSERT(visitedOrigins == static_cast<size_t>(loadedOrigins.size()));
> 
> This isn't a good assertion, since most causes of decoding failure are outside of your control. I'd just make sure you handle decoding failure properly at runtime.

Done!

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:198
>> +    auto& statistics = m_resourceStatisticsMap.find(origin)->value;
> 
> Why call both contains() and find()? Can't you check the iterator returned from find() against m_resourceStatisticsMap::end()?

Oh, duh. Thanks for catching that.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:211
>> +    return statistics;
> 
> This could just be:
> 
>     return WTFMove(m_resourceStatisticsMap);

I don't think that works, because m_resourceStatisticsMap is a HashMap, and I'm trying to return a Vector. :-(

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:223
>> +        iter->value.merge(statistic);
> 
> You could use HashMap::ensure() to store a default-constructed value when the key is missing, then you could call merge unconditionally.

Yes! That is so cool - I'm glad you told me about that method.

>> Source/WebCore/loader/ResourceLoadStatisticsStore.h:51
>> +    size_t size() const { return m_resourceStatisticsMap.size(); }
> 
> Do we need isEmpty() if we have size()?

No, but I think it reads better in code that uses it. Ultimately, isEmpty is defined in terms of size().

>> Source/WebCore/loader/ResourceLoadStatisticsStore.h:67
>> +    }
> 
> I slightly prefer to write this as ResourceLoadStatisticsStore() = default

OK!

>> Source/WebKit2/Platform/IPC/ArgumentCoders.h:2
>> + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved.
> 
> I think we can write a year range (e.g. 2010-2016) these days.

OK!

>> Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:2
>> + * Copyright (C) 2011, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

>> Source/WebKit2/Shared/WebCoreArgumentCoders.h:2
>> + * Copyright (C) 2010-2011, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

>> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:2
>> + * Copyright (C) 2010-2012, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

>> Source/WebKit2/Shared/WebProcessCreationParameters.h:2
>> + * Copyright (C) 2010-2012, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

>> Source/WebKit2/UIProcess/WebProcessPool.cpp:2
>> + * Copyright (C) 2010-2013, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

>> Source/WebKit2/UIProcess/WebProcessPool.h:2
>> + * Copyright (C) 2010-2013, 2016 Apple Inc. All rights reserved.
> 
> Ditto.

OK!

> Source/WebKit2/UIProcess/WebProcessPool.h:87
> +class WebResourceLoadStatisticsStore;

This isn't needed, either!

>> Source/WebKit2/UIProcess/WebProcessPool.h:234
>> +    WebResourceLoadStatisticsStore* resourceLoadStatisticsStore() const { return m_resourceLoadStatistics.get(); }
> 
> I don't think you need this anymore.

Oops! You are right!

>> Source/WebKit2/UIProcess/WebProcessPool.h:480
>> +    RefPtr<WebResourceLoadStatisticsStore> m_resourceLoadStatistics;
> 
> I don't think you need this anymore.

OK!

>> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:44
>> +class WebResourceLoadStatisticsStore : public IPC::Connection::WorkQueueMessageReceiver {
> 
> Seems like this should be a subclass of WebCore::ResourceLoadStatisticsStore.

Does that work when going across WebCore/WebKit2? For example, WebPage is not a subclass of WebCore::Page, it uses composition like I did here.

I tried making this a subclass of the WebCore class, but ran into trouble with duplicate destruction definitions, and conflict between FAST_MALLOC and IPC/ThreadSafe memory handling.

I don't thin this works the way you want here. :-(

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:40
>> +@property (nonatomic, setter=_setResourceLoadStatisticsEnabled:) BOOL _resourceLoadStatisticsEnabled WK_AVAILABLE(WK_MAC_TBA, WK_IOS_TBA);
> 
> Since the load statistics store is owned by the website data store, this seems like the right place for the SPI that enables the feature. But if you do it here, you should remove *all* the other places with enabled properties/logic that aren't WebsiteDataStore-related. You could also move the code that notifies web processes about state changes to WebsiteDataStore.
> 
> This patch would seem a lot clearer without the proliferation of enable code.

Agreed. As we discussed in person, this is one of the things I'm least happy with in this patch. Unfortunately, it wasn't clear to me how to get to the WKWebsiteDataStore from the process pool. I'll take another look tomorrow.

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:81
>> +    , m_resourceLoadStatisticsDirectory(WTFMove(configuration.resourceLoadStatisticsDirectory))
> 
> Do you need to store the directory path separately? You only use it to construct m_resourceLoadStatistics.

Probably not. I can get rid of that.

>> Source/WebKit2/WebProcess/WebProcess.cpp:207
>> +    ResourceLoadObserver::sharedObserver().setStatisticsStore(m_resourceLoadStatisticsStorage.copyRef());
> 
> Not sure I see why WebProcess has to keep a strong reference to the statistics store. All these objects are essentially singletons. It'd seem cleaner to have the store created and owned by ResourceLoadObserver, which could have public functions for setting the notification callback and taking the statistics vector.

I'd prefer to go the other direction (as is done in this patch), so that we can move more of this logic into WK2 in future updates.

>> Source/WebKit2/WebProcess/WebProcess.cpp:1306
>> +}
> 
> Is this called anywhere?

Nope. I made it a lambda. I'll get rid of this.

> Source/WebKit/mac/WebView/WebView.mm:611
> +static WebCore::ResourceLoadStatisticsStore *resourceLoadStatisticsStore;

I tried to make this a static RefPtr<WebCore::ResourceLoadStatisticsStore>, but clang complained about needing a global destructor. Is there a better way to do this?

[Brent Fulgham]

Created attachment 272838 [details]
Patch (v3)

[Brent Fulgham]

Comment on attachment 272838 [details]
Patch (v3)

Updated with (most of) Andy's comments, plus more attempts to fix Efl/Gtk.

I'll revise the SPI for setting the feature tomorrow when I'm in the office.

[Brent Fulgham]

Created attachment 272840 [details]
Patch (v4)

[Brent Fulgham]

Comment on attachment 272840 [details]
Patch (v4)

Correct merge.

[Andy Estes]

(In reply to comment #21)
> Comment on attachment 272774 [details]
> Patch (v2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272774&action=review
> 
> > Source/WebKit/mac/WebView/WebView.mm:611
> > +static WebCore::ResourceLoadStatisticsStore *resourceLoadStatisticsStore;
> 
> I tried to make this a static RefPtr<WebCore::ResourceLoadStatisticsStore>,
> but clang complained about needing a global destructor. Is there a better
> way to do this?

I think the raw pointer is ok, but you should leak a reference to protect against the Ref passed to ResourceLoadObserver going out of scope and leaving you with a dangling pointer:

    resourceLoadStatisticsStore = &WebCore::ResourceLoadStatisticsStore::create(supportDirectory).leakRef();
    ResourceLoadObserver::sharedObserver().setStatisticsStore(*statisticsStore);

[Brent Fulgham]

(In reply to comment #26)
> (In reply to comment #21)
> > Comment on attachment 272774 [details]
> > Patch (v2)
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=272774&action=review
> > 
> > > Source/WebKit/mac/WebView/WebView.mm:611
> > > +static WebCore::ResourceLoadStatisticsStore *resourceLoadStatisticsStore;
> > 
> > I tried to make this a static RefPtr<WebCore::ResourceLoadStatisticsStore>,
> > but clang complained about needing a global destructor. Is there a better
> > way to do this?
> 
> I think the raw pointer is ok, but you should leak a reference to protect
> against the Ref passed to ResourceLoadObserver going out of scope and
> leaving you with a dangling pointer:
> 
>     resourceLoadStatisticsStore =
> &WebCore::ResourceLoadStatisticsStore::create(supportDirectory).leakRef();
>    
> ResourceLoadObserver::sharedObserver().setStatisticsStore(*statisticsStore);

OK! Thanks.

[Brent Fulgham]

Created attachment 273028 [details]
Patch (v5)

[Brent Fulgham]

Comment on attachment 273028 [details]
Patch (v5)

Incorporates all of Andy's review comments, simplifies the SPI, and corrects the build failures.

[WebKit Commit Bot]

Attachment 273028 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:67:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
Total errors found: 1 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #30)
> Attachment 273028 [details] did not pass style-queue:
> 
> ERROR: Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:67: 
> "virtual" is redundant since function is already declared as "override" 
> [readability/inheritance] [4]
> Total errors found: 1 in 48 files

Whooooo! I'll gladly fix this :-)

[Andy Estes]

Comment on attachment 273028 [details]
Patch (v5)

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

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:197
> +        auto result = m_resourceStatisticsMap.ensure(statistic.highLevelDomain, [&statistic] {
> +            return statistic;
> +        });
> +        
> +        result.iterator->value.merge(statistic);

Won't this double-count statistics from a new high-level domain? ensure() will insert the new statistic into the map, then merge() will merge it with itself.

I think ensure() needs to insert a default-constructed statistic.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:624
> +    parameters.resourceLoadStatisticsEnabled = configuration().resourceLoadStatisticsEnabled();

I suggest below that you move the resourceLoadStatisticsEnabled flag from the configuration to WebProcessPool, which means this line would just need to access a member variable.

> Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:112
> +    WTF::String m_resourceLoadStatisticsDirectory;

I don't think this belongs in APIProcessPoolConfiguration, since there is no corresponding API in WKProcessPoolConfiguration. I think it should just be a member of WebProcessPool.

> Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:116
> +    bool m_resourceLoadStatisticsEnabled { false };

Ditto.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:360
> +    processPool.configuration().setResourceLoadStatisticsEnabled(configuration.websiteDataStore._resourceLoadStatisticsEnabled);

This seems a little flaky. What if I call -[WKWebsiteDataStore _setResourceLoadStatisticsEnabled:] after creating a WKWebView but before the first process is launched? Seems like the process pool would never get configured with the right value for creating new web processes.

If you move the settings value from APIProcessPoolConfiguration to WebProcesPool, you can just have WebsiteDataStore::setResourceLoadStatisticsEnabled() set the value on the process pool along with calling sendToAllProcesses().

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1120
> +    for (auto& processPool : WebProcessPool::allProcessPools())

I notice that some parts of the code use WebProcessPool::allProcessPools(), some use processPools(), and some use both. I don't know which is correct in this case. Maybe Anders does?

> Source/WebKit2/WebProcess/WebProcess.cpp:2
> + * Copyright (C) 2009-2010, 2012, 2014, 2016 Apple Inc. All rights reserved.

2009-2016

> Source/WebKit2/WebProcess/WebProcess.h:2
> + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved.

2010-2016

> Source/WebKit/mac/WebView/WebView.mm:611
> +static WebCore::ResourceLoadStatisticsStore *resourceLoadStatisticsStore;

The space goes after the asterisk instead of before in this case.

[Andy Estes]

Comment on attachment 273028 [details]
Patch (v5)

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:175
> +- (BOOL)_resourceLoadStatisticsEnabled
> +{
> +    return _websiteDataStore->websiteDataStore().resourceLoadStatisticsEnabled();
> +}
> +
> +- (void)_setResourceLoadStatisticsEnabled:(BOOL)enabled
> +{
> +    _websiteDataStore->websiteDataStore().setResourceLoadStatisticsEnabled(enabled);
> +}

This should probably call into member functions on APIWebsiteDataStore, which should then call into WebsiteDataStore.

[Andy Estes]

Comment on attachment 273028 [details]
Patch (v5)

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

>> Source/WebKit2/UIProcess/WebProcessPool.cpp:624
>> +    parameters.resourceLoadStatisticsEnabled = configuration().resourceLoadStatisticsEnabled();
> 
> I suggest below that you move the resourceLoadStatisticsEnabled flag from the configuration to WebProcessPool, which means this line would just need to access a member variable.

Actually, I don't think you need to add anything to WebProcessPool. You can just do this:

    parameters.resourceLoadStatisticsEnabled = m_websiteDataStore->resourceLoadStatisticsEnabled();

[Brent Fulgham]

Committed r197592: <http://trac.webkit.org/changeset/197592>

[Brent Fulgham]

Comment on attachment 273028 [details]
Patch (v5)

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

>> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:197
>> +        result.iterator->value.merge(statistic);
> 
> Won't this double-count statistics from a new high-level domain? ensure() will insert the new statistic into the map, then merge() will merge it with itself.
> 
> I think ensure() needs to insert a default-constructed statistic.

Oh, I think you are right. I'll fix that. It should have read:

    return ResourceLoadStatistics(statistic.highLevelDomain);

>> Source/WebKit2/UIProcess/WebProcessPool.cpp:624
>> +    parameters.resourceLoadStatisticsEnabled = configuration().resourceLoadStatisticsEnabled();
> 
> I suggest below that you move the resourceLoadStatisticsEnabled flag from the configuration to WebProcessPool, which means this line would just need to access a member variable.

OK! I actually gave it an accessor (to balance the 'set' function) and used it here.

>> Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:112
>> +    WTF::String m_resourceLoadStatisticsDirectory;
> 
> I don't think this belongs in APIProcessPoolConfiguration, since there is no corresponding API in WKProcessPoolConfiguration. I think it should just be a member of WebProcessPool.

Yeah, it's actually not used. I'll get rid of this variable ...

>> Source/WebKit2/UIProcess/API/APIProcessPoolConfiguration.h:116
>> +    bool m_resourceLoadStatisticsEnabled { false };
> 
> Ditto.

.... and move this to WebProcessPool as you suggest later.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:360
>> +    processPool.configuration().setResourceLoadStatisticsEnabled(configuration.websiteDataStore._resourceLoadStatisticsEnabled);
> 
> This seems a little flaky. What if I call -[WKWebsiteDataStore _setResourceLoadStatisticsEnabled:] after creating a WKWebView but before the first process is launched? Seems like the process pool would never get configured with the right value for creating new web processes.
> 
> If you move the settings value from APIProcessPoolConfiguration to WebProcesPool, you can just have WebsiteDataStore::setResourceLoadStatisticsEnabled() set the value on the process pool along with calling sendToAllProcesses().

Sounds good.

>> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1120
>> +    for (auto& processPool : WebProcessPool::allProcessPools())
> 
> I notice that some parts of the code use WebProcessPool::allProcessPools(), some use processPools(), and some use both. I don't know which is correct in this case. Maybe Anders does?

I did some searching. "processPools()" is a static function local to this file. WebProcessPool::allProcessPools() is a static method on the class (visible outside this compilation unit) that simply calls the internal singleton method.

>> Source/WebKit2/WebProcess/WebProcess.cpp:2
>> + * Copyright (C) 2009-2010, 2012, 2014, 2016 Apple Inc. All rights reserved.
> 
> 2009-2016

Done.

>> Source/WebKit2/WebProcess/WebProcess.h:2
>> + * Copyright (C) 2010, 2016 Apple Inc. All rights reserved.
> 
> 2010-2016

Done.

>> Source/WebKit/mac/WebView/WebView.mm:611
>> +static WebCore::ResourceLoadStatisticsStore *resourceLoadStatisticsStore;
> 
> The space goes after the asterisk instead of before in this case.

OK!