Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17101027:
Add a function to convert key system into UUID (Closed)
|
Created:
7 years, 6 months ago by qinmin Modified:
7 years, 6 months ago Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a function to convert key system into UUID
BUG=163552
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207815
Patch Set 1 #Patch Set 2 : moving the function into MediaDrmBridge #
Total comments: 6
Patch Set 3 : comments #
Total comments: 13
Patch Set 4 : addressing ddorwin's comments #
Total comments: 23
Patch Set 5 : move changes into key_system_info #Patch Set 6 : nits #
Total comments: 16
Patch Set 7 : addressing comments #
Total comments: 6
Patch Set 8 : nits #Messages
Total messages: 23 (0 generated)
PTAL. I am not sure whether we should put the files here. However, since UUIDs are supposed to be public, so we don't need to move it to private dirs.
On 2013/06/19 19:16:56, qinmin wrote: > PTAL. I am not sure whether we should put the files here. However, since UUIDs > are supposed to be public, so we don't need to move it to private dirs. Widevine UUID is public so no concern about that. How about making MediaDrmBridge::ctor take key_system instead of uuid, then adding the conversion function as a static function in media_drm_bridge.cc?
On 2013/06/19 20:04:23, xhwang wrote: > On 2013/06/19 19:16:56, qinmin wrote: > > PTAL. I am not sure whether we should put the files here. However, since UUIDs > > are supposed to be public, so we don't need to move it to private dirs. > > Widevine UUID is public so no concern about that. > > How about making MediaDrmBridge::ctor take key_system instead of uuid, then > adding the conversion function as a static function in media_drm_bridge.cc? Done.
https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:23: if (0 == key_system.compare("com.widevine")) Why not just if (key_system == "com.widevine") ? This could end up creating a string for "com.widevine" but I feel it's more intuitive. WDYT? https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:24: uuid.insert(uuid.end(), kWideVineUUID, kWideVineUUID + 16); uuid.assign(kWideVineUUID, kWideVineUUID + 16) ? We never need to append to an existing UUID. https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:43: std::vector<uint8> uuid = ConvertKeySystemToUUID(key_system); MediaDrmBridge should probably keep a reference to the uuid so that when we parse the pssh box, we can compare the SystemId with the current UUID and only call GRK with the Data associated with the current/selected |uuid_|.
also added AddMediaDrmBridge() call to WebMediaPlayerManagerImpl https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:23: if (0 == key_system.compare("com.widevine")) On 2013/06/19 21:30:41, xhwang wrote: > Why not just if (key_system == "com.widevine") ? This could end up creating a > string for "com.widevine" but I feel it's more intuitive. WDYT? Done. https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:24: uuid.insert(uuid.end(), kWideVineUUID, kWideVineUUID + 16); On 2013/06/19 21:30:41, xhwang wrote: > uuid.assign(kWideVineUUID, kWideVineUUID + 16) ? We never need to append to an > existing UUID. Done. https://codereview.chromium.org/17101027/diff/4001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:43: std::vector<uint8> uuid = ConvertKeySystemToUUID(key_system); added a uuid_ member variable. On 2013/06/19 21:30:41, xhwang wrote: > MediaDrmBridge should probably keep a reference to the uuid so that when we > parse the pssh box, we can compare the SystemId with the current UUID and only > call GRK with the Data associated with the current/selected |uuid_|.
lgtm https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... File content/browser/android/media_player_manager_impl.cc (right): https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... content/browser/android/media_player_manager_impl.cc:492: drm_bridges_.push_back(new MediaDrmBridge(media_keys_id, key_system)); Not sure how this is connected to InitializeCDM(key_system) call. But we can figure that out later.
https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... File content/browser/android/media_player_manager_impl.cc (right): https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... content/browser/android/media_player_manager_impl.cc:491: RemoveDrmBridge(media_keys_id); Is that really something we want to do? Should we just assert it's not already known? The actual MediaKeys object should not be reused. If |media_keys_id| is the same as the media player ID, then we might reuse it. (In the new EME API, they can't be the same as the two objects can live separately.) https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:13: // TODO(qinmin): figure out what is the UUID for com.widevine.alpha, and also See below for com.widevine.alpha. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:21: const std::string& key_system) { There are constants for the key systems in key_system*, although I don't think we actually expose them. It might be preferable to add a ConvertKeySystemToAndroidUUID method to that file. The one issue is that this code is part of the renderer at the moment. As with all functionality, it might be better to do as much of this as possible in the renderer. That would mean that the Android proxy and below would use a const vector& instead of const string& like the rest of the stack, but it might be worth it. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:23: if (key_system == "com.widevine") com.widevine.alpha com.widevine is a parent key system only used for canPlayType(). It is not actually a supported key system on its own. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:26: NOTIMPLEMENTED(); not necessarily - it's just not recognized. You could DLOG or something, but there's not necessarily anything to implement for ALL other strings. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:42: uuid_(ConvertKeySystemToUUID(key_system)) { With a Create() method, you can do the conversion and fail if the conversion fails (rather than creating an invalid object).
https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... File content/browser/android/media_player_manager_impl.cc (right): https://codereview.chromium.org/17101027/diff/9001/content/browser/android/me... content/browser/android/media_player_manager_impl.cc:491: RemoveDrmBridge(media_keys_id); That's reassuring. I was wondering what could happen if a MediaKeys can be reused. Changed this to a DCHECK(). On 2013/06/19 23:14:41, ddorwin wrote: > Is that really something we want to do? Should we just assert it's not already > known? The actual MediaKeys object should not be reused. If |media_keys_id| is > the same as the media player ID, then we might reuse it. (In the new EME API, > they can't be the same as the two objects can live separately.) https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:13: // TODO(qinmin): figure out what is the UUID for com.widevine.alpha, and also On 2013/06/19 23:14:41, ddorwin wrote: > See below for com.widevine.alpha. Done. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:21: const std::string& key_system) { Done. Changing the MediaDrmBridge ctor back to accept a vector param so renderer will send the uuid in an IPC. On 2013/06/19 23:14:41, ddorwin wrote: > There are constants for the key systems in key_system*, although I don't think > we actually expose them. It might be preferable to add a > ConvertKeySystemToAndroidUUID method to that file. > > The one issue is that this code is part of the renderer at the moment. As with > all functionality, it might be better to do as much of this as possible in the > renderer. That would mean that the Android proxy and below would use a const > vector& instead of const string& like the rest of the stack, but it might be > worth it. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:23: if (key_system == "com.widevine") On 2013/06/19 23:14:41, ddorwin wrote: > com.widevine.alpha > com.widevine is a parent key system only used for canPlayType(). It is not > actually a supported key system on its own. > Done. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:26: NOTIMPLEMENTED(); On 2013/06/19 23:14:41, ddorwin wrote: > not necessarily - it's just not recognized. You could DLOG or something, but > there's not necessarily anything to implement for ALL other strings. Done. https://codereview.chromium.org/17101027/diff/9001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:42: uuid_(ConvertKeySystemToUUID(key_system)) { On 2013/06/19 23:14:41, ddorwin wrote: > With a Create() method, you can do the conversion and fail if the conversion > fails (rather than creating an invalid object). Done.
Thanks. LG, just need to move some code. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:16: All the system specifics go in key_systems_info.h/.cc This file just wraps those calls. See KeySystemNameForUMAGeneric, for example. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:19: // TODO(qinmin): add UUIDs for other key systems. #ifdef all the new code to Android https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:20: const uint8 kWideVineUUID[] = { nit: media code uses static instead of unnamed namespaces. See below. Put this with the local method. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:173: std::vector<uint8> ConvertKeySystemToAndroidUUID( nit: Updating my previous suggesetion, GetAndroidUUIDForKeySystem might be better. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:176: if (key_system == "com.widevine.alpha") There is a constant for this in the _info file. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.h:43: // Convert |key_system| to 16 bytes android UUID. nit: ...16-byte Android... https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:757: // Currently only widevine is supported. You probably don't need this comment, and it could get out of date.
https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... File media/base/android/media_drm_bridge.h (right): https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... media/base/android/media_drm_bridge.h:41: // Drm related message was received. nit: s/Drm/DRM https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... media/base/android/media_drm_bridge.h:59: // Id of the MediaKeys object. nit: s/Id/ID https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:22: 0xA3, 0xC8, 0x27, 0xDC, 0xD5, 0x1D, 0x21, 0xED, nit: drop the last comma. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:173: std::vector<uint8> ConvertKeySystemToAndroidUUID( On 2013/06/20 03:07:27, ddorwin wrote: > nit: Updating my previous suggesetion, GetAndroidUUIDForKeySystem might be > better. I wonder whats' the extent of this UUID? Is this only used for PSSH as the System ID, or is it a general UUID for a given key_system that could possibly be used in other containers/streams? Why we limit it to Android? If it's general, how about just name it as GetUUID(key_system), which is consistent with GetPepperType() above?
https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:173: std::vector<uint8> ConvertKeySystemToAndroidUUID( On 2013/06/20 04:02:08, xhwang wrote: > On 2013/06/20 03:07:27, ddorwin wrote: > > nit: Updating my previous suggesetion, GetAndroidUUIDForKeySystem might be > > better. > > I wonder whats' the extent of this UUID? Is this only used for PSSH as the > System ID, or is it a general UUID for a given key_system that could possibly be > used in other containers/streams? Why we limit it to Android? If it's general, > how about just name it as GetUUID(key_system), which is consistent with > GetPepperType() above? Good point about GetPepperType(). I'm fine with GetUUID(). I still think it should be #ifdef'd,though, like GetPepperType(). As I understand it, UUID is used to select the DRM system in the Android API. The IDs *happen* to match the PS ID in CENC PSSHes. But, those are not used in the Chromium code for anything except on Android.
https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... File media/base/android/media_drm_bridge.h (right): https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... media/base/android/media_drm_bridge.h:41: // Drm related message was received. On 2013/06/20 04:02:08, xhwang wrote: > nit: s/Drm/DRM Done. https://codereview.chromium.org/17101027/diff/15001/media/base/android/media_... media/base/android/media_drm_bridge.h:59: // Id of the MediaKeys object. On 2013/06/20 04:02:08, xhwang wrote: > nit: s/Id/ID Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:16: On 2013/06/20 03:07:27, ddorwin wrote: > All the system specifics go in key_systems_info.h/.cc This file just wraps those > calls. See KeySystemNameForUMAGeneric, for example. Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:19: // TODO(qinmin): add UUIDs for other key systems. On 2013/06/20 03:07:27, ddorwin wrote: > #ifdef all the new code to Android Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:20: const uint8 kWideVineUUID[] = { On 2013/06/20 03:07:27, ddorwin wrote: > nit: media code uses static instead of unnamed namespaces. See below. Put this > with the local method. Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:22: 0xA3, 0xC8, 0x27, 0xDC, 0xD5, 0x1D, 0x21, 0xED, On 2013/06/20 04:02:08, xhwang wrote: > nit: drop the last comma. Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:173: std::vector<uint8> ConvertKeySystemToAndroidUUID( On 2013/06/20 04:12:44, ddorwin wrote: > On 2013/06/20 04:02:08, xhwang wrote: > > On 2013/06/20 03:07:27, ddorwin wrote: > > > nit: Updating my previous suggesetion, GetAndroidUUIDForKeySystem might be > > > better. > > > > I wonder whats' the extent of this UUID? Is this only used for PSSH as the > > System ID, or is it a general UUID for a given key_system that could possibly > be > > used in other containers/streams? Why we limit it to Android? If it's general, > > how about just name it as GetUUID(key_system), which is consistent with > > GetPepperType() above? > > Good point about GetPepperType(). I'm fine with GetUUID(). I still think it > should be #ifdef'd,though, like GetPepperType(). > > As I understand it, UUID is used to select the DRM system in the Android API. > The IDs *happen* to match the PS ID in CENC PSSHes. But, those are not used in > the Chromium code for anything except on Android. Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:176: if (key_system == "com.widevine.alpha") Seems that I need defined(WIDEVINE_CDM_AVAILABLE) in order to use kWidevineKeySystem. Not sure whether this is needed for android though, is that needed for all the builds that supports widevine? On 2013/06/20 03:07:27, ddorwin wrote: > There is a constant for this in the _info file. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.h:43: // Convert |key_system| to 16 bytes android UUID. On 2013/06/20 03:07:27, ddorwin wrote: > nit: ...16-byte Android... Done. https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:757: // Currently only widevine is supported. On 2013/06/20 03:07:27, ddorwin wrote: > You probably don't need this comment, and it could get out of date. Done.
https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.cc (right): https://codereview.chromium.org/17101027/diff/15001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.cc:176: if (key_system == "com.widevine.alpha") On 2013/06/20 17:41:53, qinmin wrote: > Seems that I need defined(WIDEVINE_CDM_AVAILABLE) in order to use > kWidevineKeySystem. Not sure whether this is needed for android though, is that > needed for all the builds that supports widevine? > > On 2013/06/20 03:07:27, ddorwin wrote: > > There is a constant for this in the _info file. > Yes. That currently does two things for non-Pepper CDM platforms: 1) Enable content_browsertests 2) Enable canPlayType() responses. But there are some caveats. Most importantly, we shouldn't do that until everything is working because it enables tests that will fail and lies about support to applications. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.h:44: // Convert |key_system| to 16-byte android UUID. nit: _A_ndroid (unless _a_ is common) https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.cc (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:103: // TODO(qinmin): add UUIDs for other key systems. You could (should?) also ifdef this with WIDEVINE_CDM_AVAILABLE. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:111: std::vector<uint8>(kWideVineUUID, kWideVineUUID + 16) } Static initialization of complex type is not allowed. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:115: arraysize(kKeySystemToUUIDMapping); fits https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.h (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.h:34: std::vector<uint8> uuid; Per the other comment, you'll need a different type. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:759: EXPECT_EQ(16u, uuid.size()); Do you care about the actual uuid? It seems we should maybe have at least one value checked. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:760: You could break out unsupported systmes into a separate _Unrecognized test. That has the same effect as the comment that was removed but in a better way. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:761: uuid = GetUUID(kWidevine); These could all be collapsed into a single line. For example: EXPECT_EQ(0u, GetUUID(kWidevine));
https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems.h (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems.h:44: // Convert |key_system| to 16-byte android UUID. On 2013/06/20 22:20:15, ddorwin wrote: > nit: _A_ndroid (unless _a_ is common) Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.cc (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:103: // TODO(qinmin): add UUIDs for other key systems. On 2013/06/20 22:20:15, ddorwin wrote: > You could (should?) also ifdef this with WIDEVINE_CDM_AVAILABLE. Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:111: std::vector<uint8>(kWideVineUUID, kWideVineUUID + 16) } On 2013/06/20 22:20:15, ddorwin wrote: > Static initialization of complex type is not allowed. Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:115: arraysize(kKeySystemToUUIDMapping); On 2013/06/20 22:20:15, ddorwin wrote: > fits Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.h (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.h:34: std::vector<uint8> uuid; On 2013/06/20 22:20:15, ddorwin wrote: > Per the other comment, you'll need a different type. Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:759: EXPECT_EQ(16u, uuid.size()); On 2013/06/20 22:20:15, ddorwin wrote: > Do you care about the actual uuid? It seems we should maybe have at least one > value checked. Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:760: On 2013/06/20 22:20:15, ddorwin wrote: > You could break out unsupported systmes into a separate _Unrecognized test. That > has the same effect as the comment that was removed but in a better way. Done. https://codereview.chromium.org/17101027/diff/31001/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:761: uuid = GetUUID(kWidevine); On 2013/06/20 22:20:15, ddorwin wrote: > These could all be collapsed into a single line. For example: > EXPECT_EQ(0u, GetUUID(kWidevine)); Changed to EXPECT_TRUE(GetUUID(kWidevine).empty())
lgtm Just nits. Thanks. https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.cc (right): https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:112: // arraySize() does not work if the array is empty, use ARRAYSIZE_UNSAFE() ... so use ARRAYSIZE_UNSAFE(). ^ might fit https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:757: TEST_F(KeySystemsTest, ConvertKeySystemToUUID) { GetUUID_Widevine https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:767: TEST_F(KeySystemsTest, ConvertUnrecognizedKeySystemToUUID) { It's nice to leave the function name in the test name. Thus, this pattern: GetUUID_Unrecognized.
https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_info.cc (right): https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_info.cc:112: // arraySize() does not work if the array is empty, use ARRAYSIZE_UNSAFE() On 2013/06/20 23:50:12, ddorwin wrote: > ... so use ARRAYSIZE_UNSAFE(). > ^ might fit Done. https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... File webkit/renderer/media/crypto/key_systems_unittest.cc (right): https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:757: TEST_F(KeySystemsTest, ConvertKeySystemToUUID) { On 2013/06/20 23:50:12, ddorwin wrote: > GetUUID_Widevine Done. https://codereview.chromium.org/17101027/diff/34002/webkit/renderer/media/cry... webkit/renderer/media/crypto/key_systems_unittest.cc:767: TEST_F(KeySystemsTest, ConvertUnrecognizedKeySystemToUUID) { On 2013/06/20 23:50:12, ddorwin wrote: > It's nice to leave the function name in the test name. Thus, this pattern: > GetUUID_Unrecognized. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/17101027/38010
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+yfriedman for OWNER of /content/browser/android
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/17101027/38010
Message was sent while issue was closed.
Change committed as 207815 |
