Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816773002:
Added switch for bypassing protected media identifier permission (Closed)
DescriptionAdded switch for bypassing protected media identifier permission
For testing protected content we need to bypass the prompt
for permission to play protected content. This change add the
switch --unsafely-allow-protected-media-identifier-for-domain
which will disable the info bar when protected content needs
permission when playing from localhost.
BUG=718608
Review-Url: https://codereview.chromium.org/2816773002
Cr-Commit-Position: refs/heads/master@{#471150}
Committed: https://chromium.googlesource.com/chromium/src/+/2ae33ffef0abd60c2fb050dbbadafae3345c568a
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added switch for by-passing permissions #
Total comments: 7
Patch Set 3 : Added switch for by-passing permissions #
Total comments: 7
Patch Set 4 : Added switch for by-passing permissions #Patch Set 5 : Added switch for bypassing permissions #
Total comments: 10
Patch Set 6 : Added switch for bypassing permissions #Patch Set 7 : Added switch for bypassing permissions #
Total comments: 8
Patch Set 8 : Added switch for bypassing permissions #
Total comments: 15
Patch Set 9 : Added switch for bypassing permissions #Patch Set 10 : Added switch for bypassing permissions #
Total comments: 16
Patch Set 11 : Added switch for bypassing permissions #Patch Set 12 : Added switch for bypassing permissions #
Total comments: 7
Patch Set 13 : Added switch for bypassing permissions #
Total comments: 4
Patch Set 14 : Added switch for bypassing permissions #Patch Set 15 : Added switch for bypassing permissions #
Total comments: 2
Patch Set 16 : Added switch for bypassing permissions #Patch Set 17 : Added switch for bypassing permissions #Patch Set 18 : Added switch for bypassing permissions #Patch Set 19 : Added switch for bypassing permissions #
Total comments: 6
Patch Set 20 : Corrected comments #Patch Set 21 : Rebasing to HEAD #Messages
Total messages: 96 (30 generated)
Description was changed from ========== Added switch for by-passing permissions For testing protected content we need to by-pass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-localhost which will disable the infor bar when protected content needs permission when playing from localhost. BUG= ========== to ========== Added switch for by-passing permissions For testing protected content we need to by-pass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-localhost which will disable the infor bar when protected content needs permission when playing from localhost. BUG= ==========
vaage@google.com changed reviewers: + joeyparrish@google.com
vaage@google.com changed reviewers: + assareh@google.com
https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.cc:106: #if defined(OS_ANDROID) I am of the opinion that this could be useful in other places than Android. ChromeOS, for example, may start prompting in similar scenarios in future, and it would be helpful if we didn't have to patch Chromium again when that happens. https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.cc:108: embedding_origin.host() == "localhost"; I found this, for context: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/WZsI_gFftmc Based on that, I think checking both here is reasonable. However, I think it would be better if host() did not have to be literally the string "localhost". "127.0.0.1", "localhost.localdomain", etc, should also be allowed (ideally). I hit cs.chromium.org and searched for "localhost" and "127.0.0.1", which led me to this utility: https://cs.chromium.org/chromium/src/net/base/url_util.cc?dr=CSs&l=350
raymes@chromium.org changed reviewers: + raymes@chromium.org
https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:115: switches::kDisableInfobarForProtectedMediaIdentifierForLocalhost); Could you elaborate on what kind of test is this needed for? We've avoided adding this kind of thing up until now for permissions.
https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:115: switches::kDisableInfobarForProtectedMediaIdentifierForLocalhost); On 2017/04/17 22:10:11, raymes wrote: > Could you elaborate on what kind of test is this needed for? We've avoided > adding this kind of thing up until now for permissions. This is for automated testing of protected content in Widevine's test lab. The current solution (accept the prompt the first time you run tests on the device) does not work for us because our tests are not run on a fixed port. We've discussed this with Xiaohan Wang, who is expecting a CL along these lines eventually. We weren't quite ready to ask for his review yet. :-) Happy to discuss in more detail if you would like.
Thanks for the explanation. A couple of additional comments below. https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:107: #if defined(OS_ANDROID) nit: these should have no identation https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:109: net::IsLoaclHost(embedding_origin.host()); nit: localhost is misspelt. https://codereview.chromium.org/2816773002/diff/20001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:115: switches::kDisableInfobarForProtectedMediaIdentifierForLocalhost); On 2017/04/17 22:49:37, Joey Parrish wrote: > On 2017/04/17 22:10:11, raymes wrote: > > Could you elaborate on what kind of test is this needed for? We've avoided > > adding this kind of thing up until now for permissions. > > This is for automated testing of protected content in Widevine's test lab. The > current solution (accept the prompt the first time you run tests on the device) > does not work for us because our tests are not run on a fixed port. > > We've discussed this with Xiaohan Wang, who is expecting a CL along these lines > eventually. We weren't quite ready to ask for his review yet. :-) > > Happy to discuss in more detail if you would like. Thanks for explaining. I think this is ok because there isn't really a more satisfying general mechanism right now. We do expose some settings to extensions to modify but we don't do that for protected media right now and I'm wary of adding new things there. Please add a test for this flag. https://codereview.chromium.org/2816773002/diff/20001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/20001/media/base/media_switch... media/base/media_switches.cc:68: const char kDisableInfobarForProtectedMediaIdentifierForLocalhost[] = Please add a description for this and it would be good to make it clear that this is only for testing purposes.
Thanks for the feedback. Is there a good example you can point me to for how tests are normally written for this module?
https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.cc:106: #if defined(OS_ANDROID) On 2017/04/17 19:45:18, Joey Parrish wrote: > I am of the opinion that this could be useful in other places than Android. > ChromeOS, for example, may start prompting in similar scenarios in future, and > it would be helpful if we didn't have to patch Chromium again when that happens. Done. https://codereview.chromium.org/2816773002/diff/1/chrome/browser/media/protec... chrome/browser/media/protected_media_identifier_permission_context.cc:108: embedding_origin.host() == "localhost"; On 2017/04/17 19:45:18, Joey Parrish wrote: > I found this, for context: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/WZsI_gFftmc > > Based on that, I think checking both here is reasonable. > > However, I think it would be better if host() did not have to be literally the > string "localhost". "127.0.0.1", "localhost.localdomain", etc, should also be > allowed (ideally). > > I hit http://cs.chromium.org and searched for "localhost" and "127.0.0.1", which led me > to this utility: > > https://cs.chromium.org/chromium/src/net/base/url_util.cc?dr=CSs&l=350 Done. https://codereview.chromium.org/2816773002/diff/20001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/20001/media/base/media_switch... media/base/media_switches.cc:68: const char kDisableInfobarForProtectedMediaIdentifierForLocalhost[] = On 2017/04/19 01:24:19, raymes wrote: > Please add a description for this and it would be good to make it clear that > this is only for testing purposes. Done.
https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:115: // Convert the switch value to a vector or URLs typo: vector _of_ URLs https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:130: found_embedding |= origins.at(i) == embedding_origin.host(); Have you confirmed that host() includes the port? https://codereview.chromium.org/2816773002/diff/40001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/40001/media/base/media_switch... media/base/media_switches.cc:68: // domains to by-pass the protected media identifier request prompt. nit: I don't think bypass should be hyphenated
On 2017/04/19 15:26:45, vaage wrote: > Thanks for the feedback. Is there a good example you can point me to for how > tests are normally written for this module? Tests in chromium live next to the units themselves. There are examples (ending in _unittest.cc) in chrome/browser/media, and they use gmock/gtest, if I'm not mistaken.
https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:115: // Convert the switch value to a vector or URLs On 2017/04/19 17:15:52, Joey Parrish wrote: > typo: vector _of_ URLs Done. https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:130: found_embedding |= origins.at(i) == embedding_origin.host(); On 2017/04/19 17:15:53, Joey Parrish wrote: > Have you confirmed that host() includes the port? If I am correct, host() does not include information about the port. Do we want to limit the port in addition to the domain? https://codereview.chromium.org/2816773002/diff/40001/media/base/media_switch... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/40001/media/base/media_switch... media/base/media_switches.cc:68: // domains to by-pass the protected media identifier request prompt. On 2017/04/19 17:15:53, Joey Parrish wrote: > nit: I don't think bypass should be hyphenated Done.
https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:130: found_embedding |= origins.at(i) == embedding_origin.host(); On 2017/04/19 17:26:22, vaage wrote: > On 2017/04/19 17:15:53, Joey Parrish wrote: > > Have you confirmed that host() includes the port? > > If I am correct, host() does not include information about the port. Do we want > to limit the port in addition to the domain? Good question. I thought so, but let's discuss offline and compare to similar flags that already exist.
On 2017/04/19 17:51:17, Joey Parrish wrote: > https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... > File chrome/browser/media/protected_media_identifier_permission_context.cc > (right): > > https://codereview.chromium.org/2816773002/diff/40001/chrome/browser/media/pr... > chrome/browser/media/protected_media_identifier_permission_context.cc:130: > found_embedding |= origins.at(i) == embedding_origin.host(); > On 2017/04/19 17:26:22, vaage wrote: > > On 2017/04/19 17:15:53, Joey Parrish wrote: > > > Have you confirmed that host() includes the port? > > > > If I am correct, host() does not include information about the port. Do we > want > > to limit the port in addition to the domain? > > Good question. I thought so, but let's discuss offline and compare to similar > flags that already exist. You might look into WebSecurityOrigin::create()->toRawString() (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/expor...). This appears to be what the --unsafely-treat-insecure-origin-as-secure uses to compare origins. (https://codereview.chromium.org/1082173003/diff/40002/Source/platform/weborig...)
assareh@chromium.org changed reviewers: + assareh@chromium.org
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:114: switches::kDisableInfobarForProtectedMediaIdentifierForDomain)) { As we had discussed offline, can you also verify that "command_line.HasSwitch(switches::kUserDataDir)"? This should make it likely that a user is tricked into using this flag on their main profile. https://codereview.chromium.org/2816773002/diff/80001/media/base/media_switch... File media/base/media_switches.h (right): https://codereview.chromium.org/2816773002/diff/80001/media/base/media_switch... media/base/media_switches.h:47: kDisableInfobarForProtectedMediaIdentifierForDomain[]; Can you also add this flag in bad_flags_bar.cc so that the security banner is shown to warn users that they have unsafe flags set. Similar to https://codereview.chromium.org/1072933006/diff/260001/chrome/browser/ui/star....
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:114: switches::kDisableInfobarForProtectedMediaIdentifierForDomain)) { On 2017/04/19 18:13:34, assareh1 wrote: > As we had discussed offline, can you also verify that > "command_line.HasSwitch(switches::kUserDataDir)"? This should make it likely > that a user is tricked into using this flag on their main profile. *less* likely :)
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:122: } Is this extra complexity really necessary? Was the localhost check not enough?
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:122: } On 2017/04/19 23:05:47, raymes wrote: > Is this extra complexity really necessary? Was the localhost check not enough? By whitelisting a user-specified domain vs only localhost, we significantly expand the usefulness of this flag. In most cases, it is unlikely that the web service can be hosted on an Android device. Therefore this flag requires additional tunneling to be useful. If we can whitelist the actual domain being used in test then we can remove the need for setting up adb tunnels.
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:109: // safe - suppress the request and allow. I think we should link to a bug or something that mentions the specific reason this was added. Specifically, it's not because we need it for automated tests in Chrome but because widevine needs it. https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:122: } On 2017/04/20 15:37:07, assareh1 wrote: > On 2017/04/19 23:05:47, raymes wrote: > > Is this extra complexity really necessary? Was the localhost check not enough? > > By whitelisting a user-specified domain vs only localhost, we significantly > expand the usefulness of this flag. In most cases, it is unlikely that the web > service can be hosted on an Android device. Therefore this flag requires > additional tunneling to be useful. If we can whitelist the actual domain being > used in test then we can remove the need for setting up adb tunnels. Ok. I would suggest rather than comparing the host, that you construct a GURL from the string and then compare equality. https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:128: for (size_t i = 0; i < origins.size(); i++) { Rather than looping through everything again, can you just do it all in the loop above and not have the vector? https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:131: } I would suggest just we just compare the requesting origin here - I don't think it will be too important to use the embedding origin: if (requesting_origin == GURL(whitelisted_origin)) return CONTENT_SETTING_ALLOW;
https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... chrome/browser/media/protected_media_identifier_permission_context.cc:109: // safe - suppress the request and allow. On 2017/04/20 23:12:42, raymes wrote: > I think we should link to a bug or something that mentions the specific reason > this was added. Specifically, it's not because we need it for automated tests in > Chrome but because widevine needs it. We've got b/35045892 filed internally, if that's helpful. We don't have any public-facing bug filed on chromium, since we decided to take it upon ourselves to contribute a patch.
On 2017/04/20 23:25:57, Joey Parrish wrote: > https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... > File chrome/browser/media/protected_media_identifier_permission_context.cc > (right): > > https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... > chrome/browser/media/protected_media_identifier_permission_context.cc:109: // > safe - suppress the request and allow. > On 2017/04/20 23:12:42, raymes wrote: > > I think we should link to a bug or something that mentions the specific reason > > this was added. Specifically, it's not because we need it for automated tests > in > > Chrome but because widevine needs it. > > We've got b/35045892 filed internally, if that's helpful. We don't have any > public-facing bug filed on chromium, since we decided to take it upon ourselves > to contribute a patch. I am still working to get this to run on my Android device.
> We've got b/35045892 <https://buganizer.corp.google.com/35045892> filed internally, if that's helpful. I think it would be good to file a bug on crbug.com as well so that when people are digging through git history they can find out why this was added. On Fri, 21 Apr 2017 at 09:25 <joeyparrish@google.com> wrote: > > > https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... > File > chrome/browser/media/protected_media_identifier_permission_context.cc > (right): > > > https://codereview.chromium.org/2816773002/diff/80001/chrome/browser/media/pr... > chrome/browser/media/protected_media_identifier_permission_context.cc:109: > // safe - suppress the request and allow. > On 2017/04/20 23:12:42, raymes wrote: > > I think we should link to a bug or something that mentions the > specific reason > > this was added. Specifically, it's not because we need it for > automated tests in > > Chrome but because widevine needs it. > > We've got b/35045892 <https://buganizer.corp.google.com/35045892> filed > internally, if that's helpful. We don't have > any public-facing bug filed on chromium, since we decided to take it > upon ourselves to contribute a patch. > > https://codereview.chromium.org/2816773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
vaage@google.com changed reviewers: + xhwang@chromium.org
Xiaohan, please take a look.
LGTM otherwise https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:18: // The request should need to ask for permission I don't understand this comment in context of the next 6 lines of code. It looks like you are showing that none of the possible settings are modified when the whitelist is empty. https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:89: TEST_F(ProtectedMediaIdentifierWhitelistTest, BypassWithFlag) { From reading the test, I don't understand the purpose of it. The name is also not helping me understand it. It appears that this covers exactly the same things as the previous test. Did I miss something? https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:110: TEST_F(ProtectedMediaIdentifierWhitelistTest, BypassWithFlagAndSubdomain) { I would love to have Xiaohan's feedback on this part. I don't think it matters to our testing whether or not subdomains are included. Are there reasons they should or should not be included? https://codereview.chromium.org/2816773002/diff/120001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/120001/media/base/media_switc... media/base/media_switches.cc:68: // domains to bypass the protected media identifier request prompt. Probably good to mention here that user-data-dir must also be specified.
https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:18: // The request should need to ask for permission On 2017/05/04 17:06:14, Joey Parrish wrote: > I don't understand this comment in context of the next 6 lines of code. It > looks like you are showing that none of the possible settings are modified when > the whitelist is empty. Done. https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:89: TEST_F(ProtectedMediaIdentifierWhitelistTest, BypassWithFlag) { On 2017/05/04 17:06:14, Joey Parrish wrote: > From reading the test, I don't understand the purpose of it. The name is also > not helping me understand it. It appears that this covers exactly the same > things as the previous test. Did I miss something? Done. https://codereview.chromium.org/2816773002/diff/120001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:110: TEST_F(ProtectedMediaIdentifierWhitelistTest, BypassWithFlagAndSubdomain) { On 2017/05/04 17:06:14, Joey Parrish wrote: > I would love to have Xiaohan's feedback on this part. I don't think it matters > to our testing whether or not subdomains are included. Are there reasons they > should or should not be included? I found it easier to work with if I use the domain check as it meant that I would only need to list the root domain if I was using sub domains and the check appeared to be a more versatile check. The other methods of checking required the string being converted to a URL and the check was more complicated . This seemed like the more friendly and versatile method. https://codereview.chromium.org/2816773002/diff/120001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/120001/media/base/media_switc... media/base/media_switches.cc:68: // domains to bypass the protected media identifier request prompt. On 2017/05/04 17:06:15, Joey Parrish wrote: > Probably good to mention here that user-data-dir must also be specified. Done.
https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:69: // will have no effect if user-data-dir is not enabled. nit: "set", rather than "enabled". user-data-dir requires an argument, so it's not a thing you "enable" so much as "specify" or "set".
https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:69: // will have no effect if user-data-dir is not enabled. On 2017/05/04 18:04:35, Joey Parrish wrote: > nit: "set", rather than "enabled". user-data-dir requires an argument, so it's > not a thing you "enable" so much as "specify" or "set". Done.
lgtm
looking pretty good. I just have a few comments. https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.cc (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:16: ContentSetting ProtectedMediaIdentifierWhitelist::modifySettingFor( This serves it's purpose. But really what it does is to check switches and check whether an origin's domain is whitelisted. IMHO it'll be cleaner to just have a IsOriginWhiteListed(origin) call, and put all the content setting code in GetPermissionStatusInternal(). That way, it's clear that only when a domain is whitelisted and the status is ASK, we bypass the prompt and return "ALLOW" directly. The unit tests will also be much simpler that way. https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:26: if (command_line.HasSwitch(switches::kUserDataDir) && Also check kUserDataDir isn't empty? https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.h (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.h:16: ContentSetting modifySettingFor(ContentSetting original, C++ Style: ModifySettingFor https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:29: command_line->AppendSwitch(switches::kUserDataDir); Since you already have test fixture (class) ProtectedMediaIdentifierWhitelistTest, you can put these common init code in ProtectedMediaIdentifierWhitelistTest's constructor, or in the overridden SetUp() method. Typically we prefer the constructor though. See https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:29: command_line->AppendSwitch(switches::kUserDataDir); Should use AppendSwitchASCII for kUserDataDir? https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/ui/star... File chrome/browser/ui/startup/bad_flags_prompt.cc (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/ui/star... chrome/browser/ui/startup/bad_flags_prompt.cc:96: switches::kDisableInfobarForProtectedMediaIdentifierForDomain, cool, I didn't know about this list. https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:68: // domains to bypass the protected media identifier request prompt. This flag nit: s/request/permission https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:69: // will have no effect if user-data-dir is not enabled. Please add an example how to use this, something similar to https://cs.chromium.org/chromium/src/chrome/common/chrome_switches.cc?rcl=2c9...
BTW, 1. Please do file a http://crbug.com for tracking. Typically a lot of the design discussions in this codereview can happen in the bug. 2. Please update the CL title to make it clear this only affects protected media identifier permission. 3. Please update the CL description which is outdated, e.g. --disable-infobar-for-protected-media-identifier-for-localhost is not up-to-date.
Description was changed from ========== Added switch for by-passing permissions For testing protected content we need to by-pass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-localhost which will disable the infor bar when protected content needs permission when playing from localhost. BUG= ========== to ========== Added switch for bypassing permissions For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG= ==========
https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.cc (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:16: ContentSetting ProtectedMediaIdentifierWhitelist::modifySettingFor( On 2017/05/04 19:57:37, xhwang wrote: > This serves it's purpose. But really what it does is to check switches and check > whether an origin's domain is whitelisted. > > IMHO it'll be cleaner to just have a IsOriginWhiteListed(origin) call, and put > all the content setting code in GetPermissionStatusInternal(). That way, it's > clear that only when a domain is whitelisted and the status is ASK, we bypass > the prompt and return "ALLOW" directly. > > The unit tests will also be much simpler that way. Done. Good call. That really cleans it up. https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:26: if (command_line.HasSwitch(switches::kUserDataDir) && On 2017/05/04 19:57:37, xhwang wrote: > Also check kUserDataDir isn't empty? Done. https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/140001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:29: command_line->AppendSwitch(switches::kUserDataDir); On 2017/05/04 19:57:37, xhwang wrote: > Since you already have test fixture (class) > ProtectedMediaIdentifierWhitelistTest, you can put these common init code in > ProtectedMediaIdentifierWhitelistTest's constructor, or in the overridden > SetUp() method. Typically we prefer the constructor though. > > See > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Done. https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:68: // domains to bypass the protected media identifier request prompt. This flag On 2017/05/04 19:57:37, xhwang wrote: > nit: s/request/permission Done. https://codereview.chromium.org/2816773002/diff/140001/media/base/media_switc... media/base/media_switches.cc:69: // will have no effect if user-data-dir is not enabled. On 2017/05/04 19:57:38, xhwang wrote: > Please add an example how to use this, something similar to > https://cs.chromium.org/chromium/src/chrome/common/chrome_switches.cc?rcl=2c9... Done.
lgtm
Description was changed from ========== Added switch for bypassing permissions For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG= ========== to ========== Added switch for bypassing permissions For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ==========
Opened the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=718608
Thanks! Please also update the CL title to make sure this CL is not for all permissions. Note that you have to modify it manually here. Changing the title in your local branch will not update the title here. Otherwise I just have a few more comments. Thanks! https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:117: } If you make IsOriginWhitelisted() as a static function (see below), this block can be further simplified to: // For automated testing of protected content - having a prompt that // requires user intervention is problematic. If the domain has been // whitelisted as safe - suppress the request and allow. if (content_setting == CONTENT_SETTING_ASK && IsOriginWhitelisted(requesting_origin)) { content_setting = CONTENT_SETTING_ALLOW; } https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:23: ? command_line.GetSwitchValueASCII(switches::kUserDataDir) GetSwitchValueASCII() returns empty string when the flag is not present, so you don't need to check both. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:44: } In Chromium, we like to return early so that readers don't need to remember a lot of local variables/state. I made some modifications on your code and it looks like below. How do you like it? bool ProtectedMediaIdentifierWhitelist::IsOriginWhitelisted( const GURL& requesting_origin) const { const base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess(); if (command_line.GetSwitchValueASCII(switches::kUserDataDir).empty()) return false; const std::string whitelist = command_line.GetSwitchValueASCII( switches::kDisableInfobarForProtectedMediaIdentifierForDomain); for (const std::string& origin : base::SplitString( whitelist, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) { if (requesting_origin.DomainIs(origin)) return true; } return false; } https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.h (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.h:16: bool IsOriginWhitelisted(const GURL& requesting_origin) const; Looks like we don't need a class then? Shall we just define a function here? You can also fold the function to protected_media_identifier_permission_context.h, but that' totally up to you. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:5: #include "chrome/browser/media/protected_media_identifier_whitelist.h" style: add an empty line here https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:15: GURL requestingSubDomain; style: requesting_domain_ https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:18: base::CommandLine* command_line; style: command_line_ https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:26: } style: put methods to be above variables. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:85: command_line->AppendSwitchASCII(switches::kUserDataDir, "/dir/for/testing"); Also add a test that kUserDataDir is appended without the real string.
https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:117: } On 2017/05/04 22:10:59, xhwang wrote: > If you make IsOriginWhitelisted() as a static function (see below), this block > can be further simplified to: > > // For automated testing of protected content - having a prompt that > // requires user intervention is problematic. If the domain has been > // whitelisted as safe - suppress the request and allow. > if (content_setting == CONTENT_SETTING_ASK && > IsOriginWhitelisted(requesting_origin)) { > content_setting = CONTENT_SETTING_ALLOW; > } Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.cc:23: ? command_line.GetSwitchValueASCII(switches::kUserDataDir) On 2017/05/04 22:10:59, xhwang wrote: > GetSwitchValueASCII() returns empty string when the flag is not present, so you > don't need to check both. Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist.h (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist.h:16: bool IsOriginWhitelisted(const GURL& requesting_origin) const; On 2017/05/04 22:10:59, xhwang wrote: > Looks like we don't need a class then? Shall we just define a function here? You > can also fold the function to protected_media_identifier_permission_context.h, > but that' totally up to you. Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_whitelist_unittest.cc (right): https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:15: GURL requestingSubDomain; On 2017/05/04 22:10:59, xhwang wrote: > style: requesting_domain_ Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:18: base::CommandLine* command_line; On 2017/05/04 22:10:59, xhwang wrote: > style: command_line_ Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:26: } On 2017/05/04 22:10:59, xhwang wrote: > style: put methods to be above variables. Done. https://codereview.chromium.org/2816773002/diff/180001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_whitelist_unittest.cc:85: command_line->AppendSwitchASCII(switches::kUserDataDir, "/dir/for/testing"); On 2017/05/04 22:10:59, xhwang wrote: > Also add a test that kUserDataDir is appended without the real string. Done.
https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/ui/star... File chrome/browser/ui/startup/bad_flags_prompt.cc (right): https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/bad_flags_prompt.cc:94: // This flag is only for automated protected media tests to prevent the This describes why the flag exists but not what the flag does. I believe in this context it should be the otherway around. // This flag allows sites to access protected media identifiers without getting the user's permission. https://codereview.chromium.org/2816773002/diff/220001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/220001/media/base/media_switc... media/base/media_switches.cc:71: // --disable-infobar-for-protected-media-identifier-for-domain=a.com,b.ca Can you clarify if port is required here? https://codereview.chromium.org/2816773002/diff/220001/media/base/media_switc... media/base/media_switches.cc:71: // --disable-infobar-for-protected-media-identifier-for-domain=a.com,b.ca Putting the crbug in the comment might be a good idea so readers can get more info about this feature.
LGTM % nit, please also take a look at assareh's comments. https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.h:55: static bool IsOriginWhitelisted(const GURL& requesting_origin); You can make this private, and declare ProtectedMediaIdentifierPermissionContextTest as a friend class so it can test it.
https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.h:55: static bool IsOriginWhitelisted(const GURL& requesting_origin); On 2017/05/05 16:55:57, xhwang wrote: > You can make this private, and declare > ProtectedMediaIdentifierPermissionContextTest as a friend class so it can test > it. Done.
https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.h:63: BypassRequiresUserDataDir); Will it work if you just have: friend class ProtectedMediaIdentifierPermissionContextTest; ?
https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:131: for (const std::string& origin : base::SplitString( nit: origin->domain
https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:131: for (const std::string& origin : base::SplitString( On 2017/05/07 22:07:26, raymes wrote: > nit: origin->domain Done. https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.h (right): https://codereview.chromium.org/2816773002/diff/240001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.h:63: BypassRequiresUserDataDir); At first I could not get it to work but after some digging I was able to find a way to get that to work.
https://codereview.chromium.org/2816773002/diff/280001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/280001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:120: const GURL& requesting_domain) { This should still be "origin" (see the callsite). Also requesting_domain.DomainIs(domain) doesn't make sense, it should be requesting_origin.DomainIs(domain). Also, the function name should be IsOriginWhitelisted() as well.
https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/ui/star... File chrome/browser/ui/startup/bad_flags_prompt.cc (right): https://codereview.chromium.org/2816773002/diff/220001/chrome/browser/ui/star... chrome/browser/ui/startup/bad_flags_prompt.cc:94: // This flag is only for automated protected media tests to prevent the On 2017/05/05 16:32:23, assareh1 wrote: > This describes why the flag exists but not what the flag does. I believe in this > context it should be the otherway around. > > // This flag allows sites to access protected media identifiers without getting > the user's permission. Done. https://codereview.chromium.org/2816773002/diff/220001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/220001/media/base/media_switc... media/base/media_switches.cc:71: // --disable-infobar-for-protected-media-identifier-for-domain=a.com,b.ca On 2017/05/05 16:32:23, assareh1 wrote: > Putting the crbug in the comment might be a good idea so readers can get more > info about this feature. Done. https://codereview.chromium.org/2816773002/diff/280001/chrome/browser/media/p... File chrome/browser/media/protected_media_identifier_permission_context.cc (right): https://codereview.chromium.org/2816773002/diff/280001/chrome/browser/media/p... chrome/browser/media/protected_media_identifier_permission_context.cc:120: const GURL& requesting_domain) { On 2017/05/08 17:21:37, xhwang wrote: > This should still be "origin" (see the callsite). Also > requesting_domain.DomainIs(domain) doesn't make sense, > it should be requesting_origin.DomainIs(domain). > > Also, the function name should be IsOriginWhitelisted() as well. Done.
Please also update the first line of the CL description, which will become the real CL title after it's committed. Otherwise LGTM++
Description was changed from ========== Added switch for bypassing permissions For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ========== to ========== Added switch for bypassing protected media identifier permission For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ==========
On 2017/05/08 18:12:55, xhwang wrote: > Please also update the first line of the CL description, which will become the > real CL title after it's committed. > > Otherwise LGTM++ Thanks for the catch.
Tip: Please add OWNERS and ping other reviewers for review, otherwise people will not notice your CL (update). Also, I've landed some trivial changes in the same set of files, so you probably need to do a rebase before committing.
vaage@google.com changed reviewers: + sky@chromium.org
sky, can you take a look?
Did you have someone from security look at this?
sky@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez to make sure this is ok from a security perspective.
On 2017/05/10 15:54:26, sky wrote: > Did you have someone from security look at this? Not that I know of. Is there someone else I should add to the review?
lgtm Yes, command line flags to do unsafe things are OK. I would recommend choosing a name for your flag with the word "unsafe" in it, however. Maybe --unsafe-disable-... But this this is already too long; do as you like.
Like you said, it is already long and we do have it set up to warn the user if it is set, so I think leaving it as is would be okay.
Changed flag name as per assareh's offline suggestion.
LGTM
The CQ bit was checked by vaage@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joeyparrish@google.com, xhwang@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2816773002/#ps340001 (title: "Added switch for bypassing permissions")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/11 15:13:22, Vaage wrote: > Changed flag name as per assareh's offline suggestion. Just for posterity, Aaron and I discussed offline how "disable-infobar-for..." doesn't convey that the flag will grant permission. The old name could be perceived as only skipping the user prompt, but logically that would also imply that the "no permission" state was maintained. The new name should be more clear that it is granting permission and therefore no prompting is necessary.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vaage@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, joeyparrish@google.com, assareh@chromium.org, tsepez@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2816773002/#ps360001 (title: "Added switch for bypassing permissions")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nit: CL description is now out of date.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/11 17:47:20, Tom Sepez wrote: > nit: CL description is now out of date. Thanks for the catch.
Description was changed from ========== Added switch for bypassing protected media identifier permission For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --disable-infobar-for-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ========== to ========== Added switch for bypassing protected media identifier permission For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --unsafely-allow-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ==========
https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:71: // domains (e.g. example.com) to allow the protected media identifier to be "allows to allow" sounds a bit odd https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:72: // shared with out showing the user a permission prompt. This flag ignores the nit: s/with out/without https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:73: // port number. This flag will have no effect if user-data-dir is not set. Please add a comment that this will not change user's content settings.
https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... File media/base/media_switches.cc (right): https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:71: // domains (e.g. example.com) to allow the protected media identifier to be On 2017/05/11 18:31:49, xhwang wrote: > "allows to allow" sounds a bit odd Done. https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:72: // shared with out showing the user a permission prompt. This flag ignores the On 2017/05/11 18:31:49, xhwang wrote: > nit: s/with out/without Done. https://codereview.chromium.org/2816773002/diff/360001/media/base/media_switc... media/base/media_switches.cc:73: // port number. This flag will have no effect if user-data-dir is not set. On 2017/05/11 18:31:49, xhwang wrote: > Please add a comment that this will not change user's content settings. Done.
lgtm
The CQ bit was checked by vaage@google.com to run a CQ dry run
lgtm
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by vaage@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vaage@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, assareh@chromium.org, tsepez@chromium.org, joeyparrish@google.com, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2816773002/#ps400001 (title: "Rebasing to HEAD")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1494548847963840,
"parent_rev": "23b616383acde9c16e0892084498620ecc46856f", "commit_rev":
"2ae33ffef0abd60c2fb050dbbadafae3345c568a"}
Message was sent while issue was closed.
Description was changed from ========== Added switch for bypassing protected media identifier permission For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --unsafely-allow-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 ========== to ========== Added switch for bypassing protected media identifier permission For testing protected content we need to bypass the prompt for permission to play protected content. This change add the switch --unsafely-allow-protected-media-identifier-for-domain which will disable the info bar when protected content needs permission when playing from localhost. BUG=718608 Review-Url: https://codereview.chromium.org/2816773002 Cr-Commit-Position: refs/heads/master@{#471150} Committed: https://chromium.googlesource.com/chromium/src/+/2ae33ffef0abd60c2fb050dbbada... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/2ae33ffef0abd60c2fb050dbbada... |
