Resource Load Statistics: Enable basic functionality in experimental debug mode

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should make the experimental debug mode feature work, at least to a basic level. This means:
- Debug logging on the INFO level.
- Permanently treat 3rdpartytestwebkit.org as a prevalent resource.
- Support manual setting of a custom permanently prevalent resource through user defaults on Cocoa platforms.

[Radar WebKit Bug Importer]

<rdar://problem/42408590>

[John Wilander]

Created attachment 345411 [details]
Patch

[John Wilander]

Created attachment 345452 [details]
Patch

[John Wilander]

An attempt to fix the non-Cocoa builds.

[John Wilander]

Failing Window tests are unrelated.

[Chris Dumez]

Comment on attachment 345452 [details]
Patch

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

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:54
> +static auto debugStaticPrevalentResource { "3rdpartytestwebkit.org" };

I'd prefer static const char* instead of static auto in this case, to be safe. Also, the variable name should probably include "Host".

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:323
> +bool ResourceLoadStatisticsMemoryStore::debugModeOverride(ResourceLoadStatistics& resourceStatistic)

The method name is not clear here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;

Should this be && instead of ||, I am unclear here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:988
> +static void debugLogDomainsInBatches(const String& message, const Vector<String>& domains)

Should the first parameter be a const char* ?
Also, wouldn't "action" be a better name for this parameter?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1001
> +    batch.reserveCapacity(maxNumberOfDomainsInOneLogStatement);

reserveInitialCapacity() since this is a brand new vector.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1003
> +    auto totalNumberOfBatches = domains.size() / maxNumberOfDomainsInOneLogStatement + (domains.size() % maxNumberOfDomainsInOneLogStatement > 0 ? 1 : 0);

unsigned numberOfBatches = std::ceil(domains.size() / static_cast<float>(maxNumberOfDomainsInOneLogStatement)); ?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1008
> +            batch.clear();

[PERF] batch.shrink(0); to clear the items but maintain the capacity.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> +            batchNumber++;

nit: ++batchNumber;

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1013
> +    if (batch.size() > 0)

!batch.isEmpty()

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:116
> +    bool resourceLoadStatisticsDebugMode() const { return m_debugModeEnabled; };

bool isDebugModeEnabled() const;

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:148
> +    bool debugModeOverride(WebCore::ResourceLoadStatistics&);

confusing naming.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> +    Vector<String> setDebugModePrevalentResources();

Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:188
> +    String m_debugManualPrevalentResource { };

{ } is not needed.

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:95
> +    m_debugModeEnabled = m_memoryStore.resourceLoadStatisticsDebugMode();

Do we need a m_debugModeEnabled data member? Could we always get its value from the memory store?

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:55
> +    void setResourceLoadStatisticsDebugMode(bool enable) { m_debugModeEnabled = enable; };

Confusing naming. Suggesting setIsDebugModeEnabled(bool);

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:187
> +void WebResourceLoadStatisticsStore::setPrevalentResourceDebugOverride(const WebCore::URL& url, CompletionHandler<void()>&& completionHandler)

