[webkit-reviews] review denied: [Bug 188376] Add CENC sanitization : [Attachment 346700] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 188376: Add CENC sanitization
https://bugs.webkit.org/show_bug.cgi?id=188376

Attachment 346700: Patch

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




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

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

I guess you also have to fix the ios builds

> Source/WebCore/Modules/encryptedmedia/InitDataRegistry.cpp:125
> +	       Ref<SharedBuffer> keyIDBuffer =
SharedBuffer::create(WTFMove(value));
> +	       keyIDs.append(WTFMove(keyIDBuffer));

You can collapse these two lines, save the variable and the move.

>
Source/WebCore/platform/graphics/iso/ISOProtectionSystemSpecificHeaderBox.cpp:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

I guess copyright does not go to Apple here, right?

>
Source/WebCore/platform/graphics/iso/ISOProtectionSystemSpecificHeaderBox.cpp:5
1
> +    auto systemID = buffer->slice(offset, offset + 16);
> +    if (!systemID)
> +	   return false;
> +
> +    offset += 16;
> +
> +    m_systemID.resize(16);
> +    memcpy(m_systemID.data(), systemID->data(), 16);

I don't have a strong preference on this one but since you are copying the
value and even when slicing does not copy the value in all cases, we might want
to do the maths ourselves and avoid slicing. Same for the key ids and data.

> Source/WebCore/platform/graphics/iso/ISOProtectionSystemSpecificHeaderBox.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

Ditto.

>
Source/WebCore/platform/graphics/iso/ISOProtectionSystemSpecificHeaderBox.h:29
> +#include "ISOProtectionSystemSpecificHeaderBox.h"

Is this file including itself or did I miss any subtle difference?