[webkit-reviews] review granted: [Bug 205999] [EME][ClearKey] Refactor CDMClearKey::update() : [Attachment 387218] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 205999: [EME][ClearKey] Refactor CDMClearKey::update()
https://bugs.webkit.org/show_bug.cgi?id=205999

Attachment 387218: Patch

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




--- Comment #2 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 387218
  --> https://bugs.webkit.org/attachment.cgi?id=387218
Patch

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

> Source/WebCore/ChangeLog:17
> +	   (WebCore::sharedBufferAsHexString): Helper method to convert a
shared buffer containing bytes into a hex string, useful for debug prints.
> +	   (WebCore::CDMInstanceClearKey::Key::keyIDAsString const): Uses the
helper to return a string containing the hex values of the key ID.
> +	   (WebCore::CDMInstanceClearKey::Key::keyValueAsString const): Ditto
for the key value.
> +	   (WebCore::operator==): Added comparison operators to refactor
updateLicense's duplication of comparisons.
> +	   (WebCore::operator<): Ditto.
> +	   (WebCore::CDMInstanceSessionClearKey::updateLicense): Refactor to
rely on equality comparisons in one place, the key class, and to remove thorny
open-coded memcmp's in the middle of conditionals which made the method
unnecessarily hard to reason about. The extra copying of the key status vector
from the key store has been eliminated, instead the vector is sorted in-place
directly inside the HashMap, which also cleaned up the method further.
> +	   * platform/encryptedmedia/clearkey/CDMClearKey.h:

Aren't these lines two long? (This is funny coming from me)

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:543
> +static String sharedBufferAsHexString(const RefPtr<SharedBuffer> buf)

& or directly move this to SharedBuffer::toHexString?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:545
> +    StringBuilder sb;

stringBuilder

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:550
> +	   if (byteOffset < buf->size() - 1)
> +	       sb.append(' ');

Are appending a space after each byte? Do you think it is needed?

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:565
> +bool operator==(const CDMInstanceClearKey::Key &k1, const
CDMInstanceClearKey::Key &k2)

Move the & next to the type, both parameters

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573
> +bool operator<(const CDMInstanceClearKey::Key &k1, const
CDMInstanceClearKey::Key &k2)

Ditto

> Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:582
> +    if (k1.keyIDData->size() != k2.keyIDData->size())
> +	   return k1.keyIDData->size() < k2.keyIDData->size();
> +
> +    size_t minimumBufferSize = std::min(k1.keyIDData->size(),
k2.keyIDData->size());
> +    return memcmp(k1.keyIDData->data(), k2.keyIDData->data(),
minimumBufferSize);

Aren't the buffers going to have the same size after the if? I think you can
remove the minimum size and use one of the sizes, right? Am I missing anything?