[webkit-reviews] review denied: [Bug 198923] Support ITP on a per-session basis : [Attachment 376954] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Katherine_cheney at apple.com's request
for review:
Bug 198923: Support ITP on a per-session basis
https://bugs.webkit.org/show_bug.cgi?id=198923

Attachment 376954: Patch

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 376954
  --> https://bugs.webkit.org/attachment.cgi?id=376954
Patch

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

>>> Source/WebCore/loader/ResourceLoadObserver.cpp:365
>>> +	     addResult.iterator->value = makeUnique<HashMap<RegistrableDomain,
ResourceLoadStatistics>>();
>> 
>> You can use m_perSessionResourceStatisticsMap.ensure here.
> 
> I think add is more readable. However, if there is a performance reason to
use ensure I will happily change it.

ah ah, I made the same comment than Youenn offline. It feel very weird to add
nullptr and then overwrite the value with something else, now that we have
ensure(), not to mention this is unnecessary work. I do believe ensure() is
faster for this reason, but I have not measured.

> Source/WebCore/loader/ResourceLoadObserver.cpp:380
> +    auto mapIter = m_perSessionResourceStatisticsMap.find(sessionID);

I think this would look better with a get() than a find().

> Source/WebCore/loader/ResourceLoadObserver.cpp:-390
> -    m_resourceStatisticsMap.clear();

Is it me or is m_resourceStatisticsMap no longer cleared now? This seems like a
bug.

>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:715
>>> +void
NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(HashMap<PAL::Sessi
onID, Vector<ResourceLoadStatistics>>&& statistics)
>> 
>> There is no real need to have a HashMap here, a Vector<struct {SessionID,
ResourceLoadStatistics}> would be good enough and probably more efficient.
> 
> Sounds good. Would a Vector of tuples or pairs be easier than declaring a new
struct?

Yes, a vector of std::pair would be fine IMO.

>>> Source/WebKit/WebProcess/WebProcess.cpp:221
>>>	    
ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionT
oWebProcess::ResourceLoadStatisticsUpdated(WTFMove(statistics)), 0);
>> 
>> I believe there is no need to WTFMove(statistics) here
> 
> Okay. I'm not familiar with why -- can we be sure that statistics won't be
copied without the WTFMove?

The WTFMove() does not hurt here, it also does not do anything. I would just
keep it personally.