[webkit-reviews] review granted: [Bug 190466] [GTK][WPE] Add DeviceIdHashSaltStorage disk persistency : [Attachment 357949] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 190466: [GTK][WPE] Add DeviceIdHashSaltStorage disk persistency
https://bugs.webkit.org/show_bug.cgi?id=190466

Attachment 357949: Patch

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




--- Comment #32 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 357949
  --> https://bugs.webkit.org/attachment.cgi?id=357949
Patch

LGTM.
Some improvements below that can either be done now or as follow-ups.

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

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:100
> +    void setDeviceIdHashSaltsStorageDirectory(const WTF::String& directory)
{ m_deviceIdHashSaltsStorageDirectory = directory; }

Could be String&&

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:63
> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler
= WTFMove(completionHandler)]() mutable {

Let's add a FIXME then.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:72
> +    if (m_deviceIdHashSaltStorageDirectory.isEmpty()) {

In the future, I would tend to lazily load the storage when we first use the
device storage.
Can we add a FIXME?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:77
> +    loadStorageFromDisk([this, protectedThis = makeRef(*this)] (auto&&
deviceIdHashSaltForOrigins) {

Agreed that we currently need to protectedThis here.
The alternative would be to just call loadStorageFromDisk without any lambda.
In loadStorageFromDisk, we would take a ref when going to background thread and
moving the ref when going back to main thread.
When going back to main thread, we could then call the lambda code as a method
of 68DeviceIdHashSaltStorage for instance.

Not needed to land this change though.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:110
> +{

Maybe ASSERT(!m_deviceIdHashSaltStorageDirectory.isEmpty());

Should we also ASSERT(!m_isLoaded), or return early in that case as well.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:183
> +    hashSaltForOrigin->lastTimeUsed =
WallTime::fromRawSeconds(lastTimeUsed);

We could change HashSaltForOrigin constructor so that lastTimeUsed is given in
the constructor
That way, we could write it:
return std::make_unique<HashSaltForOrigin>(WTFMove(securityOriginData.value()),
WTFMove(parentSecurityOriginData.value()), WTFMove(deviceIdHashSalt),
WallTime::fromRawSeconds(lastTimeUsed));

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:224
> +	   return newHashSaltForOrigin;

Write it as:
return std::make_unique<HashSaltForOrigin>(WTFMove(documentOrigin),
WTFMove(parentOrigin), WTFMove(deviceIdHashSalt));

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:239
> +	   m_pendingCompletionHandlers.append([this, documentOrigin =
documentOrigin.data().isolatedCopy(), parentOrigin =
parentOrigin.data().isolatedCopy(), completionHandler =
WTFMove(completionHandler)]() mutable {

No need to isolatedCopy these, the pending completion handler should always be
destroyed in main thread anyway.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:264
> +    m_queue->dispatch([this, protectedThis = makeRef(*this),
deviceIdHashSalt = hashSaltForOrigin.deviceIdHashSalt.isolatedCopy()]() mutable
{

To not require refing protectedThis, we could compute fileFullPath in the
lambda.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:278
> +	   if (m_deviceIdHashSaltStorageDirectory.isEmpty())

I would move this check inside deleteHashSaltFromDisk.
That would make it clearer.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:62
> +	       isolatedCopy.lastTimeUsed = lastTimeUsed;

Adding lastTimeUsed in constructor would ease writing this line.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:72
> +    DeviceIdHashSaltStorage(const String& deviceIdHashSaltStorageDirectory);

Add explicit.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:75
> +    void deleteHashSaltFromDisk(const HashSaltForOrigin&);

I would make it consistent for all three methods and return early whenever
m_deviceIdHashSaltStorageDirectory is empty.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:77
> +    std::unique_ptr<HashSaltForOrigin>
getDataFromDecoder(WebCore::KeyedDecoder*, String&& deviceIdHashSalt) const;

Should be KeyedDecoder& instead of KeyedDecoder* probably.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:79
> +    void completeDeviceIdHashSaltForOriginCall(WebCore::SecurityOriginData&&
documentOrigin, WebCore::SecurityOriginData&& parentOrigin,
CompletionHandler<void(String&&)>&&);

Maybe we should make these two names (completePendingHandler and
completeDeviceIdHashSaltForOriginCall) consistent.
Something like:
completePendingHandler -> completeOriginsRetrievalHandler
completeDeviceIdHashSaltForOriginCall ->
completeDeviceIdHashSaltRetrievalHandler