Summary: | [EME][ClearKey] Refactor CDMClearKey::update() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Charlie Turner <cturner> | ||||||||
Component: | WebKitGTK | Assignee: | Charlie Turner <cturner> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Charlie Turner
2020-01-09 06:35:13 PST
Created attachment 387218 [details]
Patch
Comment on attachment 387218 [details] 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? (In reply to Xabier Rodríguez Calvar from comment #2) > Comment on attachment 387218 [details] > 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) Hehe, fixed. > > > 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 Fixed. > > > 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? I don't know how to get the `hex` modified to zero pad, without that it can be hard, without the added spaces, to see the individual byte values, since a byte value of 5 between, say, a1 and b2 would look like a15b2 and it's get confusing... > > > 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 Fixed. > > > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573 > > +bool operator<(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2) > > Ditto Fixed. > > > 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? You are right, the defensive code was from a previous iteration. Fixed. Created attachment 387238 [details]
Patch
Comment on attachment 387238 [details] Patch Rejecting attachment 387238 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 387238, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/13301813 (In reply to Charlie Turner from comment #3) > > > > > 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? > > I don't know how to get the `hex` modified to zero pad, without that it can > be hard, without the added spaces, to see the individual byte values, since > a byte value of 5 between, say, a1 and b2 would look like a15b2 and it's get > confusing... And wouldn't it be better to write a105b2 instead, cause the 0 is actually there. Comment on attachment 387238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387238&action=review > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:543 > +static String sharedBufferAsHexString(const RefPtr<SharedBuffer>& buf) The more I look at this, the more I think we should move it to SharedBuffer::toHexString... > Source/WebCore/platform/encryptedmedia/clearkey/CDMClearKey.cpp:573 > +bool operator<(const CDMInstanceClearKey::Key &k1, const CDMInstanceClearKey::Key &k2) & next to the type! Created attachment 387321 [details]
Patch
Comment on attachment 387321 [details] Patch Clearing flags on attachment: 387321 Committed r254334: <https://trac.webkit.org/changeset/254334> All reviewed patches have been landed. Closing bug. |