Bug 151221

Created attachment 265432 [details] WIP

Created attachment 266468 [details] Patch

Created attachment 266559 [details] Patch

Comment on attachment 266559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266559&action=review > Source/WebCore/ChangeLog:53 > + (-[CDMSessionAVStreamSessionObserver initWithParent:]): Moved from AVSessionMediaSourceAVFObjC. Don't you mean CDMSessionMediaSourceAVFObjCObserver? > Source/WebCore/Modules/encryptedmedia/MediaKeySession.cpp:121 > + if (errorCode) { Oops :-O > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:54 > + virtual RefPtr<Uint8Array> generateKeyRequest(const String& mimeType, Uint8Array* initData, String& destinationURL, unsigned short& errorCode, unsigned long& systemCode) override; > + virtual void releaseKeys() override; > + virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override; Nit: why name initData and not the first parameter to update? Why name nextMessage? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:82 > +inline CDMSessionAVContentKeySession* toCDMSessionAVContentKeySession(CDMSession* session) > +{ > + if (!session || session->type() != CDMSessionTypeAVContentKeySession) > + return nullptr; > + return static_cast<CDMSessionAVContentKeySession*>(session); Is it ever OK for someone to call this with anything except a CDMSessionTypeAVContentKeySession? If not, you should add an ASSERT. > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:97 > + > + Nit: don't need two blank lines. > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:171 > + if (equalIgnoringCase(mimeType, "keyrelease")) { > + m_mode = KeyRelease; > + return generateKeyReleaseMessage(errorCode, systemCode); > + } > + > + if (!m_certificate) { > + String certificateString(ASCIILiteral("certificate")); > + RefPtr<Uint8Array> array = Uint8Array::create(certificateString.length()); > + for (unsigned i = 0, length = certificateString.length(); i < length; ++i) > + array->set(i, certificateString[i]); > + return array; > + } It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:183 > + if (hasContentKeySession()) { Nit: this could be an early return. > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:229 > +static bool isEqual(Uint8Array* data, const char* literal) > +{ > + ASSERT(data); > + ASSERT(literal); > + unsigned length = data->length(); > + > + for (unsigned i = 0; i < length; ++i) { > + if (!literal[i]) > + return false; > + > + if (data->item(i) != static_cast<uint8_t>(literal[i])) > + return false; > + } > + return !literal[length]; > +} It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:286 > + NSData* requestData = [m_keyRequest contentKeyRequestDataForApp:certificateData.get() contentIdentifier:initData.get() options:options.get() error:nil]; Doesn't this leak? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:319 > + systemCode = '!mor'; Can you return '!mor' from a method in the base class? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. :-O > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:52 > + virtual RefPtr<Uint8Array> generateKeyRequest(const String& mimeType, Uint8Array* initData, String& destinationURL, unsigned short& errorCode, unsigned long& systemCode) override; > + virtual void releaseKeys() override; > + virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override; Ditto my nit from the other class header. > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved. :-O > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:140 > +void CDMSessionAVStreamSession::releaseKeys() This whole method is annoyingly similar to CDMSessionAVContentKeySession::releaseKeys... > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:216 > + bool shouldGenerateKeyRequest = !m_certificate || isEqual(key, "renew"); > + if (!m_certificate) { > + LOG(Media, "CDMSessionAVStreamSession::update(%p) - certificate data", this); > + > + m_certificate = key; > + } > + > + if (isEqual(key, "acknowledged")) { > + LOG(Media, "CDMSessionAVStreamSession::update(%p) - acknowleding secure stop message", this); > + > + if (!m_expiredSession) { > + errorCode = MediaPlayer::InvalidPlayerState; > + return false; > + } > + > + RetainPtr<NSData> certificateData = adoptNS([[NSData alloc] initWithBytes:m_certificate->data() length:m_certificate->length()]); > + > + if ([getAVStreamSessionClass() respondsToSelector:@selector(removePendingExpiredSessionReports:withAppIdentifier:storageDirectoryAtURL:)]) > + [getAVStreamSessionClass() removePendingExpiredSessionReports:@[m_expiredSession.get()] withAppIdentifier:certificateData.get() storageDirectoryAtURL:[NSURL fileURLWithPath:storagePath()]]; > + m_expiredSession = nullptr; > + return true; > + } > + > + if (m_mode == KeyRelease) > + return false; It looks like this is exactly the same as the first part of CDMSessionAVContentKeySession::update. Can you put it into a shared method in the base class? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:237 > + > + Nit: two blank lines. > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionMediaSourceAVFObjC.h:2 > + * Copyright (C) 2015 Apple Inc. All rights reserved. 2014-2015? > Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionMediaSourceAVFObjC.mm:2 > - * Copyright (C) 2014 Apple Inc. All rights reserved. > + * Copyright (C) 2015 Apple Inc. All rights reserved. Ditto.

Comment on attachment 266559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266559&action=review >> Source/WebCore/ChangeLog:53 >> + (-[CDMSessionAVStreamSessionObserver initWithParent:]): Moved from AVSessionMediaSourceAVFObjC. > > Don't you mean CDMSessionMediaSourceAVFObjCObserver? I actually meant the source file, but you're right. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:54 >> + virtual bool update(Uint8Array*, RefPtr<Uint8Array>& nextMessage, unsigned short& errorCode, unsigned long& systemCode) override; > > Nit: why name initData and not the first parameter to update? Why name nextMessage? Copy pasta. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.h:82 >> + return static_cast<CDMSessionAVContentKeySession*>(session); > > Is it ever OK for someone to call this with anything except a CDMSessionTypeAVContentKeySession? If not, you should add an ASSERT. Ok. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:171 >> + } > > It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class? That may be hard; I'll have to do some refactoring, but i'll do that in a follow up. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:229 >> +} > > It looks like this is exactly the same in CDMSessionAVStreamSession, can you put it in the base class? Not without making it a class method. I'll see what I can do. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:286 >> + NSData* requestData = [m_keyRequest contentKeyRequestDataForApp:certificateData.get() contentIdentifier:initData.get() options:options.get() error:nil]; > > Doesn't this leak? The return value is autoreleased, so no. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVContentKeySession.mm:319 >> + systemCode = '!mor'; > > Can you return '!mor' from a method in the base class? I can probably refactor things, or add a constant static in the base class. >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.h:2 >> + * Copyright (C) 2014 Apple Inc. All rights reserved. > > :-O Whoops! >> Source/WebCore/platform/graphics/avfoundation/objc/CDMSessionAVStreamSession.mm:216 >> + return false; > > It looks like this is exactly the same as the first part of CDMSessionAVContentKeySession::update. Can you put it into a shared method in the base class? Same as above; i'll have do so some refactoring to get there.

Committed r193479: <http://trac.webkit.org/changeset/193479>