Confusing naming.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:191
> +    postTask([this, url, completionHandler = WTFMove(completionHandler)]() mutable {

you're passing a URL to another thread here without isolatedCopy...

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:193
> +            m_memoryStore->setPrevalentResourceDebugOverride(url.host().toString());

So we do not need to use ResourceLoadStatistics::primaryDomain() here?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:136
> +    void setPrevalentResourceDebugOverride(const WebCore::URL& host, CompletionHandler<void()>&&);

const WebCore::URL& vs host

Make up your mind :)

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1517
>      if (m_resourceLoadStatistics)

You fail to call the completion handler in the else case.

[John Wilander]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 345452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:54
> > +static auto debugStaticPrevalentResource { "3rdpartytestwebkit.org" };
> 
> I'd prefer static const char* instead of static auto in this case, to be
> safe. Also, the variable name should probably include "Host".

Got it.

> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:323
> > +bool ResourceLoadStatisticsMemoryStore::debugModeOverride(ResourceLoadStatistics& resourceStatistic)
> 
> The method name is not clear here.

I agree.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> > +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> 
> Should this be && instead of ||, I am unclear here.

It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:988
> > +static void debugLogDomainsInBatches(const String& message, const Vector<String>& domains)
> 
> Should the first parameter be a const char* ?

I could.

> Also, wouldn't "action" be a better name for this parameter?

Maybe. I can change it.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1001
> > +    batch.reserveCapacity(maxNumberOfDomainsInOneLogStatement);
> 
> reserveInitialCapacity() since this is a brand new vector.

My mistake.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1003
> > +    auto totalNumberOfBatches = domains.size() / maxNumberOfDomainsInOneLogStatement + (domains.size() % maxNumberOfDomainsInOneLogStatement > 0 ? 1 : 0);
> 
> unsigned numberOfBatches = std::ceil(domains.size() /
> static_cast<float>(maxNumberOfDomainsInOneLogStatement)); ?

Nice.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1008
> > +            batch.clear();
> 
> [PERF] batch.shrink(0); to clear the items but maintain the capacity.

Go it.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> > +            batchNumber++;
> 
> nit: ++batchNumber;

Got it. Is this for style reasons?

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1013
> > +    if (batch.size() > 0)
> 
> !batch.isEmpty()

Got it.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:116
> > +    bool resourceLoadStatisticsDebugMode() const { return m_debugModeEnabled; };
> 
> bool isDebugModeEnabled() const;
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:148
> > +    bool debugModeOverride(WebCore::ResourceLoadStatistics&);
> 
> confusing naming.

Yup. Will fix.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> > +    Vector<String> setDebugModePrevalentResources();
> 
> Super confusing to have something that returns a Vector, starts with "set"
> and is not marked as const.

I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():

void ResourceLoadStatisticsMemoryStore::clear()
{
    ...
    auto primaryDomainsToBlock = setDebugModePrevalentResources();
    updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
}

Any thoughts on how we make this nicer?

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:188
> > +    String m_debugManualPrevalentResource { };
> 
> { } is not needed.

Got it.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:95
> > +    m_debugModeEnabled = m_memoryStore.resourceLoadStatisticsDebugMode();
> 
> Do we need a m_debugModeEnabled data member? Could we always get its value
> from the memory store?

Maybe. Let me have a look. It would be nice since we would avoid any out-of-sync situations.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:55
> > +    void setResourceLoadStatisticsDebugMode(bool enable) { m_debugModeEnabled = enable; };
> 
> Confusing naming. Suggesting setIsDebugModeEnabled(bool);

Got it.

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:187
> > +void WebResourceLoadStatisticsStore::setPrevalentResourceDebugOverride(const WebCore::URL& url, CompletionHandler<void()>&& completionHandler)
> 
> Confusing naming.

Yup.

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:191
> > +    postTask([this, url, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> you're passing a URL to another thread here without isolatedCopy...

Got it.

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:193
> > +            m_memoryStore->setPrevalentResourceDebugOverride(url.host().toString());
> 
> So we do not need to use ResourceLoadStatistics::primaryDomain() here?

Since it's a developer facing thing, I just reckoned they'll figure it out.

> > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:136
> > +    void setPrevalentResourceDebugOverride(const WebCore::URL& host, CompletionHandler<void()>&&);
> 
> const WebCore::URL& vs host
> 
> Make up your mind :)

Will do.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1517
> >      if (m_resourceLoadStatistics)
> 
> You fail to call the completion handler in the else case.

Oups. Will fix.

Thanks, Chris!

[Chris Dumez]

Comment on attachment 345452 [details]
Patch

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

>>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
>>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
>> 
>> Should this be && instead of ||, I am unclear here.
> 
> It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?

Ok, just making sure.

>>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
>>> +            batchNumber++;
>> 
>> nit: ++batchNumber;
> 
> Got it. Is this for style reasons?

Yes, webkit style. For some types it could also be more efficient but not here since it is is a builtin.

>>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
>>> +    Vector<String> setDebugModePrevalentResources();
>> 
>> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> 
> I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> 
> void ResourceLoadStatisticsMemoryStore::clear()
> {
>     ...
>     auto primaryDomainsToBlock = setDebugModePrevalentResources();
>     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> }
> 
> Any thoughts on how we make this nicer?

Hard for me to come up with names since I am not 100% sure that these things actually do :) Let me take a shot though:
Vector<String> prevalentResourceSetByDebugMode() const; ?

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 345452 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> 
> >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> >>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> >> 
> >> Should this be && instead of ||, I am unclear here.
> > 
> > It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?
> 
> Ok, just making sure.
> 
> >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> >>> +            batchNumber++;
> >> 
> >> nit: ++batchNumber;
> > 
> > Got it. Is this for style reasons?
> 
> Yes, webkit style. For some types it could also be more efficient but not
> here since it is is a builtin.
> 
> >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> >>> +    Vector<String> setDebugModePrevalentResources();
> >> 
> >> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> > 
> > I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> > 
> > void ResourceLoadStatisticsMemoryStore::clear()
> > {
> >     ...
> >     auto primaryDomainsToBlock = setDebugModePrevalentResources();
> >     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> > }
> > 
> > Any thoughts on how we make this nicer?
> 
> Hard for me to come up with names since I am not 100% sure that these things
> actually do :) Let me take a shot though:
> Vector<String> prevalentResourceSetByDebugMode() const; ?

