Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1001723002:
media: Refactor PlatformVerificationFlow. (Closed)
|
Created:
5 years, 9 months ago by xhwang Modified:
5 years, 9 months ago CC:
chromium-reviews, eme-reviews_chromium.org, posciak+watch_chromium.org, stevenjb+watch_chromium.org, mcasas+watch_chromium.org, dkrahn+watch_chromium.org, feature-media-reviews_chromium.org, oshima+watch_chromium.org, wjia+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Refactor PlatformVerificationFlow.
Highlight of this CL:
- PlatformVerificationDialog and PlatformVerificationFlow are independant of
each other now.
- PlatformVerificationDialog is only triggered from
ProtectedMediaIdentifierPermissionContext::RequestPermission().
- ProxyDecryptor requests permission in certain conditions.
- PlatformVerificationFlow always checks permission before doing RA.
- ProtectedMediaIdentifierPermissionContext is the central place checking all
related settings and conditions, and handling the UI for ChromeOS.
This CL doesn't change the behavior on Android yet, which will be done in the
next CL.
BUG=455271, 455262, 446263
TBR=bauerb@chromium.org
TEST=Tested local test player and existing content providers's site/app.
Committed: https://crrev.com/c84739eb25ab0d91e55ee4bfbe0bdcc91c014467
Cr-Commit-Position: refs/heads/master@{#320628}
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix unittest #
Total comments: 36
Patch Set 3 : comments addressed #
Total comments: 32
Patch Set 4 : comments addressed #
Total comments: 6
Patch Set 5 : comments addressed #Patch Set 6 : comments addressed #
Total comments: 2
Patch Set 7 : comments addressed #Messages
Total messages: 27 (4 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, dkrahn@chromium.org
PTAL
Also, I need to fix the PlatformVerificationFlowTest. But please review the implementation change first. Thanks! https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (left): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:244: !found || !pref_service->GetBoolean(prefs::kRAConsentGranted); This logic is moved to ProtectedMediaIdentifierPermissionContext. https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:482: void PlatformVerificationFlow::RecordOriginConsent( These are now handled by ProtectedMediaIdentifierPermissionContext. https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:206: base::Bind(&PlatformVerificationFlow::OnAttestationPrepared, I skipped the CheckEnrollment step because we don't care about the enrollment status.
https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.h:31: }; We should probably just replace this with PermissionStatus: https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont... https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:184: // Checking them here explicitly to report the correct error type. The results are reported to UMA, that's why I have to make the result error type correct :(
Some comments. Switching to PS2. https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:107: // hasn't been made yet. Or the 'X' is clicked or "esc" are pressed? https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:196: if (!IsPermittedByUser(web_contents)) { Perhaps note that the user should have already been prompted if necessary. https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:198: ReportError(callback, USER_REJECTED); OOC, where do these values go and why? Do we use them for UMAs? They shouldn't be exposed to the app, or even perhaps the renderer. https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.cc:359: // TODO(xhwang): Using delegate_->GetURL() here is not right. The platform What URL does this give us? GLCURL below gets us the top-level origin. Your comment is saying delegate_ is not the requesting frame, right? https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/chromeos/att... chrome/browser/chromeos/attestation/platform_verification_flow.h:170: // Checks whether the device has already been enrolled for attestation. The Is this comment still accurate? https://codereview.chromium.org/1001723002/diff/1/chrome/browser/media/protec... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/1001723002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.h:57: // Returns whether "Protected content" is enabled considering factors other nit: comma after enabled. Or, perhaps s/considering/based on/ https://codereview.chromium.org/1001723002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.h:59: // it can be disabled by a master switch in content settings, in incognito or tiny nit: Capital I and G
LG overall. Thanks. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.h:31: }; On 2015/03/12 00:25:00, xhwang wrote: > We should probably just replace this with PermissionStatus: > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont... "ASK" might be a bit weird, but whatever you think is best. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:97: // TODO(xhwang): Using delegate_->GetURL() here is not right. The platform What URL does this give us? GLCURL below gets us the top-level origin. Your comment is saying delegate_ is not the requesting frame, right? https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:197: ReportError(callback, PLATFORM_NOT_VERIFIED); Should we DCHECK here? Unlike user settings, it's not possible for these to change so the permission context code should prevent us from getting here, right? https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:59: // These values are reported to UMA. DO NOT CHANGE THE EXISTING VALUES! Why isn't there a LAST value like other UMA enums? https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:90: if (!consent_required) { nit: All the negative logic is a bit confusing. Perhaps invert it to full_consent_given or something. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:106: // Instead, we check the content setting and show the existing platform ... setting (above) and ... ? https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:138: return CONTENT_SETTING_BLOCK; Why do we block in this case rather than return NONE? Also, we seem to handle this in two locations - here and line 88. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:195: #if defined(OS_CHROMEOS) Should this be #elif? https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:203: // This could be disabled by user's master switch or by the device policy. It appears we check in the opposite order. If so, please update. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:257: base::UserMetricsAction("PlatformVerificationAccepted")); Should we record None too? Would be interesting to know how many hit 'X'/"esc". https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:110: uint8* init_data_vector_data = Move to 114 https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:114: media_keys_->LoadSession( In theory, a user could have allowed then denied and an app is now attempting to load. Chrome OS would block any requests. Android may not, but we don't support load there, so this is probably fine (and it's slated for removal). https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:136: media_permission_->RequestPermission( External Clear Key will now prompt. I guess that's fine. https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:143: // that don't use AesDecryptor. Bug? Deadline?
https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.h:31: }; On 2015/03/12 03:58:11, ddorwin wrote: > On 2015/03/12 00:25:00, xhwang wrote: > > We should probably just replace this with PermissionStatus: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/cont... > > "ASK" might be a bit weird, but whatever you think is best. Agree that "ASK" sounds weird, but it's the standard term used throughout the permission code. This CL is already pretty big, so I'll leave this cleanup to a later CL. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:44: const int kAttestationResultHistogramMax = 10; This is used for UMA reporting instead of a "LAST" or "MAX" enum. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:97: // TODO(xhwang): Using delegate_->GetURL() here is not right. The platform On 2015/03/12 03:58:11, ddorwin wrote: > What URL does this give us? GLCURL below gets us the top-level origin. Your > comment is saying delegate_ is not the requesting frame, right? It's up to the delegate's implementation. For non-test code, it's defined in l.79, which is either GLCURL, or GetVisibalURL. This is error prone, but somehow works for existing content providers. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:197: ReportError(callback, PLATFORM_NOT_VERIFIED); On 2015/03/12 03:58:11, ddorwin wrote: > Should we DCHECK here? Unlike user settings, it's not possible for these to > change so the permission context code should prevent us from getting here, > right? Well, for prefixed EME, we are not stopping the player to call GKR() anywhere in the stack. So we can end up here even in Guest mode (after permission request failed). https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:59: // These values are reported to UMA. DO NOT CHANGE THE EXISTING VALUES! On 2015/03/12 03:58:11, ddorwin wrote: > Why isn't there a LAST value like other UMA enums? See the .cc file. line 44. const int kAttestationResultHistogramMax = 10; kAttestationResultHistogramMax is used for UMA reporting instead of a LAST or MAX enum. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:90: if (!consent_required) { On 2015/03/12 03:58:12, ddorwin wrote: > nit: All the negative logic is a bit confusing. Perhaps invert it to > full_consent_given or something. Done. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:106: // Instead, we check the content setting and show the existing platform On 2015/03/12 03:58:12, ddorwin wrote: > ... setting (above) and ... ? Done. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:138: return CONTENT_SETTING_BLOCK; On 2015/03/12 03:58:12, ddorwin wrote: > Why do we block in this case rather than return NONE? Good point. Changed to ASK. > Also, we seem to handle this in two locations - here and line 88. Yes, this is the tricky part which is explained in the comment above. For RequestPermission, we'll show the UI when kRAConsentGranted is false. For GetPermissionStatus, we'll return ASK. Since the behavior is different, it's hard to put this check in one single place. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:195: #if defined(OS_CHROMEOS) On 2015/03/12 03:58:12, ddorwin wrote: > Should this be #elif? Done. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:203: // This could be disabled by user's master switch or by the device policy. On 2015/03/12 03:58:12, ddorwin wrote: > It appears we check in the opposite order. If so, please update. Done. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:257: base::UserMetricsAction("PlatformVerificationAccepted")); On 2015/03/12 03:58:12, ddorwin wrote: > Should we record None too? Would be interesting to know how many hit 'X'/"esc". This and PlatformVerificationRejected are actually in Counts, not in Histograms. I also feel a Histogram (with a NONE entry) would be more useful. +Darren. https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:110: uint8* init_data_vector_data = On 2015/03/12 03:58:12, ddorwin wrote: > Move to 114 Done. https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:114: media_keys_->LoadSession( On 2015/03/12 03:58:12, ddorwin wrote: > In theory, a user could have allowed then denied and an app is now attempting to > load. Chrome OS would block any requests. Android may not, but we don't support > load there, so this is probably fine (and it's slated for removal). Acknowledged. https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:136: media_permission_->RequestPermission( On 2015/03/12 03:58:12, ddorwin wrote: > External Clear Key will now prompt. I guess that's fine. Actually External Clear Key browser tests fail now :) Fixed this... https://codereview.chromium.org/1001723002/diff/20001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:143: // that don't use AesDecryptor. On 2015/03/12 03:58:12, ddorwin wrote: > Bug? Deadline? I'll do it right after this CL. Just don't want to make this CL even bigger. Added deadline.
https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:97: // TODO(xhwang): Using delegate_->GetURL() here is not right. The platform On 2015/03/12 17:35:26, xhwang wrote: > On 2015/03/12 03:58:11, ddorwin wrote: > > What URL does this give us? GLCURL below gets us the top-level origin. Your > > comment is saying delegate_ is not the requesting frame, right? > > It's up to the delegate's implementation. For non-test code, it's defined in > l.79, which is either GLCURL, or GetVisibalURL. This is error prone, but somehow > works for existing content providers. Existing CPs probably aren't iframed, except maybe Play. Definitely a bug. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:197: ReportError(callback, PLATFORM_NOT_VERIFIED); On 2015/03/12 17:35:26, xhwang wrote: > On 2015/03/12 03:58:11, ddorwin wrote: > > Should we DCHECK here? Unlike user settings, it's not possible for these to > > change so the permission context code should prevent us from getting here, > > right? > > Well, for prefixed EME, we are not stopping the player to call GKR() anywhere in > the stack. So we can end up here even in Guest mode (after permission request > failed). Oh, so TODO to DCHECK after removing prefixed (249976)? https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:106: // This method is called when the tab is closed and in that case the decision What function handles the 'X' and "esc"? https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:44: const int kAttestationResultHistogramMax = 10; On 2015/03/12 17:35:26, xhwang wrote: > This is used for UMA reporting instead of a "LAST" or "MAX" enum. Is this just a made up number believed to be big enough? That seems wrong. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:157: // Checks whether the device has already been enrolled for attestation. The Is this comment still accurate? It seems not. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:87: profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted); I don't think we need this anymore because this will always cause ASK. You can DCHECK it at line 90 if you want I guess. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:131: // Block if user never granted RA consent on this device. It's possible that "Block" is no longer correct? https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:133: // setting is set to "allow" by server sync. In this case, we should ask. It's not clear where all this is called. (I think it could be used anywhere since it's accessible via at least two interfaces.) I guess this is okay, as long as the platform verification code, etc. consider ASK to be BLOCK. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:246: case PlatformVerificationDialog::CONSENT_RESPONSE_NONE: Let's please bottom out on the histogram decision and file a bug if necessary. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.h:65: // Returns whether "Protected content" is enabled considering factors other s/considering/based on/ ? https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:130: if (CanUseAesDecryptor(key_system_) || IsExternalClearKey(key_system_)) { On 2015/03/12 17:35:27, xhwang wrote: > On 2015/03/12 03:58:12, ddorwin wrote: > > External Clear Key will now prompt. I guess that's fine. > > Actually External Clear Key browser tests fail now :) Fixed this... Do Widevine browser tests fail too (on the linux_chromeos bots)?
https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (left): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:490: ContentSettingsPattern::CreateBuilder(false)); Is the logic from this builder still present in the permissions logic? I didn't see it. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:100: // be solved when http://crbug.com/454847 is fixed. nit: extra line // will be solved... https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (left): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:435: } Are the tests removed from here popping up elsewhere? https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:158: DVLOG_IF(1, !granted) << "Permission request rejected."; Maybe I'm not following -- is this all we do when the user rejects? It looks like |granted| is not checked anywhere.
comments addressed
PTAL again. Thanks! https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:106: // This method is called when the tab is closed and in that case the decision On 2015/03/12 21:38:53, ddorwin wrote: > What function handles the 'X' and "esc"? This is the function for 'x' and 'esc'. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (left): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:490: ContentSettingsPattern::CreateBuilder(false)); On 2015/03/12 22:12:17, Darren Krahn wrote: > Is the logic from this builder still present in the permissions logic? I didn't > see it. The new permission and content settings are completely based on the requesting_origin (origin of the frame making the permission request) and embedding origin (origin of the top level frame). The origin will contain the scheme, domain and host. We shouldn't need to build our own pattern. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:44: const int kAttestationResultHistogramMax = 10; On 2015/03/12 21:38:53, ddorwin wrote: > On 2015/03/12 17:35:26, xhwang wrote: > > This is used for UMA reporting instead of a "LAST" or "MAX" enum. > > Is this just a made up number believed to be big enough? That seems wrong. Agreed. But feel it should be in another CL. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:100: // be solved when http://crbug.com/454847 is fixed. On 2015/03/12 22:12:17, Darren Krahn wrote: > nit: extra line > > // will be solved... Done. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:157: // Checks whether the device has already been enrolled for attestation. The On 2015/03/12 21:38:53, ddorwin wrote: > Is this comment still accurate? It seems not. Done. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (left): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:435: } On 2015/03/12 22:12:17, Darren Krahn wrote: > Are the tests removed from here popping up elsewhere? No. The related logic are moved to ProtectedMediaIdentifierPermissionContext and should be tested there. Unfortunately, there are not testing infrastructure for ProtectedMediaIdentifierPermissionContext yet :( https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:87: profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted); On 2015/03/12 21:38:54, ddorwin wrote: > I don't think we need this anymore because this will always cause ASK. You can > DCHECK it at line 90 if you want I guess. I am not sure I follow your comment. Can you elaborate? https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:131: // Block if user never granted RA consent on this device. It's possible that On 2015/03/12 21:38:53, ddorwin wrote: > "Block" is no longer correct? Done. Also I restructured this function a bit to make it more clear and more consistent with RequestPermission(). https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:133: // setting is set to "allow" by server sync. In this case, we should ask. On 2015/03/12 21:38:53, ddorwin wrote: > It's not clear where all this is called. (I think it could be used anywhere > since it's accessible via at least two interfaces.) I guess this is okay, as > long as the platform verification code, etc. consider ASK to be BLOCK. Acknowledged. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:246: case PlatformVerificationDialog::CONSENT_RESPONSE_NONE: On 2015/03/12 21:38:54, ddorwin wrote: > Let's please bottom out on the histogram decision and file a bug if necessary. +Darren. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.h:65: // Returns whether "Protected content" is enabled considering factors other On 2015/03/12 21:38:54, ddorwin wrote: > s/considering/based on/ ? Done. https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:130: if (CanUseAesDecryptor(key_system_) || IsExternalClearKey(key_system_)) { On 2015/03/12 21:38:54, ddorwin wrote: > On 2015/03/12 17:35:27, xhwang wrote: > > On 2015/03/12 03:58:12, ddorwin wrote: > > > External Clear Key will now prompt. I guess that's fine. > > > > Actually External Clear Key browser tests fail now :) Fixed this... > > Do Widevine browser tests fail too (on the linux_chromeos bots)? No. Only ECK failed. https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:158: DVLOG_IF(1, !granted) << "Permission request rejected."; On 2015/03/12 22:12:17, Darren Krahn wrote: > Maybe I'm not following -- is this all we do when the user rejects? It looks > like |granted| is not checked anywhere. ProxyDecryptor is only used by Prefixed EME, where RequestPermission() is mainly for triggering the permission UI. Later when we call CheckPermission() (e.g. in PlatformVerificationFlow), we'll fail there. In unprefixed EME, we'll also call RequestPermission() [1], and the result is not ignored [2]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... [2] https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... You can think it this way: - Unprefixed EME needs to call RequestPermission early and show the UI. - Since we don't want to show the UI twice to the user, we should NOT trigger UI in PlatformVerificationFlow, hence CheckPermission() instead of RequestPermission there. - But we still want to show UI to user at least once for prefixed EME, so call RequestPermission() in ProxyDecryptor.
A couple comments in the previous PS. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:106: // This method is called when the tab is closed and in that case the decision On 2015/03/13 00:54:42, xhwang wrote: > On 2015/03/12 21:38:53, ddorwin wrote: > > What function handles the 'X' and "esc"? > > This is the function for 'x' and 'esc'. The comment says "tab". Should that say "dialog" or something like that? https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:87: profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted); On 2015/03/13 00:54:42, xhwang wrote: > On 2015/03/12 21:38:54, ddorwin wrote: > > I don't think we need this anymore because this will always cause ASK. You can > > DCHECK it at line 90 if you want I guess. > > I am not sure I follow your comment. Can you elaborate? I was saying that we don't need the RHS of the && because GetPermissionStatus() would never return ALLOW without kRAConsentGranted being set. However, I now see that we're calling the base class's GetPermissionStatus(), which wouldn't have that check. Is there a reason we call the base class? https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: // are in |context| and |attestation_prepared| specifies whether attestation nit: comma before "and" (this case of the two parameters together is especially confusing). https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:75: ContentSetting content_setting = PermissionContextBase::GetPermissionStatus( If we called this class's method, we could eliminate line 68 and line 86. We would need to move more of this code out of the #ifdef, which might be redundant with line 113, but it would have the benefit of keeping most of the cros-specific stuff in one place. OTOH, checking and setting kRAConsentGranted in the same function is nice. https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:129: if (content_setting == CONTENT_SETTING_BLOCK) This only applies to OS_CHROMEOS too. It's probably not even necessary there since we explicitly check for ALLOW at 133.
comments addressed
comments addressed
Sorry I missed some previous comments. Now I have responses in 3 different PS. PTAL again. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:97: // TODO(xhwang): Using delegate_->GetURL() here is not right. The platform On 2015/03/12 21:38:53, ddorwin wrote: > On 2015/03/12 17:35:26, xhwang wrote: > > On 2015/03/12 03:58:11, ddorwin wrote: > > > What URL does this give us? GLCURL below gets us the top-level origin. Your > > > comment is saying delegate_ is not the requesting frame, right? > > > > It's up to the delegate's implementation. For non-test code, it's defined in > > l.79, which is either GLCURL, or GetVisibalURL. This is error prone, but > somehow > > works for existing content providers. > > Existing CPs probably aren't iframed, except maybe Play. Definitely a bug. Acknowledged. https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.cc:197: ReportError(callback, PLATFORM_NOT_VERIFIED); On 2015/03/12 21:38:53, ddorwin wrote: > On 2015/03/12 17:35:26, xhwang wrote: > > On 2015/03/12 03:58:11, ddorwin wrote: > > > Should we DCHECK here? Unlike user settings, it's not possible for these to > > > change so the permission context code should prevent us from getting here, > > > right? > > > > Well, for prefixed EME, we are not stopping the player to call GKR() anywhere > in > > the stack. So we can end up here even in Guest mode (after permission request > > failed). > > Oh, so TODO to DCHECK after removing prefixed (249976)? Done. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:106: // This method is called when the tab is closed and in that case the decision On 2015/03/13 17:03:09, ddorwin wrote: > On 2015/03/13 00:54:42, xhwang wrote: > > On 2015/03/12 21:38:53, ddorwin wrote: > > > What function handles the 'X' and "esc"? > > > > This is the function for 'x' and 'esc'. > > The comment says "tab". Should that say "dialog" or something like that? Updated comments. https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:87: profile()->GetPrefs()->GetBoolean(prefs::kRAConsentGranted); On 2015/03/13 17:03:10, ddorwin wrote: > On 2015/03/13 00:54:42, xhwang wrote: > > On 2015/03/12 21:38:54, ddorwin wrote: > > > I don't think we need this anymore because this will always cause ASK. You > can > > > DCHECK it at line 90 if you want I guess. > > > > I am not sure I follow your comment. Can you elaborate? > > I was saying that we don't need the RHS of the && because GetPermissionStatus() > would never return ALLOW without kRAConsentGranted being set. However, I now see > that we're calling the base class's GetPermissionStatus(), which wouldn't have > that check. > > Is there a reason we call the base class? Good point. Done. https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/attestation/platform_verification_flow.h (right): https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/attestation/platform_verification_flow.h:158: // are in |context| and |attestation_prepared| specifies whether attestation On 2015/03/13 17:03:10, ddorwin wrote: > nit: comma before "and" (this case of the two parameters together is especially > confusing). Done. https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:75: ContentSetting content_setting = PermissionContextBase::GetPermissionStatus( On 2015/03/13 17:03:10, ddorwin wrote: > If we called this class's method, we could eliminate line 68 and line 86. We > would need to move more of this code out of the #ifdef, which might be redundant > with line 113, but it would have the benefit of keeping most of the > cros-specific stuff in one place. OTOH, checking and setting kRAConsentGranted > in the same function is nice. Done. https://codereview.chromium.org/1001723002/diff/60001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:129: if (content_setting == CONTENT_SETTING_BLOCK) On 2015/03/13 17:03:10, ddorwin wrote: > This only applies to OS_CHROMEOS too. > It's probably not even necessary there since we explicitly check for ALLOW at > 133. Done.
LGTM. Thanks! https://codereview.chromium.org/1001723002/diff/100001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/100001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:84: callback.Run(CONTENT_SETTING_ASK); What happens when we return ASK? Does it block, retry, or fail?
https://codereview.chromium.org/1001723002/diff/100001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/100001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:84: callback.Run(CONTENT_SETTING_ASK); On 2015/03/13 22:11:52, ddorwin wrote: > What happens when we return ASK? Does it block, retry, or fail? I talked to bauberb@. CONTENT_SETTING_DEFAULT means the setting doesn't exist and should only be used for some special settings. For us, we'll only have ASK/ALLOW/BLOCK, which matches the PermissionStatus. ASK will be returned back to the render process, and converted to a "false" (deny) in MediaPermissionDispatcher: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
LGTM https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/1001723002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:257: base::UserMetricsAction("PlatformVerificationAccepted")); On 2015/03/12 17:35:26, xhwang wrote: > On 2015/03/12 03:58:12, ddorwin wrote: > > Should we record None too? Would be interesting to know how many hit > 'X'/"esc". > > This and PlatformVerificationRejected are actually in Counts, not in Histograms. > I also feel a Histogram (with a NONE entry) would be more useful. +Darren. I don't care either way. IIUC, user actions are typically measured with 'action' metrics -- we could just add a 'cancel' action. A histogram works too, though. https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:158: DVLOG_IF(1, !granted) << "Permission request rejected."; On 2015/03/13 00:54:42, xhwang wrote: > On 2015/03/12 22:12:17, Darren Krahn wrote: > > Maybe I'm not following -- is this all we do when the user rejects? It looks > > like |granted| is not checked anywhere. > > ProxyDecryptor is only used by Prefixed EME, where RequestPermission() is mainly > for triggering the permission UI. Later when we call CheckPermission() (e.g. in > PlatformVerificationFlow), we'll fail there. > > In unprefixed EME, we'll also call RequestPermission() [1], and the result is > not ignored [2]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... > > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... > > You can think it this way: > - Unprefixed EME needs to call RequestPermission early and show the UI. > - Since we don't want to show the UI twice to the user, we should NOT trigger UI > in PlatformVerificationFlow, hence CheckPermission() instead of > RequestPermission there. > - But we still want to show UI to user at least once for prefixed EME, so call > RequestPermission() in ProxyDecryptor. Understood. Can you please put a comment to clarify this in the code? Likely others will have a similar question.
comments addressed
https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... File media/cdm/proxy_decryptor.cc (right): https://codereview.chromium.org/1001723002/diff/40001/media/cdm/proxy_decrypt... media/cdm/proxy_decryptor.cc:158: DVLOG_IF(1, !granted) << "Permission request rejected."; On 2015/03/13 22:47:11, Darren Krahn wrote: > On 2015/03/13 00:54:42, xhwang wrote: > > On 2015/03/12 22:12:17, Darren Krahn wrote: > > > Maybe I'm not following -- is this all we do when the user rejects? It looks > > > like |granted| is not checked anywhere. > > > > ProxyDecryptor is only used by Prefixed EME, where RequestPermission() is > mainly > > for triggering the permission UI. Later when we call CheckPermission() (e.g. > in > > PlatformVerificationFlow), we'll fail there. > > > > In unprefixed EME, we'll also call RequestPermission() [1], and the result is > > not ignored [2]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... > > > > [2] > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webenc... > > > > You can think it this way: > > - Unprefixed EME needs to call RequestPermission early and show the UI. > > - Since we don't want to show the UI twice to the user, we should NOT trigger > UI > > in PlatformVerificationFlow, hence CheckPermission() instead of > > RequestPermission there. > > - But we still want to show UI to user at least once for prefixed EME, so call > > RequestPermission() in ProxyDecryptor. > > Understood. Can you please put a comment to clarify this in the code? Likely > others will have a similar question. Done.
xhwang@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@: TBRing you on trivial changes in chrome/browser/prefs/browser_prefs.cc.
The CQ bit was checked by xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org, dkrahn@chromium.org Link to the patchset: https://codereview.chromium.org/1001723002/#ps120001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1001723002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c84739eb25ab0d91e55ee4bfbe0bdcc91c014467 Cr-Commit-Position: refs/heads/master@{#320628} |
