[webkit-reviews] review granted: [Bug 190466] [GTK][WPE] Add DeviceIdHashSaltStorage disk persistency : [Attachment 357677] 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 357677: Patch

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




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

LGTM with proposed changes below.
Unit tests covering private browsing might be good too, now or as a follow-up.

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

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:733
> +    // We have to remove the hash salts when cookies are removed.

I would add a FIXME comment then, since this should be dealt with in platform
generic code.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:58
> +	   origins.add(hashSaltForOrigin.value->parentOrigin.isolatedCopy());

Do we need to isolate copy these?
It seems there is no threading issue, it is all main thread.
Maybe we should add an ASSERT here for main thread.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:64
> +DeviceIdHashSaltStorage::DeviceIdHashSaltStorage(const String&
deviceIdHashSaltStorageDirectory, bool isPersistent)

We probably do not need isPersistent.
If deviceIdHashSaltStorageDirectory, isPersistent is false and we do not need
to compute m_deviceIdHashSaltStorageDirectory.
The persistency check can be done as
m_deviceIdHashSaltStorageDirectory.isEmpty().

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

s/auto/auto&&/ might be better.
No need for protectedThis here since loadStorageFromDisk is already refing
'this'.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:79
> +	   }

I would write it this way:
auto m_pendingGetCompletionHandlers = WTFMove(m_pendingGetCompletionHandler);
for (auto& completionHandler : m_pendingGetCompletionHandler)
    this->completePendingHandler(WTFMove(completionHandler));
No need for clear.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:86
> +	   }

Ditto here.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:157
> +std::unique_ptr<DeviceIdHashSaltStorage::HashSaltForOrigin>
DeviceIdHashSaltStorage::getDataFromDecoder(KeyedDecoder* decoder, String&&
deviceIdHashSalt) const

Can be free function.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:184
> +std::unique_ptr<KeyedEncoder>
DeviceIdHashSaltStorage::createEncoderFromData(const HashSaltForOrigin&
hashSaltForOrigin) const

Ditto.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:204
>      auto& deviceIdHashSalt = m_deviceIdHashSaltForOrigins.ensure(origins,
[&documentOrigin, &parentOrigin] () {

It is fine as is.
I wonder though whether it would be clearer to do:
[documentOrigin = WTFMove(documentOrigin), parentOrigin=
WTFMove(parentOrigin)]() mutable

That way, it is clearer that documentOrigin is moved at that point.
But this has a penalty cost as we would call the move constructor.
Let's keep it like this then, given the method is not long.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:223
> +	   storeHashSaltToDisk(*deviceIdHashSalt.get());

I would tend to move the if check in storeHashSaltToDisk and make it exit early
if m_isPersistent is false.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:247
> +	   m_pendingGetCompletionHandler.append(WTFMove(completionHandler));

I think we can make m_pendingGetCompletionHandler and this completionHandler be
a CompletionHandler<void()>.
This way, m_pendingGetCompletionHandler and m_pendingDeviceIdHashSaltForOrigin
can be merged as one Vector.

This can be done by writing:
m_pendingGetCompletionHandlers.append([this, completionHandler	=
WTFMove(completionHandler)] {
    this->completePendingHandler(WTFMove(completionHandler));
});

Note that we probably need to iterate over m_pendingGetCompletionHandlers in
DeviceIdHashSaltStorage destructor.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:285
> +	       this->deleteHashSaltFromDisk(*keyAndValue.value.get());

If none persistent, this call is not needed. Let's have deleteHashSaltFromDisk
exit early if !m_isPersistent.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:42
> +class UserMediaPermissionRequestManagerProxy : public
CanMakeWeakPtr<UserMediaPermissionRequestManagerProxy> {

I wonder whether it can be a private CanMakeWeakPtr<>

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:95
> +    ,
m_deviceIdHashSaltStorage(DeviceIdHashSaltStorage::create(API::WebsiteDataStore
::defaultDeviceIdHashSaltsStorageDirectory(), isPersistent()))

Should we use m_configuration->deviceIdHashSaltsStorageDirectory() instead of
the default one?