Oops, typo:

Vector<String> prevalentResourcesSetByDebugMode() const;
or
Vector<String> prevalentResourcesSetForDebugMode() const;

[John Wilander]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 345452 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> > 
> > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> > >>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> > >> 
> > >> Should this be && instead of ||, I am unclear here.
> > > 
> > > It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?
> > 
> > Ok, just making sure.
> > 
> > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> > >>> +            batchNumber++;
> > >> 
> > >> nit: ++batchNumber;
> > > 
> > > Got it. Is this for style reasons?
> > 
> > Yes, webkit style. For some types it could also be more efficient but not
> > here since it is is a builtin.
> > 
> > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> > >>> +    Vector<String> setDebugModePrevalentResources();
> > >> 
> > >> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> > > 
> > > I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> > > 
> > > void ResourceLoadStatisticsMemoryStore::clear()
> > > {
> > >     ...
> > >     auto primaryDomainsToBlock = setDebugModePrevalentResources();
> > >     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> > > }
> > > 
> > > Any thoughts on how we make this nicer?
> > 
> > Hard for me to come up with names since I am not 100% sure that these things
> > actually do :) Let me take a shot though:
> > Vector<String> prevalentResourceSetByDebugMode() const; ?
> 
> Oops, typo:
> 
> Vector<String> prevalentResourcesSetByDebugMode() const;
> or
> Vector<String> prevalentResourcesSetForDebugMode() const;

But it can't be const since it's setting members of the store. Let me explain what it's doing.

setDebugModePrevalentResources() is a function that, if debug mode is enabled, sets 3rdpartytestwebkit.org and any custom domain added through setPrevalentResourceDebugOverride() as prevalent. It is used to make sure that what ever you do, those two domains remain prevalent in debug mode. If you clear all history, the clear() function is called and setDebugModePrevalentResources() immediately sets the two domains to prevalent again.

The reason why I return a vector is to not have to iterate through the store again to find the one or two newly set prevalent resources. But maybe I can conditionalize it on debug mode and only then iterate?

[Chris Dumez]

(In reply to John Wilander from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > Comment on attachment 345452 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> > > 
> > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> > > >>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> > > >> 
> > > >> Should this be && instead of ||, I am unclear here.
> > > > 
> > > > It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?
> > > 
> > > Ok, just making sure.
> > > 
> > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> > > >>> +            batchNumber++;
> > > >> 
> > > >> nit: ++batchNumber;
> > > > 
> > > > Got it. Is this for style reasons?
> > > 
> > > Yes, webkit style. For some types it could also be more efficient but not
> > > here since it is is a builtin.
> > > 
> > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> > > >>> +    Vector<String> setDebugModePrevalentResources();
> > > >> 
> > > >> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> > > > 
> > > > I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> > > > 
> > > > void ResourceLoadStatisticsMemoryStore::clear()
> > > > {
> > > >     ...
> > > >     auto primaryDomainsToBlock = setDebugModePrevalentResources();
> > > >     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> > > > }
> > > > 
> > > > Any thoughts on how we make this nicer?
> > > 
> > > Hard for me to come up with names since I am not 100% sure that these things
> > > actually do :) Let me take a shot though:
> > > Vector<String> prevalentResourceSetByDebugMode() const; ?
> > 
> > Oops, typo:
> > 
> > Vector<String> prevalentResourcesSetByDebugMode() const;
> > or
> > Vector<String> prevalentResourcesSetForDebugMode() const;
> 
> But it can't be const since it's setting members of the store. Let me
> explain what it's doing.
> 
> setDebugModePrevalentResources() is a function that, if debug mode is
> enabled, sets 3rdpartytestwebkit.org and any custom domain added through
> setPrevalentResourceDebugOverride() as prevalent. It is used to make sure
> that what ever you do, those two domains remain prevalent in debug mode. If
> you clear all history, the clear() function is called and
> setDebugModePrevalentResources() immediately sets the two domains to
> prevalent again.
> 
> The reason why I return a vector is to not have to iterate through the store
> again to find the one or two newly set prevalent resources. But maybe I can
> conditionalize it on debug mode and only then iterate?

