Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
881983003:
media: Invoke PlatformVerificationDialog from ProtectedMediaIdentifierPermissionContext. (Closed)
|
Created:
5 years, 10 months ago by xhwang Modified:
5 years, 10 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, dkrahn+watch_chromium.org, feature-media-reviews_chromium.org, oshima+watch_chromium.org, markusheintz_, stevenjb+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: Invoke PlatformVerificationDialog from ProtectedMediaIdentifierPermissionContext.
This is a follow-up CL of r314351.
Currently this is invoked from PermissionContextBase. Since this is for
protected media identifier only, it's better to put it in
ProtectedMediaIdentifierPermissionContext.
Also included in this fix:
- Pass |requesting_origin| to PlatformVerificationDialog::ShowDialog().
Currently we are using the origin of the GetLastCommittedURL(), which is
wrong.
- Add ProtectedMediaIdentifierPermissionContext::CancelPermissionRequest() so
that we can cancel the prompt and drop the callback properly. Otherwise, this
will cause a DCHECK in PermissionServiceImp when closing the browser while the
prompt is shown.
BUG=446263
TEST=Manually tested various cases.
Committed: https://crrev.com/0d9194365f50890b7ac586c6e8ce3863c97c4027
Cr-Commit-Position: refs/heads/master@{#314924}
Patch Set 1 #
Total comments: 49
Patch Set 2 : comments addressed #
Total comments: 8
Patch Set 3 : comments addressed #
Total comments: 5
Patch Set 4 : comments addressed #Patch Set 5 : added todo + bug #Messages
Total messages: 22 (5 generated)
xhwang@chromium.org changed reviewers: + bauerb@chromium.org, ddorwin@chromium.org, mukai@chromium.org
xhwang@chromium.org changed required reviewers: + ddorwin@chromium.org
xhwang@chromium.org changed reviewers: + dkrahn@chromium.org
ddorwin: Please review everything. Darren Krahn: Please OWNERS review chrome/browser/chromeos/attestation/*; it's mainly UI code change. Jun Mukai: Please review how I cancel the Dialog. Bernhard Bauer: I am removing code from chrome/browser/content_settings/* to keep the hack in our code. Please OWNERS review.
the dialog itself seems fine. a few nitpicks. https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/permission_context_base.cc:29: nit: remove blank line https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:19: #include "ui/views/widget/widget.h" blank line between #include and using. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:37: pending_id_(GetInvalidPendingId()), It seems that pending_id_ and widget_ have really similar role. I think you can remove pending_id_ and use (widget_ == nullptr) for the validation. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:44: ~ProtectedMediaIdentifierPermissionContext() { Should invoke CancelPermissionRequest? Otherwise, if this is deleted before the widget is closed, the callback won't be called at all, which may confuse PlatformVerificationFlow (or no problem?) https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:123: if (!widget_ || !pending_id_.Equals(id)) Do we have to care about pending_id_ here? https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:130: widget_->Close(); then, widget_ = nullptr; https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.h:76: // destroyed first, which will invalidate weak pointers I believe the callback won't run during the destructor, so strictly speaking I guess this comment is not true. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.h:77: base::WeakPtrFactory<ProtectedMediaIdentifierPermissionContext> weak_factory_; weak_factory_ is also used chromeos only, so might be a good idea to put into ifdef OS_CHROMEOS
https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:46: GURL url = web_contents->GetLastCommittedURL(); navigation_url, top_level_url, or something like that? https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:51: std::string origin = extension ? extension->name() : requesting_origin.spec(); should we confirm that the requesting_origin is the same as the url's origin? Otherwise, we might do the wrong thing. I wonder what the desired behavior is... This could happen if an app is hosting an iframe on a server. Maybe this is handled in other ways, though. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.h:31: // valid before |callback| is returned. s/returned/called/? https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow.cc:243: context.web_contents->GetLastCommittedURL().GetOrigin(), This is the old path and will be removed when prefixed is fixed, right? https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:82: callback.Run(response_); Check requesting_origin.isvalid() or something like that? https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/permission_context_base.cc:29: ? https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:67: // Instead, we check the content setting and show existing platform nit: show the... https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:75: callback.Run(false); I'm a little concerned about the differences between this and the base. I don't have any better ideas, though. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:84: // We only support one prompt and one pending permission request. Reference 447005? https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:98: return; #else to avoid unreachable code https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:123: if (!widget_ || !pending_id_.Equals(id)) Should we DCHECK(pending_id_.Equals(id))? This would be a code bug, right? https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:132: #endif Ditto on #else. Or just ifdef the existence of this override. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:190: if (!pending_id_.Equals(id)) ditto on DCHECK https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:197: NotifyPermissionSet(id, requesting_origin, embedding_origin, callback, Like the base code, this appears to store by requesting and top-level origin, but that is not how things appear to be presented in chrome://settings/contentExceptions#popups. Are these actually stored by combination?
Thanks for the review! https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:46: GURL url = web_contents->GetLastCommittedURL(); On 2015/02/05 02:23:23, ddorwin wrote: > navigation_url, top_level_url, or something like that? Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:51: std::string origin = extension ? extension->name() : requesting_origin.spec(); On 2015/02/05 02:23:23, ddorwin wrote: > should we confirm that the requesting_origin is the same as the url's origin? > Otherwise, we might do the wrong thing. I wonder what the desired behavior is... > > This could happen if an app is hosting an iframe on a server. Maybe this is > handled in other ways, though. |requesting_origin| is the root of trust here, it's the real one requesting this permission. We have url here only to get the extension/app name. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.h:31: // valid before |callback| is returned. On 2015/02/05 02:23:23, ddorwin wrote: > s/returned/called/? Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow.cc:243: context.web_contents->GetLastCommittedURL().GetOrigin(), On 2015/02/05 02:23:24, ddorwin wrote: > This is the old path and will be removed when prefixed is fixed, right? Yes, this is not secure. But it'll be removed soon. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:82: callback.Run(response_); On 2015/02/05 02:23:24, ddorwin wrote: > Check requesting_origin.isvalid() or something like that? This is test code and requesting_origin is given.... I guess no need to check? https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/permission_context_base.cc:29: On 2015/02/05 02:08:14, Jun Mukai wrote: > nit: remove blank line Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/content_setti... chrome/browser/content_settings/permission_context_base.cc:29: On 2015/02/05 02:23:24, ddorwin wrote: > ? Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:19: #include "ui/views/widget/widget.h" On 2015/02/05 02:08:14, Jun Mukai wrote: > blank line between #include and using. Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:37: pending_id_(GetInvalidPendingId()), On 2015/02/05 02:08:14, Jun Mukai wrote: > It seems that pending_id_ and widget_ have really similar role. I think you can > remove pending_id_ and use (widget_ == nullptr) for the validation. There are two reasons I like pending_id_: 1, We don't want to cancel the wrong request. Even if the second request is rejected, but if the caller didn't get the response immediately (e.g. a callback is posted), it could happen. 2, We could have an reentrancy issue here. See below. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:44: ~ProtectedMediaIdentifierPermissionContext() { On 2015/02/05 02:08:14, Jun Mukai wrote: > Should invoke CancelPermissionRequest? Otherwise, if this is deleted before the > widget is closed, the callback won't be called at all, which may confuse > PlatformVerificationFlow (or no problem?) It seems no other *PermissionContext is doing this and we always call CalcelPermissionRequest() during teardown. Even in the case where we distroy |this| without calling CancelPermissionRequest(), the widget will close itself which will fire the callback, since our callback is bounded with a weak pointer I think we are still safe. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:67: // Instead, we check the content setting and show existing platform On 2015/02/05 02:23:24, ddorwin wrote: > nit: show the... Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:75: callback.Run(false); On 2015/02/05 02:23:24, ddorwin wrote: > I'm a little concerned about the differences between this and the base. I don't > have any better ideas, though. Thanks for checking that. Updated to match base. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:84: // We only support one prompt and one pending permission request. On 2015/02/05 02:23:24, ddorwin wrote: > Reference 447005? Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:98: return; On 2015/02/05 02:23:24, ddorwin wrote: > #else to avoid unreachable code Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:123: if (!widget_ || !pending_id_.Equals(id)) On 2015/02/05 02:08:14, Jun Mukai wrote: > Do we have to care about pending_id_ here? See above. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:123: if (!widget_ || !pending_id_.Equals(id)) On 2015/02/05 02:23:24, ddorwin wrote: > Should we DCHECK(pending_id_.Equals(id))? This would be a code bug, right? IN case there are multiple requests, this can be tricky. I didn't fully inspected the full stack to make sure this should never happen. We will remove all of these when we have the UI fixed, so I just want to make this code robust for now.... WDYT? https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:130: widget_->Close(); On 2015/02/05 02:08:14, Jun Mukai wrote: > then, widget_ = nullptr; widget->Close() could cause OnPlatformVerificationResult to be called, which will reenter this class and check |widget_|. So widget_ = nullptr will be too late here. I hate firing callbacks synchronously but this seems to be the style we have here. #1 0x7f69fe495a79 chromeos::attestation::PlatformVerificationDialog::Close() #2 0x7f69fa1a25d3 views::DialogClientView::CanClose() #3 0x7f69fa1a64d0 views::NonClientView::CanClose() #4 0x7f69fa18bb1f views::Widget::Close() https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:132: #endif On 2015/02/05 02:23:24, ddorwin wrote: > Ditto on #else. Or just ifdef the existence of this override. Done. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:190: if (!pending_id_.Equals(id)) On 2015/02/05 02:23:24, ddorwin wrote: > ditto on DCHECK This will happen if CancelPermissionRequest() is called. See l.129. https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.cc:197: NotifyPermissionSet(id, requesting_origin, embedding_origin, callback, On 2015/02/05 02:23:24, ddorwin wrote: > Like the base code, this appears to store by requesting and top-level origin, > but that is not how things appear to be presented in > chrome://settings/contentExceptions#popups. Are these actually stored by > combination? This is what I have in the preference file: "content_settings": { "pattern_pairs": { "http://[*.]www.netflix.com,http://[*.]www.netflix.com": { "protected-media-identifier": 1 } }, "pref_version": 1 }, Yeah, in chrome://settings/contentExceptions#popups we only have one domain, hmm.... https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.h:76: // destroyed first, which will invalidate weak pointers On 2015/02/05 02:08:14, Jun Mukai wrote: > I believe the callback won't run during the destructor, so strictly speaking I > guess this comment is not true. hmm, can you elaborate? This comment doesn't mention callback.... https://codereview.chromium.org/881983003/diff/1/chrome/browser/media/protect... chrome/browser/media/protected_media_identifier_permission_context.h:77: base::WeakPtrFactory<ProtectedMediaIdentifierPermissionContext> weak_factory_; On 2015/02/05 02:08:14, Jun Mukai wrote: > weak_factory_ is also used chromeos only, so might be a good idea to put into > ifdef OS_CHROMEOS Done.
Some replies in the previous PS. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:51: std::string origin = extension ? extension->name() : requesting_origin.spec(); On 2015/02/05 02:49:34, xhwang wrote: > On 2015/02/05 02:23:23, ddorwin wrote: > > should we confirm that the requesting_origin is the same as the url's origin? > > Otherwise, we might do the wrong thing. I wonder what the desired behavior > is... > > > > This could happen if an app is hosting an iframe on a server. Maybe this is > > handled in other ways, though. > > |requesting_origin| is the root of trust here, it's the real one requesting this > permission. We have url here only to get the extension/app name. But in the app case, we display the app name instead of the requesting origin. Thus, apps/extensions behave differently than normal web pages - not just in the use of a pretty name, but in the entity being identified. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:82: callback.Run(response_); On 2015/02/05 02:49:34, xhwang wrote: > On 2015/02/05 02:23:24, ddorwin wrote: > > Check requesting_origin.isvalid() or something like that? > > This is test code and requesting_origin is given.... I guess no need to check? It would test whether it gets passed through the product code. Minor issue, though. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:46: GURL extension_url = web_contents->GetLastCommittedURL(); nit: We don't actually know that this is an extension URL until line 52. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_flow.cc:242: // may be requested by a frame from a different origin. Add bug#? https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... chrome/browser/media/protected_media_identifier_permission_context.cc:104: #elif #else https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... chrome/browser/media/protected_media_identifier_permission_context.cc:135: #elif ditto
comments addressed
https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:51: std::string origin = extension ? extension->name() : requesting_origin.spec(); On 2015/02/05 03:25:26, ddorwin wrote: > On 2015/02/05 02:49:34, xhwang wrote: > > On 2015/02/05 02:23:23, ddorwin wrote: > > > should we confirm that the requesting_origin is the same as the url's > origin? > > > Otherwise, we might do the wrong thing. I wonder what the desired behavior > > is... > > > > > > This could happen if an app is hosting an iframe on a server. Maybe this is > > > handled in other ways, though. > > > > |requesting_origin| is the root of trust here, it's the real one requesting > this > > permission. We have url here only to get the extension/app name. > > But in the app case, we display the app name instead of the requesting origin. > Thus, apps/extensions behave differently than normal web pages - not just in the > use of a pretty name, but in the entity being identified. Do you mean to check that the frame's url is the same as the embedding url for extension/app? https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_flow_unittest.cc:82: callback.Run(response_); On 2015/02/05 03:25:26, ddorwin wrote: > On 2015/02/05 02:49:34, xhwang wrote: > > On 2015/02/05 02:23:24, ddorwin wrote: > > > Check requesting_origin.isvalid() or something like that? > > > > This is test code and requesting_origin is given.... I guess no need to check? > > It would test whether it gets passed through the product code. Minor issue, > though. Acknowledged. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:46: GURL extension_url = web_contents->GetLastCommittedURL(); On 2015/02/05 03:25:26, ddorwin wrote: > nit: We don't actually know that this is an extension URL until line 52. Done. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_flow.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_flow.cc:242: // may be requested by a frame from a different origin. On 2015/02/05 03:25:26, ddorwin wrote: > Add bug#? Done. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... chrome/browser/media/protected_media_identifier_permission_context.cc:104: #elif On 2015/02/05 03:25:27, ddorwin wrote: > #else Done. https://codereview.chromium.org/881983003/diff/20001/chrome/browser/media/pro... chrome/browser/media/protected_media_identifier_permission_context.cc:135: #elif On 2015/02/05 03:25:26, ddorwin wrote: > ditto Done.
LGTM. I think we need to solve the app issue (see comment), but I'm not sure what the correct solution is. We could just file a bug. https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:51: std::string origin = extension ? extension->name() : requesting_origin.spec(); On 2015/02/05 05:46:51, xhwang wrote: > On 2015/02/05 03:25:26, ddorwin wrote: > > On 2015/02/05 02:49:34, xhwang wrote: > > > On 2015/02/05 02:23:23, ddorwin wrote: > > > > should we confirm that the requesting_origin is the same as the url's > > origin? > > > > Otherwise, we might do the wrong thing. I wonder what the desired behavior > > > is... > > > > > > > > This could happen if an app is hosting an iframe on a server. Maybe this > is > > > > handled in other ways, though. > > > > > > |requesting_origin| is the root of trust here, it's the real one requesting > > this > > > permission. We have url here only to get the extension/app name. > > > > But in the app case, we display the app name instead of the requesting origin. > > Thus, apps/extensions behave differently than normal web pages - not just in > the > > use of a pretty name, but in the entity being identified. > > Do you mean to check that the frame's url is the same as the embedding url for > extension/app? Yes. And otherwise display the frame. Perhaps, if extension && equal, use name. Otherwise, use frame origin.
content_settings/ LGTM.
https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:49: .GetExtensionOrAppByURL(web_contents->GetLastCommittedURL()); Should we use requesting_origin here too? https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.h:34: const GURL& requesting_origin, nit: undocumented arg Also, this means the dialog can display something to the user that is different than the WebContents acting as the dialog parent. This seems misleading.
New patchsets have been uploaded after l-g-t-m from ddorwin@chromium.org,bauerb@chromium.org
comments addressed
https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:49: .GetExtensionOrAppByURL(web_contents->GetLastCommittedURL()); On 2015/02/05 16:40:44, Darren Krahn wrote: > Should we use requesting_origin here too? I am not sure. If the requesting_origin is different from the last committed URL this call will fail? +mukai for insights. https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.h (right): https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.h:34: const GURL& requesting_origin, On 2015/02/05 16:40:44, Darren Krahn wrote: > nit: undocumented arg Done. > Also, this means the dialog can display something to the user that is different > than the WebContents acting as the dialog parent. This seems misleading. hmm, this is consistent with how permission bubble/infobar works. After all it's |requesting_origin| requesting the permission, and |requesting_origin| can be very different from the url of the |web_contents|.
https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/881983003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:49: .GetExtensionOrAppByURL(web_contents->GetLastCommittedURL()); On 2015/02/05 18:09:48, xhwang wrote: > On 2015/02/05 16:40:44, Darren Krahn wrote: > > Should we use requesting_origin here too? > > I am not sure. If the requesting_origin is different from the last committed URL > this call will fail? +mukai for insights. Confirmed. GetExtensionOrAppByURL only expects chrome-extension://<id>/* urls. So it's really the true frame's url instead of the requesting origin. That said, we should correctly handle the case where the request is from an iframe within the extension. I added TODO and bug for this.
attestation/* LGTM
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/881983003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0d9194365f50890b7ac586c6e8ce3863c97c4027 Cr-Commit-Position: refs/heads/master@{#314924} |
