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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 22 12:11:51 PDT 2019

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

Attachment 376954: Patch


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

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,
>> 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

>>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:715
>>> +void
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

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

>>> Source/WebKit/WebProcess/WebProcess.cpp:221
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.

More information about the webkit-reviews mailing list