Ok, another attempt then:
void setPrevalentResourcesForDebugMode(Vector<String>& prevalentResources);

[John Wilander]

(In reply to Chris Dumez from comment #11)
> (In reply to John Wilander from comment #10)
> > (In reply to Chris Dumez from comment #9)
> > > (In reply to Chris Dumez from comment #8)
> > > > Comment on attachment 345452 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> > > > 
> > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> > > > >>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> > > > >> 
> > > > >> Should this be && instead of ||, I am unclear here.
> > > > > 
> > > > > It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?
> > > > 
> > > > Ok, just making sure.
> > > > 
> > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> > > > >>> +            batchNumber++;
> > > > >> 
> > > > >> nit: ++batchNumber;
> > > > > 
> > > > > Got it. Is this for style reasons?
> > > > 
> > > > Yes, webkit style. For some types it could also be more efficient but not
> > > > here since it is is a builtin.
> > > > 
> > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> > > > >>> +    Vector<String> setDebugModePrevalentResources();
> > > > >> 
> > > > >> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> > > > > 
> > > > > I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> > > > > 
> > > > > void ResourceLoadStatisticsMemoryStore::clear()
> > > > > {
> > > > >     ...
> > > > >     auto primaryDomainsToBlock = setDebugModePrevalentResources();
> > > > >     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> > > > > }
> > > > > 
> > > > > Any thoughts on how we make this nicer?
> > > > 
> > > > Hard for me to come up with names since I am not 100% sure that these things
> > > > actually do :) Let me take a shot though:
> > > > Vector<String> prevalentResourceSetByDebugMode() const; ?
> > > 
> > > Oops, typo:
> > > 
> > > Vector<String> prevalentResourcesSetByDebugMode() const;
> > > or
> > > Vector<String> prevalentResourcesSetForDebugMode() const;
> > 
> > But it can't be const since it's setting members of the store. Let me
> > explain what it's doing.
> > 
> > setDebugModePrevalentResources() is a function that, if debug mode is
> > enabled, sets 3rdpartytestwebkit.org and any custom domain added through
> > setPrevalentResourceDebugOverride() as prevalent. It is used to make sure
> > that what ever you do, those two domains remain prevalent in debug mode. If
> > you clear all history, the clear() function is called and
> > setDebugModePrevalentResources() immediately sets the two domains to
> > prevalent again.
> > 
> > The reason why I return a vector is to not have to iterate through the store
> > again to find the one or two newly set prevalent resources. But maybe I can
> > conditionalize it on debug mode and only then iterate?
> 
> Ok, another attempt then:
> void setPrevalentResourcesForDebugMode(Vector<String>& prevalentResources);

Yeah, I considered that one early on but it's confusing in its own way since it looks like your setting something to that vector. Maybe I can rename the vector something-something-result.

[Chris Dumez]

(In reply to John Wilander from comment #12)
> (In reply to Chris Dumez from comment #11)
> > (In reply to John Wilander from comment #10)
> > > (In reply to Chris Dumez from comment #9)
> > > > (In reply to Chris Dumez from comment #8)
> > > > > Comment on attachment 345452 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=345452&action=review
> > > > > 
> > > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:549
> > > > > >>> +    m_debugLoggingEnabled = m_debugLoggingEnabled || enable;
> > > > > >> 
> > > > > >> Should this be && instead of ||, I am unclear here.
> > > > > > 
> > > > > > It should be || because I don't what a setResourceLoadStatisticsDebugMode(false) to turn off debug logging if debug logging has been turned on through other means such as User Defaults. Should I add a comment?
> > > > > 
> > > > > Ok, just making sure.
> > > > > 
> > > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1009
> > > > > >>> +            batchNumber++;
> > > > > >> 
> > > > > >> nit: ++batchNumber;
> > > > > > 
> > > > > > Got it. Is this for style reasons?
> > > > > 
> > > > > Yes, webkit style. For some types it could also be more efficient but not
> > > > > here since it is is a builtin.
> > > > > 
> > > > > >>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:149
> > > > > >>> +    Vector<String> setDebugModePrevalentResources();
> > > > > >> 
> > > > > >> Super confusing to have something that returns a Vector, starts with "set" and is not marked as const.
> > > > > > 
> > > > > > I know. :( This was me trying to fix the existing call to updateCookiePartitioningForDomains() in clear():
> > > > > > 
> > > > > > void ResourceLoadStatisticsMemoryStore::clear()
> > > > > > {
> > > > > >     ...
> > > > > >     auto primaryDomainsToBlock = setDebugModePrevalentResources();
> > > > > >     updateCookiePartitioningForDomains({ }, primaryDomainsToBlock, { }, ShouldClearFirst::Yes, []() { });
> > > > > > }
> > > > > > 
> > > > > > Any thoughts on how we make this nicer?
> > > > > 
> > > > > Hard for me to come up with names since I am not 100% sure that these things
> > > > > actually do :) Let me take a shot though:
> > > > > Vector<String> prevalentResourceSetByDebugMode() const; ?
> > > > 
> > > > Oops, typo:
> > > > 
> > > > Vector<String> prevalentResourcesSetByDebugMode() const;
> > > > or
> > > > Vector<String> prevalentResourcesSetForDebugMode() const;
> > > 
> > > But it can't be const since it's setting members of the store. Let me
> > > explain what it's doing.
> > > 
> > > setDebugModePrevalentResources() is a function that, if debug mode is
> > > enabled, sets 3rdpartytestwebkit.org and any custom domain added through
> > > setPrevalentResourceDebugOverride() as prevalent. It is used to make sure
> > > that what ever you do, those two domains remain prevalent in debug mode. If
> > > you clear all history, the clear() function is called and
> > > setDebugModePrevalentResources() immediately sets the two domains to
> > > prevalent again.
> > > 
> > > The reason why I return a vector is to not have to iterate through the store
> > > again to find the one or two newly set prevalent resources. But maybe I can
> > > conditionalize it on debug mode and only then iterate?
> > 
> > Ok, another attempt then:
> > void setPrevalentResourcesForDebugMode(Vector<String>& prevalentResources);
> 
> Yeah, I considered that one early on but it's confusing in its own way since
> it looks like your setting something to that vector. Maybe I can rename the
> vector something-something-result.

Vector<String> ensurePrevalentResourcesForDebugMode();

[John Wilander]

Created attachment 345489 [details]
Patch

[John Wilander]

I hope I got all the things fixed.

[Chris Dumez]

Comment on attachment 345489 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:323
> +bool ResourceLoadStatisticsMemoryStore::isDebugModeOverride(ResourceLoadStatistics& resourceStatistic)

Still unclear IMO. maybe isPrevalentDueToDebugMode() ?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:999
> +    batch.reserveCapacity(maxNumberOfDomainsInOneLogStatement);

reserveInitialCapacity

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1007
> +            batchNumber++;

++batchNumber

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:-968
> -#if !RELEASE_LOG_DISABLED

Shouldn't we keep this?

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:116
> +    bool resourceLoadStatisticsDebugMode() const { return m_debugModeEnabled; };

isDebugModeEnabled()

boolean getters without a prefix are very unclear.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:117
> +    void setPrevalentResourceForDebugMode(const String&);

I would keep the parameter name for clarity here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:188
> +    String m_debugManualPrevalentResource { };

Unnecessary { }

[John Wilander]

Created attachment 345493 [details]
Patch

[John Wilander]

Comment on attachment 345493 [details]
Patch

Thanks, Chris! Fixed the remaining issues except for the extra #if !RELEASE_LOG_DISABLED. It makes the function unused on non-Cocoa platforms and I check in the function where the log statements are anyway.

I'll wait for the bots to go green before putting on the queue.

[WebKit Commit Bot]

Comment on attachment 345493 [details]
Patch

Clearing flags on attachment: 345493

Committed r234080: <https://trac.webkit.org/changeset/234080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.