Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2552203007:
Public Sessions - prompt the user for pageCapture requests (Closed)
|
Created:
4 years ago by Ivan Šandrk Modified:
3 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPublic Sessions - prompt the user for pageCapture requests
In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API.
BUG=689478
BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest*
UNITTEST=PublicSessionPermissionHelperTest.*
Review-Url: https://codereview.chromium.org/2552203007
Cr-Commit-Position: refs/heads/master@{#450383}
Committed: https://chromium.googlesource.com/chromium/src/+/309c0997afe19ab971686abf5cca1dd7fd4f7d8b
Patch Set 1 #Patch Set 2 : Fixed up prompt #Patch Set 3 : Updated comments #Patch Set 4 : Store status in prefs, tests #Patch Set 5 : #if defined(OS_CHROMEOS) #Patch Set 6 : Unused variable #
Total comments: 14
Patch Set 7 : GetAssociatedWebContents, nits #
Total comments: 14
Patch Set 8 : Moved CrOS only code to a separate file #
Total comments: 31
Patch Set 9 : Devlin's comments #
Total comments: 6
Patch Set 10 : Rebase #Patch Set 11 : Refactored use of IsPublicSession() #Patch Set 12 : Nits #
Total comments: 11
Patch Set 13 : Changed pref name & location; removed incognito code #
Total comments: 1
Patch Set 14 : Moved pref to ExtensionPrefs #Patch Set 15 : Added PermissionHelper which handles permission requests and caches user choices #
Total comments: 37
Patch Set 16 : Devlin's comments, added other files #
Total comments: 19
Patch Set 17 : Merged helper functionality into one class, others #Patch Set 18 : Removed _impl.cc/h #
Total comments: 14
Patch Set 19 : More nits #
Total comments: 20
Patch Set 20 : Using a prompt id #
Total comments: 5
Patch Set 21 : Removed PermissionAllowed(), only one callback, moved impl to .cc file #Patch Set 22 : Using PermissionIDSet instead of PermissionHelperSet, better CHECK #Patch Set 23 : Copy callbacks to be invoked, move prompt deletion (had a crash) #
Total comments: 9
Patch Set 24 : Drew's comments, unittest #Patch Set 25 : Moved PublicSessionTabCaptureAccessHandler to parent directory #
Total comments: 2
Patch Set 26 : Const ref for passing PermissionIDSet around #
Total comments: 2
Patch Set 27 : const PermissionIDSet& in unittest #Patch Set 28 : Using factory callback for prompts #
Total comments: 10
Patch Set 29 : Drew's comments #
Total comments: 17
Patch Set 30 : Unittest - WeakPtr, detecting prompt shown/not shown, nits #
Total comments: 6
Patch Set 31 : More robust check for whether the dialog was shown #Patch Set 32 : Git cl format #Patch Set 33 : Rebase #Patch Set 34 : Nitfix #Messages
Total messages: 213 (149 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Description was changed from ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 ========== to ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 ==========
isandrk@chromium.org changed reviewers: + atwilson@chromium.org, rdevlin.cronin@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hey Devlin, please do a general review. I still need to fix up some comments, and figure out a place where to store user choices (got any suggestions btw?).
The CQ bit was checked by isandrk@chromium.org 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.
On 2016/12/08 20:52:45, Ivan Šandrk wrote: > Hey Devlin, please do a general review. I still need to fix up some comments, > and figure out a place where to store user choices (got any suggestions btw?). Devlin can you take a quick look and tell me if you see anything obviously wrong with the approach. The current version doesn't persist the choices across the user session (obviously), I'll probably stick it in prefs. Ignore this for now.
A few thoughts. First off, how many of these are there going to be? Seems like we have pageCapture and streaming. If there are going to be more, it would perhaps make sense to have a more general place to put these, which could be reused for all of them (and could even be used in the future for more runtime permissions). But if this is the last one, perhaps not worth it yet. Second, smaller note: we'll want to handle the extension making multiple requests while the dialog is open (before the user has made a choice).
The CQ bit was checked by isandrk@chromium.org 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...
Description was changed from ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 ========== to ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* ==========
Hey Devlin, it's ready for review now. Ptal! On 2016/12/09 21:00:57, Devlin (in MON - PST plus 3) wrote: > A few thoughts. First off, how many of these are there going to be? Seems like > we have pageCapture and streaming. If there are going to be more, it would > perhaps make sense to have a more general place to put these, which could be > reused for all of them (and could even be used in the future for more runtime > permissions). But if this is the last one, perhaps not worth it yet. This is the last one for now. > Second, smaller note: we'll want to handle the extension making multiple > requests while the dialog is open (before the user has made a choice). Done!
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 isandrk@chromium.org 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 checked by isandrk@chromium.org 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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by isandrk@chromium.org 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...
https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:251: // callback on destruction of this. What if this object outlives the Prefs? https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:266: prompt_ = base::MakeUnique<ExtensionInstallPrompt>(GetWebContents()); GetWebContents() can return null here, which would be bad for the prompt. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:66: #if defined(OS_CHROMEOS) This is a fair amount of cros-only code. I think it would maybe be cleaner if we pulled this into a public session helper-type object in a file only compiled on cros, and limited this to (pseudo-code) #if defined(cros) PageCapturePermissionHelper permission_helper_; #endif .cc #if defined(cros) permission_helper_.RequestPermission(base::Bind(&OnPermissionRequested)); #else OnPermissionRequested(GRANTED); #endif https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:77: // Function used to resolve user decision regarding allowing page capture. nit: typically, we don't need "Function used to"-type references in the function comment, because it's clear that it's a function (and presumably will be used). :) So here, let's just say something like "Handles the user decision of whether to allow page capture." or something like that. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:82: void ChainRunAsync(); I'm not sure the "ChainRunAsync" name makes sense to me...
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 isandrk@chromium.org 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...
https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:251: // callback on destruction of this. On 2016/12/12 20:09:37, Devlin wrote: > What if this object outlives the Prefs? Prefs are destoyed during logout. This object either gets destroyed together with the browser (before logout), or it doesn't get destroyed at all (is that a memory leak btw?) so the PrefChangeRegistrar doesn't touch anything. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:266: prompt_ = base::MakeUnique<ExtensionInstallPrompt>(GetWebContents()); On 2016/12/12 20:09:37, Devlin wrote: > GetWebContents() can return null here, which would be bad for the prompt. Good catch. Replaced it with GetAssociatedWebContents which is a bit more sturdy afaict (and also added a null check). https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:66: #if defined(OS_CHROMEOS) On 2016/12/12 20:09:37, Devlin wrote: > This is a fair amount of cros-only code. I think it would maybe be cleaner if > we pulled this into a public session helper-type object in a file only compiled > on cros, and limited this to > > (pseudo-code) > > #if defined(cros) > PageCapturePermissionHelper permission_helper_; > #endif > > .cc > #if defined(cros) > permission_helper_.RequestPermission(base::Bind(&OnPermissionRequested)); > #else > OnPermissionRequested(GRANTED); > #endif That's a good point. Are you fine with me doing a follow up CL to address this? I was planning anyhow on refactoring the work I've done so far on permissions. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:77: // Function used to resolve user decision regarding allowing page capture. On 2016/12/12 20:09:37, Devlin wrote: > nit: typically, we don't need "Function used to"-type references in the function > comment, because it's clear that it's a function (and presumably will be used). > :) So here, let's just say something like "Handles the user decision of whether > to allow page capture." or something like that. True, done. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:82: void ChainRunAsync(); On 2016/12/12 20:09:37, Devlin wrote: > I'm not sure the "ChainRunAsync" name makes sense to me... Does ResolvePermissionRequest sound better?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:266: prompt_ = base::MakeUnique<ExtensionInstallPrompt>(GetWebContents()); On 2016/12/13 16:32:47, Ivan Šandrk wrote: > On 2016/12/12 20:09:37, Devlin wrote: > > GetWebContents() can return null here, which would be bad for the prompt. > > Good catch. Replaced it with GetAssociatedWebContents which is a bit more sturdy > afaict (and also added a null check). GetAssociatedWebContents() isn't particularly appropriate here. I think the original intent of GetWebContents() was to retrieve the web contents that were going to be captured - which largely makes sense. And if we can't find that web contents, we should probably just return an error. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:66: #if defined(OS_CHROMEOS) On 2016/12/13 16:32:47, Ivan Šandrk wrote: > On 2016/12/12 20:09:37, Devlin wrote: > > This is a fair amount of cros-only code. I think it would maybe be cleaner if > > we pulled this into a public session helper-type object in a file only > compiled > > on cros, and limited this to > > > > (pseudo-code) > > > > #if defined(cros) > > PageCapturePermissionHelper permission_helper_; > > #endif > > > > .cc > > #if defined(cros) > > permission_helper_.RequestPermission(base::Bind(&OnPermissionRequested)); > > #else > > OnPermissionRequested(GRANTED); > > #endif > > That's a good point. Are you fine with me doing a follow up CL to address this? > I was planning anyhow on refactoring the work I've done so far on permissions. Is there a reason not to do it here? I don't feel terribly strongly, but have a slight preference for having them land together. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:236: void PageCaptureSaveAsMHTMLFunction::HandlePermissionRequest() { It's kind of weird that we define this function for all platforms, but it's only used for CrOS... (which would be fixed with pulling out cros-related code) https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:254: pref_change_registrar_->Add(prefs::kPublicSessionPermissionsUserChoiceCache, Is this git cl format'd? https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:314: return GetProfile()->GetOffTheRecordPrefs(); Why OffTheRecordPrefs()? (add a comment) https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:327: int value; nit: initialize value
https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:53: bool IsPublicSession() { Seems like this should be in ifdef(OS_CHROMEOS), and also the one caller should also be ifdef(OS_CHROMEOS) - this would allow you to similarly make HandlePermissionPrompt cros-only. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:253: // callback on destruction of this. nit: this object https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:330: UserChoiceSet(static_cast<PermissionState>(value)); Why do we need to call UserChoiceSet here? Why not just set value = NOT_PROMPTED and fall through to return it? It's just weird to have two possible states for an unprompted extension to be in - one where the extension is referenced in kPublicSessionPermissionsUserChoiceCache and one where it isn't. Seems more consistent to say that the UserChoiceCache only holds actual user choices.
The CQ bit was checked by isandrk@chromium.org 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...
Hey Devlin, ptal! https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:266: prompt_ = base::MakeUnique<ExtensionInstallPrompt>(GetWebContents()); On 2016/12/13 21:51:29, Devlin wrote: > On 2016/12/13 16:32:47, Ivan Šandrk wrote: > > On 2016/12/12 20:09:37, Devlin wrote: > > > GetWebContents() can return null here, which would be bad for the prompt. > > > > Good catch. Replaced it with GetAssociatedWebContents which is a bit more > sturdy > > afaict (and also added a null check). > > GetAssociatedWebContents() isn't particularly appropriate here. I think the > original intent of GetWebContents() was to retrieve the web contents that were > going to be captured - which largely makes sense. And if we can't find that web > contents, we should probably just return an error. I'd personally show the permission dialog anyway, but this also makes sense. Done. https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:66: #if defined(OS_CHROMEOS) On 2016/12/13 21:51:29, Devlin (ooo until Jan 4) wrote: > On 2016/12/13 16:32:47, Ivan Šandrk wrote: > > On 2016/12/12 20:09:37, Devlin wrote: > > > This is a fair amount of cros-only code. I think it would maybe be cleaner > if > > > we pulled this into a public session helper-type object in a file only > > compiled > > > on cros, and limited this to > > > > > > (pseudo-code) > > > > > > #if defined(cros) > > > PageCapturePermissionHelper permission_helper_; > > > #endif > > > > > > .cc > > > #if defined(cros) > > > permission_helper_.RequestPermission(base::Bind(&OnPermissionRequested)); > > > #else > > > OnPermissionRequested(GRANTED); > > > #endif > > > > That's a good point. Are you fine with me doing a follow up CL to address > this? > > I was planning anyhow on refactoring the work I've done so far on permissions. > > Is there a reason not to do it here? I don't feel terribly strongly, but have a > slight preference for having them land together. Done. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:53: bool IsPublicSession() { On 2016/12/14 13:39:21, Andrew T Wilson (Slow) wrote: > Seems like this should be in ifdef(OS_CHROMEOS), and also the one caller should > also be ifdef(OS_CHROMEOS) - this would allow you to similarly make > HandlePermissionPrompt cros-only. Done. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:236: void PageCaptureSaveAsMHTMLFunction::HandlePermissionRequest() { On 2016/12/13 21:51:29, Devlin wrote: > It's kind of weird that we define this function for all platforms, but it's only > used for CrOS... (which would be fixed with pulling out cros-related code) Wanted to see how this variant fared - the regular code didn't have to use any ifdefs, so seemingly less modifications to the original. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:253: // callback on destruction of this. On 2016/12/14 13:39:21, Andrew T Wilson (Slow) wrote: > nit: this object Done. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:254: pref_change_registrar_->Add(prefs::kPublicSessionPermissionsUserChoiceCache, On 2016/12/13 21:51:29, Devlin wrote: > Is this git cl format'd? Done https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:314: return GetProfile()->GetOffTheRecordPrefs(); On 2016/12/13 21:51:29, Devlin wrote: > Why OffTheRecordPrefs()? (add a comment) Done. It's used because the pref must not be persistent across user sessions. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:327: int value; On 2016/12/13 21:51:29, Devlin wrote: > nit: initialize value Done. https://codereview.chromium.org/2552203007/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:330: UserChoiceSet(static_cast<PermissionState>(value)); On 2016/12/14 13:39:20, Andrew T Wilson (Slow) wrote: > Why do we need to call UserChoiceSet here? Why not just set value = NOT_PROMPTED > and fall through to return it? It's just weird to have two possible states for > an unprompted extension to be in - one where the extension is referenced in > kPublicSessionPermissionsUserChoiceCache and one where it isn't. Seems more > consistent to say that the UserChoiceCache only holds actual user choices. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 isandrk@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_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 isandrk@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by isandrk@chromium.org 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 checked by isandrk@chromium.org 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
For some reason I'm getting a failing test on linux_chromium_chromeos_rel_ng now. ExtensionPageCaptureApiTest.PublicSessionRequestAllowed fails at line 73 - ASSERT_FALSE(base::PathExists(delegate.temp_file_)), and I'm unsure why.
Overall, this looks pretty good - some nits and clarifying questions left. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:90: permission_helper_.reset(new PageCapturePermissionHelper( nit: prefer base::MakeUnique<> https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_apitest.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_apitest.cc:73: ASSERT_FALSE(base::PathExists(delegate.temp_file_)); On 2017/01/05 18:07:01, Ivan Šandrk wrote: > For some reason I'm getting a failing test on linux_chromium_chromeos_rel_ng > now. ExtensionPageCaptureApiTest.PublicSessionRequestAllowed fails at line 73 - > ASSERT_FALSE(base::PathExists(delegate.temp_file_)), and I'm unsure why. Why should this be false? The user allowed the request, so the API call should succeed - doesn't that result in a new file being created? https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:21: using extensions::PageCapturePermissionHelper; instead, just put this file in the extensions:: namespace https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:36: : extension_(extension), prefs_(prefs), success_callback_(success_callback), nit: indentation off https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:44: if (user_choice == PermissionState::ALLOWED || I'd prefer we use a switch here https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:55: pref_change_registrar_.reset(new PrefChangeRegistrar); prefer base::MakeUnique https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:73: if (!web_contents) { Can we put this first (before creating the permission set?) https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:102: // Permission was allowed/denied for some other extension, continue sleeping. nit: s/sleeping/waiting. sleep often has a specific meaning. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:103: if (user_choice == PermissionState::SHOWN_PROMPT) { preferably a switch here too https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:123: auto dictionary = prefs_->GetDictionary( here, let's not use auto. It's unclear what the return result is (a DictionaryValue*? std::unique_ptr<DictionaryValue>? DictionaryPrefUpdate? Some other dictionary?) https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:26: base::Callback<void()> success_callback, nit: base::Closure https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:26: base::Callback<void()> success_callback, nit: these can be const & https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:52: void UserChoiceSet(PermissionState value); nit: prefer SetUserChoice and GetUserChoice https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:57: const extensions::Extension* extension_; nit: no extensions:: prefix https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:59: base::Callback<void()> success_callback_; nit: base::Closure
The CQ bit was checked by isandrk@chromium.org 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.
https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:90: permission_helper_.reset(new PageCapturePermissionHelper( On 2017/01/06 20:45:49, Devlin wrote: > nit: prefer base::MakeUnique<> Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_apitest.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_apitest.cc:73: ASSERT_FALSE(base::PathExists(delegate.temp_file_)); On 2017/01/06 20:45:49, Devlin wrote: > On 2017/01/05 18:07:01, Ivan Šandrk wrote: > > For some reason I'm getting a failing test on linux_chromium_chromeos_rel_ng > > now. ExtensionPageCaptureApiTest.PublicSessionRequestAllowed fails at line 73 > - > > ASSERT_FALSE(base::PathExists(delegate.temp_file_)), and I'm unsure why. > > Why should this be false? The user allowed the request, so the API call should > succeed - doesn't that result in a new file being created? This test is basically c/p of the original test just with Public Session stuff added. The temporary file indeed gets created, but it also gets destroyed once the javascript side reads the contents. For some reason unknown to me, this was working in the previous version (before I forked off CrOS only code), but doesn't work now. I double checked, the old version still works. And it's weird that it only fails on one trybot. I'll try to do a deeper investigation. EDIT: Figured it out. I was keeping a reference to PageCaptureSaveAsMHTMLFunction inside permission_helper_ (success/failure callbacks) ergo circular reference. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:21: using extensions::PageCapturePermissionHelper; On 2017/01/06 20:45:49, Devlin wrote: > instead, just put this file in the extensions:: namespace Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:36: : extension_(extension), prefs_(prefs), success_callback_(success_callback), On 2017/01/06 20:45:49, Devlin wrote: > nit: indentation off Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:44: if (user_choice == PermissionState::ALLOWED || On 2017/01/06 20:45:49, Devlin wrote: > I'd prefer we use a switch here Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:55: pref_change_registrar_.reset(new PrefChangeRegistrar); On 2017/01/06 20:45:49, Devlin wrote: > prefer base::MakeUnique Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:73: if (!web_contents) { On 2017/01/06 20:45:49, Devlin wrote: > Can we put this first (before creating the permission set?) Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:102: // Permission was allowed/denied for some other extension, continue sleeping. On 2017/01/06 20:45:49, Devlin wrote: > nit: s/sleeping/waiting. sleep often has a specific meaning. Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:103: if (user_choice == PermissionState::SHOWN_PROMPT) { On 2017/01/06 20:45:49, Devlin wrote: > preferably a switch here too Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:123: auto dictionary = prefs_->GetDictionary( On 2017/01/06 20:45:49, Devlin wrote: > here, let's not use auto. It's unclear what the return result is (a > DictionaryValue*? std::unique_ptr<DictionaryValue>? DictionaryPrefUpdate? > Some other dictionary?) Done. Is this because of the compiler or because of the reader? Or is this for future-proofing the code? Because there's only one PrefService::GetDictionary. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:26: base::Callback<void()> success_callback, On 2017/01/06 20:45:49, Devlin wrote: > nit: base::Closure Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:26: base::Callback<void()> success_callback, On 2017/01/06 20:45:49, Devlin wrote: > nit: these can be const & Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:52: void UserChoiceSet(PermissionState value); On 2017/01/06 20:45:49, Devlin wrote: > nit: prefer SetUserChoice and GetUserChoice Done. They do sound better, was thinking of this myself. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:57: const extensions::Extension* extension_; On 2017/01/06 20:45:49, Devlin wrote: > nit: no extensions:: prefix Done. https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:59: base::Callback<void()> success_callback_; On 2017/01/06 20:45:49, Devlin wrote: > nit: base::Closure Done.
The CQ bit was checked by isandrk@chromium.org 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...
Patchset #10 (id:200001) has been deleted
The CQ bit was checked by isandrk@chromium.org 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...
lgtm https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:123: auto dictionary = prefs_->GetDictionary( On 2017/01/09 13:14:55, Ivan Šandrk wrote: > On 2017/01/06 20:45:49, Devlin wrote: > > here, let's not use auto. It's unclear what the return result is (a > > DictionaryValue*? std::unique_ptr<DictionaryValue>? DictionaryPrefUpdate? > > Some other dictionary?) > > Done. > > Is this because of the compiler or because of the reader? Or is this for > future-proofing the code? Because there's only one PrefService::GetDictionary. TL;DR: This is just for the reader's benefit. auto should pretty much only be used when it's clear from this file/class what the result is. We should only use auto when it's abundantly clear what the return type is and/or when auto itself improves readability. So, good examples for auto: auto foo = base::MakeUnique<Foo>(); // MakeUnique<> takes the class name as the template; impossible for this to return anything but a std::unique_ptr<Foo>. std::vector<Foo> foos; for (const auto& foo : foos) { // Since we know |foos| is a vector of Foo, we know that auto deduces to Foo - and this is arguably much more readable than // for (std::vector<Foo>::const_iterator iter = foos.begin(); iter != foos.end(); ++iter) } In this particular case, while we might know that this returns a "dictionary" of some kind, we don't know exactly what it is from looking at this code (or knowing this class) alone. We can probably assume that it's a base::DictionaryValue from the chromium codebase and the methods called, but even then, we'd actually have no idea if this was a std::unique_ptr<> or a base::DictionaryValue* without looking up the method signature (which creates more work for the reader/reviewer). Similarly, we don't know anything about the const-ness of the result. On the other hand, just using const base::DictionaryValue* is short, sweet, and clear. :) https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_apitest.cc (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_apitest.cc:70: ASSERT_FALSE(delegate.temp_file_.empty()); Might be worth commenting why expect !empty() here and !exists below. https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:116: default: We shouldn't need this default, right? https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. not 2016 anymore
The CQ bit was checked by isandrk@chromium.org 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...
Ok, all done. Thanks for clarifying auto. https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_apitest.cc (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_apitest.cc:70: ASSERT_FALSE(delegate.temp_file_.empty()); On 2017/01/09 18:05:07, Devlin wrote: > Might be worth commenting why expect !empty() here and !exists below. Added comments to the original test. https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.cc:116: default: On 2017/01/09 18:05:07, Devlin wrote: > We shouldn't need this default, right? Right, forgot that the compiler throws an error if we don't handle all possible enumerations. https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_permission_helper.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/09 18:05:07, Devlin wrote: > not 2016 anymore Done.
isandrk@chromium.org changed reviewers: + gab@chromium.org, xiyuan@chromium.org
xiyuan: ptal at c/b/chromeos/preferences.cc gab: ptal at c/b/prefs/pref_service_syncable_util.cc Thanks, Ivan
c/b/chromeos/preferences.cc lgtm
https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1335: // *************** LOCAL STATE *************** Prefs below this line are stored in Local State. Yours is a profile prefs and therefore belongs in an OS_CHROMEOS block above this line (same in header). https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; I don't see existing public_session.* prefs. With "permissions." This will create two levels of JSON nesting for a single item, can an existing root be re-used?
https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1335: // *************** LOCAL STATE *************** On 2017/01/09 20:06:43, gab wrote: > Prefs below this line are stored in Local State. Yours is a profile prefs and > therefore belongs in an OS_CHROMEOS block above this line (same in header). Can you perhaps explain me the difference? I'm quite unsure of the whole placement of my pref. Basically I need the pref to be non-persistent across user sessions, that's why I added that line in pref_service_syncable_util.cc. I hope that's an ok usage. https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/09 20:06:43, gab wrote: > I don't see existing public_session.* prefs. With "permissions." This will > create two levels of JSON nesting for a single item, can an existing root be > re-used? I'll think of something else here. Can I just put everything in one level, ie. public_session_permissions_user_choice_cache?
https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1335: // *************** LOCAL STATE *************** On 2017/01/09 20:25:54, Ivan Šandrk wrote: > On 2017/01/09 20:06:43, gab wrote: > > Prefs below this line are stored in Local State. Yours is a profile prefs and > > therefore belongs in an OS_CHROMEOS block above this line (same in header). > > Can you perhaps explain me the difference? I'm quite unsure of the whole > placement of my pref. > > Basically I need the pref to be non-persistent across user sessions, that's why > I added that line in pref_service_syncable_util.cc. I hope that's an ok usage. Local State is for browser-wide prefs. Preferences are per-profile prefs. Either way, prefs are specifically meant to be persisted to disk. If you don't want persistence, don't use prefs (store it in memory in a member of a class of yours). https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/09 20:25:54, Ivan Šandrk wrote: > On 2017/01/09 20:06:43, gab wrote: > > I don't see existing public_session.* prefs. With "permissions." This will > > create two levels of JSON nesting for a single item, can an existing root be > > re-used? > > I'll think of something else here. Can I just put everything in one level, ie. > public_session_permissions_user_choice_cache? Top-level isn't done often either, maybe extensions.?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/09 20:47:01, gab wrote: > On 2017/01/09 20:25:54, Ivan Šandrk wrote: > > On 2017/01/09 20:06:43, gab wrote: > > > I don't see existing public_session.* prefs. With "permissions." This will > > > create two levels of JSON nesting for a single item, can an existing root be > > > re-used? > > > > I'll think of something else here. Can I just put everything in one level, ie. > > public_session_permissions_user_choice_cache? > > Top-level isn't done often either, maybe extensions.? You could store this in ExtensionPrefs, which is the extensions layer on top of the profile prefs and maps to extensions.*.
The CQ bit was checked by isandrk@chromium.org 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...
gab: ptal! https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1335: // *************** LOCAL STATE *************** On 2017/01/09 20:47:01, gab wrote: > On 2017/01/09 20:25:54, Ivan Šandrk wrote: > > On 2017/01/09 20:06:43, gab wrote: > > > Prefs below this line are stored in Local State. Yours is a profile prefs > and > > > therefore belongs in an OS_CHROMEOS block above this line (same in header). > > > > Can you perhaps explain me the difference? I'm quite unsure of the whole > > placement of my pref. > > > > Basically I need the pref to be non-persistent across user sessions, that's > why > > I added that line in pref_service_syncable_util.cc. I hope that's an ok usage. > > Local State is for browser-wide prefs. > Preferences are per-profile prefs. > > Either way, prefs are specifically meant to be persisted to disk. If you don't > want persistence, don't use prefs (store it in memory in a member of a class of > yours). Thanks for the explanation gab. I learned today that everything is wiped clean in Public Sessions, so no prefs will be persisted which is perfect. https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/09 20:47:01, gab wrote: > On 2017/01/09 20:25:54, Ivan Šandrk wrote: > > On 2017/01/09 20:06:43, gab wrote: > > > I don't see existing public_session.* prefs. With "permissions." This will > > > create two levels of JSON nesting for a single item, can an existing root be > > > re-used? > > > > I'll think of something else here. Can I just put everything in one level, ie. > > public_session_permissions_user_choice_cache? > > Top-level isn't done often either, maybe extensions.? I hope this one sounds ok. https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/09 21:22:41, Devlin wrote: > On 2017/01/09 20:47:01, gab wrote: > > On 2017/01/09 20:25:54, Ivan Šandrk wrote: > > > On 2017/01/09 20:06:43, gab wrote: > > > > I don't see existing public_session.* prefs. With "permissions." This will > > > > create two levels of JSON nesting for a single item, can an existing root > be > > > > re-used? > > > > > > I'll think of something else here. Can I just put everything in one level, > ie. > > > public_session_permissions_user_choice_cache? > > > > Top-level isn't done often either, maybe extensions.? > > You could store this in ExtensionPrefs, which is the extensions layer on top of > the profile prefs and maps to extensions.*. Thanks for the suggestion devlin. I'll keep it this way unless that's better for some reason I'm unaware of. I'm also planning on doing a bigger refactoring anyway, so it shouldn't matter too much.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
prefs lgtm w/ nit https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/260001/chrome/common/pref_nam... chrome/common/pref_names.cc:1910: "public_session.permissions.user_choice_cache"; On 2017/01/11 17:41:50, Ivan Šandrk wrote: > On 2017/01/09 21:22:41, Devlin wrote: > > On 2017/01/09 20:47:01, gab wrote: > > > On 2017/01/09 20:25:54, Ivan Šandrk wrote: > > > > On 2017/01/09 20:06:43, gab wrote: > > > > > I don't see existing public_session.* prefs. With "permissions." This > will > > > > > create two levels of JSON nesting for a single item, can an existing > root > > be > > > > > re-used? > > > > > > > > I'll think of something else here. Can I just put everything in one level, > > ie. > > > > public_session_permissions_user_choice_cache? > > > > > > Top-level isn't done often either, maybe extensions.? > > > > You could store this in ExtensionPrefs, which is the extensions layer on top > of > > the profile prefs and maps to extensions.*. > > Thanks for the suggestion devlin. I'll keep it this way unless that's better for > some reason I'm unaware of. If that's where all extensions.* prefs are then that's probably better but I'll defer to devlin on that one since this pref_names.cc already has a few extensions.* prefs. > > I'm also planning on doing a bigger refactoring anyway, so it shouldn't matter > too much. https://codereview.chromium.org/2552203007/diff/280001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2552203007/diff/280001/chrome/common/pref_nam... chrome/common/pref_names.cc:941: // Dictionary saving user choices regarding granted permissions to nit: Empty line above (others are together because they're related)
devlin can you chime in with your opinion?
On 2017/01/11 20:06:11, Ivan Šandrk wrote: > devlin can you chime in with your opinion? Sorry for the delay. I think it would make sense to use ExtensionPrefs for this instead. The extensions. namespace in prefs is pretty much entirely handled by that object (I don't know of any that are extensions. and aren't there?), so it makes sense for consistency. The conversion should be pretty easy - it's pretty much just ExtensionPrefs::ReadPrefAsInteger().
The CQ bit was checked by isandrk@chromium.org 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...
I moved the prefs to ExtensionPrefs. Devlin please take another look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/18 18:28:23, Ivan Šandrk wrote: > I moved the prefs to ExtensionPrefs. Devlin please take another look. I'm a bit confused - when we talked offline, didn't we agree that we didn't need a pref for this, since it will be removed soon and it wasn't worth adding it and then adding the requisite cleanup code?
The CQ bit was checked by isandrk@chromium.org 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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, no build URL)
Patchset #15 (id:320001) has been deleted
The CQ bit was checked by isandrk@chromium.org 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Heya Devlin, I made the adjustments. I'm getting some buildbot failures, I'll check them tomorrow, too tired at this point. If the code seems complex, it's because I want it to also support making changes to public_session_tab_capture_access_handler.cc/h and public_session_media_access_handler.cc/h
https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:17: CR_DEFINE_STATIC_LOCAL(PublicSessionPermissionHelper, instance, ()); usually for something like this, we prefer LazyInstance https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:25: namespace permission_helper { we probably don't need a namespace just for these classes. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:28: enum PermissionState { It looks like this might not be used at all? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:34: using PermissionHelperSet = std::set<APIPermission::ID>; We have an APIPermissionSet; is there a reason we can't use that here? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:38: static PublicSessionPermissionHelper& Instance(); style forbids non-const refs being returned. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:57: PermissionState GetUserChoice(ExtensionId extension_id, Needed? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:66: failure_callback.Run(); It's a little weird that a request can request multiple permissions and have some granted, and some not. When can this happen? Do we anticipate calling this with multiple permissions? If not, removing this could simplify some of the other code. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:89: prompt->ShowDialog( Here, it looks like we're anticipating showing multiple of these prompts and being able to queue them up. But right now, do we just spam-open them all at once if an extension requests multiple permissions in separate requests? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:22: struct RequestCallback { Can this be internal to the class (i.e., a private struct)? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:34: class PublicSessionPermissionHelperImpl { It looks to me like this class can probably be combined with the other. Since this predominantly just contains a bit of data, we could probably just replace the entry of PublicSessionPermissionHelperImpl in the map with a struct containing that data. That would cut down on the duplication of code between the two files. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:60: void SetUserChoice(APIPermission::ID permission_id, PermissionState value); Never defined
The CQ bit was checked by isandrk@chromium.org 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...
https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:17: CR_DEFINE_STATIC_LOCAL(PublicSessionPermissionHelper, instance, ()); On 2017/01/23 22:59:48, Devlin (catching up) wrote: > usually for something like this, we prefer LazyInstance Why? https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:25: namespace permission_helper { On 2017/01/23 22:59:48, Devlin (catching up) wrote: > we probably don't need a namespace just for these classes. Done. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:28: enum PermissionState { On 2017/01/23 22:59:48, Devlin (catching up) wrote: > It looks like this might not be used at all? Used in public_session_media_access_handler.cc https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:34: using PermissionHelperSet = std::set<APIPermission::ID>; On 2017/01/23 22:59:48, Devlin (catching up) wrote: > We have an APIPermissionSet; is there a reason we can't use that here? ContainsAnyID accepts a set<APIPermission::ID> but not a APIPermissionSet. PermissionIDSet would be really good if it could work with ContainsAnyID and if it had a clean constructor from an initializer list (or something like this). Are you fine with me making modifications to PermissionIDSet? :-) https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:38: static PublicSessionPermissionHelper& Instance(); On 2017/01/23 22:59:48, Devlin (catching up) wrote: > style forbids non-const refs being returned. Can you link this? I wasn't able to find it in the style. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:57: PermissionState GetUserChoice(ExtensionId extension_id, On 2017/01/23 22:59:48, Devlin (catching up) wrote: > Needed? I'll upload also the changes to the other files so it becomes more apparent why it's needed. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:66: failure_callback.Run(); On 2017/01/23 22:59:48, Devlin (catching up) wrote: > It's a little weird that a request can request multiple permissions and have > some granted, and some not. When can this happen? Do we anticipate calling > this with multiple permissions? If not, removing this could simplify some of > the other code. PublicSessionMediaAccessHandler may ask up to two permissions at the same time. Request #1 - Audio -> Prompt is shown only for audio Request #2 - Audio+Video -> Prompt is shown only for video (because we already prompted audio) User allows #1, but not #2. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:89: prompt->ShowDialog( On 2017/01/23 22:59:48, Devlin (catching up) wrote: > Here, it looks like we're anticipating showing multiple of these prompts and > being able to queue them up. But right now, do we just spam-open them all at > once if an extension requests multiple permissions in separate requests? Yes, spam-open. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:22: struct RequestCallback { On 2017/01/23 22:59:48, Devlin (catching up) wrote: > Can this be internal to the class (i.e., a private struct)? Done. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:34: class PublicSessionPermissionHelperImpl { On 2017/01/23 22:59:48, Devlin (catching up) wrote: > It looks to me like this class can probably be combined with the other. Since > this predominantly just contains a bit of data, we could probably just replace > the entry of PublicSessionPermissionHelperImpl in the map with a struct > containing that data. That would cut down on the duplication of code between > the two files. In one of the earlier iterations I had it all in one class but it seemed too messy to me. Every time I had to access one of the member variables, I had to also put in the extension_id. This way it seems more elegant to me, logical separation at the extension level. I can tho merge the files together, and have the impl be private to public_session_permission_helper.cc. And maybe pick a better name than just _impl (sometimes I'm not creative with names). https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:60: void SetUserChoice(APIPermission::ID permission_id, PermissionState value); On 2017/01/23 22:59:48, Devlin (catching up) wrote: > Never defined Right, had a cleanup and missed this. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Ideally, I'd also like to see some more unittests for the public session prompt, since we're adding quite a bit of logic (e.g. chaining prompts) that we didn't have before, but I'll let Drew decide if it's necessary, since it's only valid in public session code. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:17: CR_DEFINE_STATIC_LOCAL(PublicSessionPermissionHelper, instance, ()); On 2017/01/24 19:57:22, Ivan Šandrk wrote: > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > usually for something like this, we prefer LazyInstance > > Why? Mostly just because it is the canonical, known-to-be-safe, easy way to create a singleton instance of an object. That's not to say it's the only way, or is concretely better than alternatives, but we prefer using the common patterns we have so that readers instantly know how it works and how to use it. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:28: enum PermissionState { On 2017/01/24 19:57:22, Ivan Šandrk wrote: > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > It looks like this might not be used at all? > > Used in public_session_media_access_handler.cc Ah, that file didn't used to exist. :) https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:34: using PermissionHelperSet = std::set<APIPermission::ID>; On 2017/01/24 19:57:22, Ivan Šandrk wrote: > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > We have an APIPermissionSet; is there a reason we can't use that here? > > ContainsAnyID accepts a set<APIPermission::ID> but not a APIPermissionSet. > > PermissionIDSet would be really good if it could work with ContainsAnyID and if > it had a clean constructor from an initializer list (or something like this). > Are you fine with me making modifications to PermissionIDSet? :-) Grr... these weird sets are annoying (totally not related to this CL). I don't want to ask you to make yet-more-changes, since I've already been pretty picky here. ;) Let's move the typedef into private: part of the class so it doesn't pollute the overall namespace, and put a note of why we have it. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:38: static PublicSessionPermissionHelper& Instance(); On 2017/01/24 19:57:22, Ivan Šandrk wrote: > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > style forbids non-const refs being returned. > > Can you link this? I wasn't able to find it in the style. Hmm... I've found https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which requires it for parameters. I've also found numerous places in the codebase where it references that we don't allow them. I wonder if a rule changed recently, or if we're supposed to infer that reference arguments apply to return values? In either case, since I've failed to come up with a link, we can ignore this one (I have no personal objections to it). https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:77: if (unprompted_permissions.empty()) return; return; should be on a new line https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:130: callback != callbacks_.end(); ) { nit: no space after ; https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:34: class PublicSessionPermissionHelperImpl { On 2017/01/24 19:57:22, Ivan Šandrk wrote: > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > It looks to me like this class can probably be combined with the other. Since > > this predominantly just contains a bit of data, we could probably just replace > > the entry of PublicSessionPermissionHelperImpl in the map with a struct > > containing that data. That would cut down on the duplication of code between > > the two files. > > In one of the earlier iterations I had it all in one class but it seemed too > messy to me. Every time I had to access one of the member variables, I had to > also put in the extension_id. This way it seems more elegant to me, logical > separation at the extension level. I can tho merge the files together, and have > the impl be private to public_session_permission_helper.cc. And maybe pick a > better name than just _impl (sometimes I'm not creative with names). To elaborate a little here. Currently, PublicSessionPermissionHelper just contains a map<ExtensionId, HelperImpl>. What if we instead do something like: // .h class Helper { public: static HandlePermissionRequest(); static GetUserChoice(); private: HandlePermissionRequest(); GetUserChoice(); prompt_map_; prompted_set_; .... }; // .cc namespace { LazyInstance<std::map<ExtensionId, Helper>> helpers; } // static Helper::HandlePermissionRequest() { (*helpers.Get())[extension->id()]->HandlePermissionRequest(); } // static Helper::GetUserChoice() { (*helpers.Get())[extension->id()]->GetUserChoice(); } Helper::HandlePermissionChoice() { // impl } Helper::GetUserChoice() { // impl } That way, the separation and class logic remains the same, but we coalesce the two files into one. WDYT? https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:26: CHECK(extension); I know most places pass a const Extension*, but in places we know it's non-null, we may as well just pass a const & and have the compiler enforce this (optionally (D)CHECKing at the pass site). https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:26: enum PermissionState { Let's put this in the class. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:48: void HandlePermissionRequest(const Extension* extension, nit: it seems like we can have these two functions be static, and avoid surfacing the instance (which seems to be an implementation detail). https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:32: permission_list(other.permission_list) {} can we not just use = default? https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:64: if (unresolved_permissions.empty()) { If an item requests A and B, and the user has already rejected B, do we need to prompt for A? Or would it make sense to just fire the failure callback immediately, since there's no way it could succeed (and potentially no way to tell if having just permission A would be useful)? https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:98: prompt_map_[first_unprompted_permission] = std::move(prompt); storing by arbitrary permission is a little weird to me, since we prompt for multiple. Why not just store a set<prompt>? https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:138: callbacks_.erase(callback); don't we need a callback = callbacks_.erase() or something like that here? https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:21: class PublicSessionPermissionHelperImpl { This class needs class-level and function-level comments https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:58: base::Unretained(this), web_contents, request, callback, extension); document why this Unretained() is safe https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:58: base::Unretained(this), web_contents, request, callback, extension); document why this Unretained() is safe
The CQ bit was checked by isandrk@chromium.org 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...
Devlin take another look please! https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:17: CR_DEFINE_STATIC_LOCAL(PublicSessionPermissionHelper, instance, ()); On 2017/01/25 16:00:31, Devlin (catching up) wrote: > On 2017/01/24 19:57:22, Ivan Šandrk wrote: > > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > > usually for something like this, we prefer LazyInstance > > > > Why? > > Mostly just because it is the canonical, known-to-be-safe, easy way to create a > singleton instance of an object. That's not to say it's the only way, or is > concretely better than alternatives, but we prefer using the common patterns we > have so that readers instantly know how it works and how to use it. I meant why as in why LazyInstance over CR_DEFINE_STATIC_LOCAL :) CR is obviously also used across the codebase, and for the same purpose of creating a singleton (eg. https://goo.gl/Nrnlll). Why have two things to do one thing in the codebase anyway? I'd say the CR is also a canonical way to create a singleton (Meyers singleton) - https://yaqs.googleplex.com/eng/q/5555555103932416. Maybe even simpler than LazyInstance now that we have C++11 (in terms of implementation), because as far as I can tell LazyInstance was used over CR because it was thread safe. Anyway since you're the owner you make the call :) https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:34: using PermissionHelperSet = std::set<APIPermission::ID>; On 2017/01/25 16:00:31, Devlin (catching up) wrote: > On 2017/01/24 19:57:22, Ivan Šandrk wrote: > > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > > We have an APIPermissionSet; is there a reason we can't use that here? > > > > ContainsAnyID accepts a set<APIPermission::ID> but not a APIPermissionSet. > > > > PermissionIDSet would be really good if it could work with ContainsAnyID and > if > > it had a clean constructor from an initializer list (or something like this). > > Are you fine with me making modifications to PermissionIDSet? :-) > > Grr... these weird sets are annoying (totally not related to this CL). I don't > want to ask you to make yet-more-changes, since I've already been pretty picky > here. ;) > > Let's move the typedef into private: part of the class so it doesn't pollute the > overall namespace, and put a note of why we have it. Done. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:38: static PublicSessionPermissionHelper& Instance(); On 2017/01/25 16:00:31, Devlin (catching up) wrote: > On 2017/01/24 19:57:22, Ivan Šandrk wrote: > > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > > style forbids non-const refs being returned. > > > > Can you link this? I wasn't able to find it in the style. > > Hmm... I've found > https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which > requires it for parameters. I've also found numerous places in the codebase > where it references that we don't allow them. I wonder if a rule changed > recently, or if we're supposed to infer that reference arguments apply to return > values? > > In either case, since I've failed to come up with a link, we can ignore this one > (I have no personal objections to it). I think people have misunderstood that rule to mean "never use non-const refs when functions are involved" :) Titus Winters talks about it here https://goto.google.com/totw/87: So, if non-const references are only forbidden as interface parameters, are there still good uses for them? Of course. You may sometimes want to return a non-const reference from a function. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:77: if (unprompted_permissions.empty()) return; On 2017/01/25 16:00:31, Devlin (catching up) wrote: > return; should be on a new line Style guide allows this https://google.github.io/styleguide/cppguide.html#Conditionals: (plus it's used in other places like this) Short conditional statements may be written on one line if this enhances readability. You may use this only when the line is brief and the statement does not use the else clause. But then you and I may not agree on the readability part :) https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:130: callback != callbacks_.end(); ) { On 2017/01/25 16:00:31, Devlin (catching up) wrote: > nit: no space after ; https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace - Loops and Conditionals: // For loops always have a space after the semicolon. They may have a space // before the semicolon, but this is rare. Altho a quick code search shows both cases, with no space coming up more often. I personally find it more readable this way, but then again you're the owner :) https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:34: class PublicSessionPermissionHelperImpl { On 2017/01/25 16:00:31, Devlin (catching up) wrote: > On 2017/01/24 19:57:22, Ivan Šandrk wrote: > > On 2017/01/23 22:59:48, Devlin (catching up) wrote: > > > It looks to me like this class can probably be combined with the other. > Since > > > this predominantly just contains a bit of data, we could probably just > replace > > > the entry of PublicSessionPermissionHelperImpl in the map with a struct > > > containing that data. That would cut down on the duplication of code > between > > > the two files. > > > > In one of the earlier iterations I had it all in one class but it seemed too > > messy to me. Every time I had to access one of the member variables, I had to > > also put in the extension_id. This way it seems more elegant to me, logical > > separation at the extension level. I can tho merge the files together, and > have > > the impl be private to public_session_permission_helper.cc. And maybe pick a > > better name than just _impl (sometimes I'm not creative with names). > > To elaborate a little here. > > Currently, PublicSessionPermissionHelper just contains a map<ExtensionId, > HelperImpl>. What if we instead do something like: > > // .h > > class Helper { > public: > static HandlePermissionRequest(); > static GetUserChoice(); > > private: > HandlePermissionRequest(); > GetUserChoice(); > > prompt_map_; > prompted_set_; > .... > }; > > // .cc > > namespace { > LazyInstance<std::map<ExtensionId, Helper>> helpers; > } > > // static > Helper::HandlePermissionRequest() { > (*helpers.Get())[extension->id()]->HandlePermissionRequest(); > } > > // static > Helper::GetUserChoice() { > (*helpers.Get())[extension->id()]->GetUserChoice(); > } > > Helper::HandlePermissionChoice() { > // impl > } > > Helper::GetUserChoice() { > // impl > } > > That way, the separation and class logic remains the same, but we coalesce the > two files into one. WDYT? Ah I see, excellent idea! Done! https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:26: CHECK(extension); On 2017/01/25 16:00:32, Devlin (catching up) wrote: > I know most places pass a const Extension*, but in places we know it's non-null, > we may as well just pass a const & and have the compiler enforce this > (optionally (D)CHECKing at the pass site). Good suggestion. Done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:26: enum PermissionState { On 2017/01/25 16:00:32, Devlin (catching up) wrote: > Let's put this in the class. Removed completely. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:48: void HandlePermissionRequest(const Extension* extension, On 2017/01/25 16:00:32, Devlin (catching up) wrote: > nit: it seems like we can have these two functions be static, and avoid > surfacing the instance (which seems to be an implementation detail). Done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:32: permission_list(other.permission_list) {} On 2017/01/25 16:00:32, Devlin (catching up) wrote: > can we not just use = default? Done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:64: if (unresolved_permissions.empty()) { On 2017/01/25 16:00:32, Devlin (catching up) wrote: > If an item requests A and B, and the user has already rejected B, do we need to > prompt for A? Or would it make sense to just fire the failure callback > immediately, since there's no way it could succeed (and potentially no way to > tell if having just permission A would be useful)? PublicSessionMediaAccessHandler can pass to the App only one of the requested A/V streams and it's up to it to decide what it wants to do. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:98: prompt_map_[first_unprompted_permission] = std::move(prompt); On 2017/01/25 16:00:32, Devlin (catching up) wrote: > storing by arbitrary permission is a little weird to me, since we prompt for > multiple. Why not just store a set<prompt>? Done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:138: callbacks_.erase(callback); On 2017/01/25 16:00:32, Devlin (catching up) wrote: > don't we need a callback = callbacks_.erase() or something like that here? Indeed, done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.h:21: class PublicSessionPermissionHelperImpl { On 2017/01/25 16:00:32, Devlin (catching up) wrote: > This class needs class-level and function-level comments Done. https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.cc (right): https://codereview.chromium.org/2552203007/diff/360001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.cc:58: base::Unretained(this), web_contents, request, callback, extension); On 2017/01/25 16:00:32, Devlin (catching up) wrote: > document why this Unretained() is safe Done.
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 isandrk@chromium.org 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.
Nice! This looks much better; thanks for bearing with me. :) In a perfect world, I think we'd have separate unittests for the permission helper (exercising behavior like chaining requests, etc) - if it's not too terrible, it would be great to add some. Up to Drew (since it's public session code) if it's a blocker or not. https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:77: if (unprompted_permissions.empty()) return; On 2017/01/26 18:53:21, Ivan Šandrk wrote: > On 2017/01/25 16:00:31, Devlin (catching up) wrote: > > return; should be on a new line > > Style guide allows this > https://google.github.io/styleguide/cppguide.html#Conditionals: (plus it's used > in other places like this) > > Short conditional statements may be written on one line if this enhances > readability. You may use this only when the line is brief and the statement does > not use the else clause. > > But then you and I may not agree on the readability part :) In this case, I'm just going to lean on the fact that it's very rare to do this in chromium code, and ask to be consistent (since there's no functional difference). https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:105: base::Unretained(this), &prompt, nit: add a comment explaining why this Unretained is safe. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:118: if (allowed_permission_set_.ContainsID(permission_id)) if (foo) return true; return false; -> return foo; https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:125: PermissionIDSet unprompted_permissions, const PermissionIDSet& https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:128: prompts_.erase(*prompt); for paranoia, can we do something like: size_t erased = prompts_.erase(*prompt); [D]CHECK_NE(0u, erased); https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:140: if (prompted_permission_set_.ContainsAnyID(callback->permission_list)) { nit: maybe add a comment like: // The request is still waiting on one or more permissions to be resolved; wait until all permissions are ready to call the callback. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:40: // ContainsAnyID function accepts only std::set<APIPermission::ID> argument, nitty nit: PermissionIDSet::ContainsAnyID() https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:93: base::Unretained(this))); explain why the unretained()s are safe. Alternatively, ExtensionFunctions are ref-counted, so you could just pass |this| directly.
The CQ bit was checked by isandrk@chromium.org 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...
Devlin ptal again! https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc (right): https://codereview.chromium.org/2552203007/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_impl.cc:77: if (unprompted_permissions.empty()) return; On 2017/01/30 17:04:16, Devlin wrote: > On 2017/01/26 18:53:21, Ivan Šandrk wrote: > > On 2017/01/25 16:00:31, Devlin (catching up) wrote: > > > return; should be on a new line > > > > Style guide allows this > > https://google.github.io/styleguide/cppguide.html#Conditionals: (plus it's > used > > in other places like this) > > > > Short conditional statements may be written on one line if this enhances > > readability. You may use this only when the line is brief and the statement > does > > not use the else clause. > > > > But then you and I may not agree on the readability part :) > > In this case, I'm just going to lean on the fact that it's very rare to do this > in chromium code, and ask to be consistent (since there's no functional > difference). Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:105: base::Unretained(this), &prompt, On 2017/01/30 17:04:17, Devlin wrote: > nit: add a comment explaining why this Unretained is safe. Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:118: if (allowed_permission_set_.ContainsID(permission_id)) On 2017/01/30 17:04:17, Devlin wrote: > if (foo) > return true; > return false; > > -> > > return foo; Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:125: PermissionIDSet unprompted_permissions, On 2017/01/30 17:04:17, Devlin wrote: > const PermissionIDSet& Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:128: prompts_.erase(*prompt); On 2017/01/30 17:04:16, Devlin wrote: > for paranoia, can we do something like: > size_t erased = prompts_.erase(*prompt); > [D]CHECK_NE(0u, erased); Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:140: if (prompted_permission_set_.ContainsAnyID(callback->permission_list)) { On 2017/01/30 17:04:17, Devlin wrote: > nit: maybe add a comment like: > // The request is still waiting on one or more permissions to be resolved; wait > until all permissions are ready to call the callback. Makes sense here. Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:40: // ContainsAnyID function accepts only std::set<APIPermission::ID> argument, On 2017/01/30 17:04:17, Devlin wrote: > nitty nit: PermissionIDSet::ContainsAnyID() Done. https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.cc (right): https://codereview.chromium.org/2552203007/diff/400001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.cc:93: base::Unretained(this))); On 2017/01/30 17:04:17, Devlin wrote: > explain why the unretained()s are safe. > > Alternatively, ExtensionFunctions are ref-counted, so you could just pass |this| > directly. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks like the tests are failing?
The CQ bit was checked by isandrk@chromium.org 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...
Devlin: Should be good now, I bet you saw this one coming. Do you want me to keep the DCHECK? And are you happy with this prompt_id solution?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/31 15:23:48, Ivan Šandrk wrote:
> Devlin: Should be good now, I bet you saw this one coming. Do you want me to
> keep the DCHECK? And are you happy with this prompt_id solution?
I don't really like having an extra id used for this, and my personal preference
would be to keep using a set and substitute erase(Prompt*) with something like:
auto iter = std::find_if(begin, end, [prompt](check) { return check.get() ==
prompt; })
erase(iter)
but that's largely a matter of preference, so I won't block you on it.
I also commented a few times that unittests on the permission helper would be
nice, but up to Drew if they're necessary. Not sure if you've seen those
comments, since you didn't respond one way or the other?
On 2017/01/30 17:04:17, Devlin wrote:
> In a perfect world, I think we'd have separate unittests for the permission
> helper (exercising behavior like chaining requests, etc) - if it's not too
> terrible, it would be great to add some. Up to Drew (since it's public
session
> code) if it's a blocker or not.
All that said, this lgtm. Thanks again for your hard work! :)
https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:62: PermissionHelperSet requested_permissions, Why does PermissionHelperSet exist? Why don't we just pass in PermissionIDSet instead? https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:81: if (unresolved_permissions.empty()) { kind of weird, that even if we know the permissions request will fail (since there is at least one denied permission) we still prompt the user for unresolved permissions anyway. Not a huge deal, but kind of a weird API wrinkle based on how you've defined the API (to have a failure callback that is invoked even though some permissions have been granted). https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:93: if (unprompted_permissions.empty()) You should comment the various steps in this flow, because it's a bit subtle. For example, I think in this case what's happening is you have one or more permissions prompt on-screen right now, so no need to do anything further. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:132: bool allowed = prompt_result == ExtensionInstallPrompt::Result::ACCEPTED; this is fine, but maybe make it clear that this is a shorthand for (prompt_result == ACCEPTED) by marking it as "const bool allowed". https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:39: private: Why is this private section up here at the top? https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:42: // used in this class. nit: this naming is weird - PermissionHelperSet is not a set of PermissionHelpers, it's a set of permissions. Maybe you could call it something like "APIPermissionSet" or something? Generally a "FooSet" is a set of Foo objects. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:48: // failure_callback depending on all permissions being allowed. So failure_callback is invoked if *any* permission is denied? Maybe make that clear either here or in the last paragraph of this documentation. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:52: // enqueued to be called when the permission(s) is resolved. Are you saying that the user will be prompted multiple times? Or just that you will queue up the success/failure callback and invoke it after the current prompt is answered? Can you clarify the description? https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:66: // called inside success/failure callbacks. Why is it only useful inside a callback? Why don't we pass in the permissions granted to the extension to the callback, and remove this funcion? https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:70: PublicSessionPermissionHelper(); Why do we have public constructors when all APIs are static? Feels like you should just define 1-2 static functions in a namespace, and move this entire class into a .cc file (no need to expose in a header file). The fact that you implement the static method with a class instance is really an implementation detail that should be hidden. https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:67: There's some subtle logic in this implementation - dealing with overlapping permission requests, etc. How is this covered by tests (for example, if one caller asks for permissions A & B, and a second caller asks for permission B while the prompt for A&B is still on-screen, what test covers that case? https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:153: callback = callbacks_.erase(callback); What happens if in the callback, the callback invokes HandlePermissionRequestImpl() again? Won't that modify the set and change your iterator/yield some kind of exception/crash?
The CQ bit was checked by isandrk@chromium.org 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...
Devlin: decided to use std::find_if as you suggested. Drew: I've addressed most comments, I need still to write a test and I need to try using PermissionIDSet instead of PermissionHelperSet. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:62: PermissionHelperSet requested_permissions, On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > Why does PermissionHelperSet exist? Why don't we just pass in PermissionIDSet > instead? PermissionIDSet::ContainsAnyID() function accepts only std::set<APIPermission::ID> argument. Plus the PermissionIDSet doesn't have a nice constructor that can take an argument like {Permission_A} or {Permission_B, Permission_C}. I may make some modifications to PermissionIDSet so the ContainsAnyID function supports PermissionIDSet, and the constructor behaves more nicely. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:81: if (unresolved_permissions.empty()) { On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > kind of weird, that even if we know the permissions request will fail (since > there is at least one denied permission) we still prompt the user for unresolved > permissions anyway. Not a huge deal, but kind of a weird API wrinkle based on > how you've defined the API (to have a failure callback that is invoked even > though some permissions have been granted). Scraped completely the failure/success callbacks, using only one callback now. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:93: if (unprompted_permissions.empty()) On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > You should comment the various steps in this flow, because it's a bit subtle. > For example, I think in this case what's happening is you have one or more > permissions prompt on-screen right now, so no need to do anything further. Done. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:132: bool allowed = prompt_result == ExtensionInstallPrompt::Result::ACCEPTED; On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > this is fine, but maybe make it clear that this is a shorthand for > (prompt_result == ACCEPTED) by marking it as "const bool allowed". Done. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:39: private: On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > Why is this private section up here at the top? I wanted to define PermissionHelperSet as private to the class, and had to have it before HandlePermissionRequest because it uses it as an argument type. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:42: // used in this class. On 2017/01/31 16:22:25, Andrew T Wilson (Slow) wrote: > nit: this naming is weird - PermissionHelperSet is not a set of > PermissionHelpers, it's a set of permissions. Maybe you could call it something > like "APIPermissionSet" or something? Generally a "FooSet" is a set of Foo > objects. APIPermissionSet already exists. So does PermissionIDSet (best name), and PermissionSet. I wasn't too happy with any of the existing Permission collections, that is why I went with this. How does ExtensionPermissionSet sound? I could use PermissionIDSet tho, but I would need to make some modifications to it and that depends on whether Devlin will be fine with them. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:48: // failure_callback depending on all permissions being allowed. On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > So failure_callback is invoked if *any* permission is denied? Maybe make that > clear either here or in the last paragraph of this documentation. Done. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:52: // enqueued to be called when the permission(s) is resolved. On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > Are you saying that the user will be prompted multiple times? Or just that you > will queue up the success/failure callback and invoke it after the current > prompt is answered? Can you clarify the description? User will be prompted only once per permission; the callback will be queued up to be invoked when the current prompt is answered. Done. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:66: // called inside success/failure callbacks. On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > Why is it only useful inside a callback? Why don't we pass in the permissions > granted to the extension to the callback, and remove this funcion? The function should be called only after the permission has been resolved. You are right, I should remove it. https://codereview.chromium.org/2552203007/diff/420001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:70: PublicSessionPermissionHelper(); On 2017/01/31 16:22:24, Andrew T Wilson (Slow) wrote: > Why do we have public constructors when all APIs are static? Feels like you > should just define 1-2 static functions in a namespace, and move this entire > class into a .cc file (no need to expose in a header file). The fact that you > implement the static method with a class instance is really an implementation > detail that should be hidden. Good point. Done. https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:67: On 2017/01/31 16:22:25, Andrew T Wilson (Slow) wrote: > There's some subtle logic in this implementation - dealing with overlapping > permission requests, etc. How is this covered by tests (for example, if one > caller asks for permissions A & B, and a second caller asks for permission B > while the prompt for A&B is still on-screen, what test covers that case? Currently none, I'll write one. https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:153: callback = callbacks_.erase(callback); On 2017/01/31 16:22:25, Andrew T Wilson (Slow) wrote: > What happens if in the callback, the callback invokes > HandlePermissionRequestImpl() again? Won't that modify the set and change your > iterator/yield some kind of exception/crash? Very good point, it can possibly result in a crash. I need to add explicit CHECKs to guard against this and mention in the comments that calling HandlePermissionRequest from inside the callback is not allowed.
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 isandrk@chromium.org 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.
https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:153: callback = callbacks_.erase(callback); On 2017/02/01 18:11:29, Ivan Šandrk wrote: > On 2017/01/31 16:22:25, Andrew T Wilson (Slow) wrote: > > What happens if in the callback, the callback invokes > > HandlePermissionRequestImpl() again? Won't that modify the set and change your > > iterator/yield some kind of exception/crash? > > Very good point, it can possibly result in a crash. I need to add explicit > CHECKs to guard against this and mention in the comments that calling > HandlePermissionRequest from inside the callback is not allowed. BTW, the canonical way to do this is not to add documentation/CHECKs() but to instead do something like: 1) Walk list of callbacks. 2) If a callback needs to get invoked, remove it from the list and add it to a new temporary list. 3) Once you are done, walk the temporary list and invoke all of the callbacks. That way, if a given callback does something that changes the main list, it doesn't cause a crash because we aren't walking that list - we're walking our own local list of "callbacks to be invoked". I haven't reviewed your change yet, so maybe that's what you've done, but that would be my recommendation here.
The CQ bit was checked by isandrk@chromium.org 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...
Drew: Much better approach, thanks. Done. Devlin: ptal at extensions/common/permissions/api_permission_set.*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=672093 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* ========== to ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=689478 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* ==========
LGTM, pending one question about test coverage. https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:107: return; Since the body of this if-statement is multi-line due to the comment, put braces around it all. Also, is this code path (the one where a prompt is issued when one is already in progress) currently covered by tests? I didn't see a test that exercises this. https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:155: callbacks_to_invoke.push_back(std::move(*callback)); Explain why you're doing this (i.e. because callbacks_ can be changed by the callback) https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:51: void ResolvePermissionRequest(PermissionIDSet allowed_permissions); This is fine as is - but would declaring this as const PermissionIDSet& provide any efficiencies (so you can pass by reference with no copies?) https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/media/w... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/media/w... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:71: DCHECK(profiles::IsPublicSession() && extension); BTW, if this DCHECK is hit, it won't be clear which part of the expression evaluated to false. So you might want to just have 2 DCHECKs.
The CQ bit was checked by isandrk@chromium.org 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...
Here are the unit tests. Devlin: please take a look at extensions/common/permissions/api_permission_set.* https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:107: return; On 2017/02/07 14:26:51, Andrew T Wilson (Slow) wrote: > Since the body of this if-statement is multi-line due to the comment, put braces > around it all. > > Also, is this code path (the one where a prompt is issued when one is already in > progress) currently covered by tests? I didn't see a test that exercises this. Done. I've written some tests now, this case is covered as well. https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:155: callbacks_to_invoke.push_back(std::move(*callback)); On 2017/02/07 14:26:51, Andrew T Wilson (Slow) wrote: > Explain why you're doing this (i.e. because callbacks_ can be changed by the > callback) Done. https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:51: void ResolvePermissionRequest(PermissionIDSet allowed_permissions); On 2017/02/07 14:26:51, Andrew T Wilson (Slow) wrote: > This is fine as is - but would declaring this as const PermissionIDSet& provide > any efficiencies (so you can pass by reference with no copies?) Not in the current version - two callsites both make the call with a temporary which should afaik be std::moved into the function call. I may be wrong. https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/media/w... File chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/media/w... chrome/browser/media/webrtc/public_session_tab_capture_access_handler.cc:71: DCHECK(profiles::IsPublicSession() && extension); On 2017/02/07 14:26:51, Andrew T Wilson (Slow) wrote: > BTW, if this DCHECK is hit, it won't be clear which part of the expression > evaluated to false. So you might want to just have 2 DCHECKs. Good point. Though I've decided to completely remove this DCHECK as it isn't really useful or neccessary (the function shouldn't even be called because of the if clause above).
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 isandrk@chromium.org 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...
isandrk@chromium.org changed reviewers: + sergeyu@chromium.org
Sergey: ptal at chrome/browser/media/*. I've moved public_session_tab_capture_access_handler one directory up, as @miu wanted on the previous code review (crrev.com/2558843002).
isandrk@chromium.org changed reviewers: - gab@chromium.org, xiyuan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/browser/media lgtm https://codereview.chromium.org/2552203007/diff/540001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2552203007/diff/540001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:53: extensions::PermissionIDSet allowed_permissions); should this be a const reference?
The CQ bit was checked by isandrk@chromium.org 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...
Drew: please take a look at chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc Devlin: please take a look at extensions/common/permissions/api_permission_set.* chrome/browser/extensions/api/page_capture/page_capture_api.* chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (not your area, but you always give valuable comments:) https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... File chrome/browser/extensions/api/page_capture/page_capture_api.h (right): https://codereview.chromium.org/2552203007/diff/500001/chrome/browser/extensi... chrome/browser/extensions/api/page_capture/page_capture_api.h:51: void ResolvePermissionRequest(PermissionIDSet allowed_permissions); On 2017/02/07 14:26:51, Andrew T Wilson (Slow) wrote: > This is fine as is - but would declaring this as const PermissionIDSet& provide > any efficiencies (so you can pass by reference with no copies?) You are actually right, it's better as a const ref. Done. I wanted to use pass by value so the compiler can do std::move optimizations, but that's only useful if you need a copy inside the function (AFAIK). https://codereview.chromium.org/2552203007/diff/540001/chrome/browser/media/p... File chrome/browser/media/public_session_media_access_handler.h (right): https://codereview.chromium.org/2552203007/diff/540001/chrome/browser/media/p... chrome/browser/media/public_session_media_access_handler.h:53: extensions::PermissionIDSet allowed_permissions); On 2017/02/10 01:31:53, Sergey Ulanov wrote: > should this be a const reference? You are right! Done. I wanted to use pass by value so the compiler can do std::move optimizations, but that's only useful if you need a copy inside the function (AFAIK).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I see some compile errors?
The CQ bit was checked by isandrk@chromium.org 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...
On 2017/02/10 13:02:19, Andrew T Wilson (Slow) wrote: > I see some compile errors? Forgot to update unittest. Fixed!
https://codereview.chromium.org/2552203007/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:37: if (g_extension_install_prompt_for_testing.Get()) { I'm not a huge fan of this pattern, especially since it has to be called every time you call HandlePermissionRequest, but you can't tell if HandlePermissionRequest will consume the object or not. A better pattern IMO would be to inject an optional ExtensionInstallPrompt factory (just a callback) which can be null to use the default ExtensionInstallPrompt class. Then callers can just pass an empty callback, while your tests can pass a callback that creates a fake InstallPrompt instance.
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 isandrk@chromium.org 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...
isandrk@chromium.org changed reviewers: - sergeyu@chromium.org
https://codereview.chromium.org/2552203007/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:37: if (g_extension_install_prompt_for_testing.Get()) { On 2017/02/10 13:47:51, Andrew T Wilson (Slow) wrote: > I'm not a huge fan of this pattern, especially since it has to be called every > time you call HandlePermissionRequest, but you can't tell if > HandlePermissionRequest will consume the object or not. > > A better pattern IMO would be to inject an optional ExtensionInstallPrompt > factory (just a callback) which can be null to use the default > ExtensionInstallPrompt class. Then callers can just pass an empty callback, > while your tests can pass a callback that creates a fake InstallPrompt instance. I've tried using a factory approach. Seems good?
Mostly LG, but some feedback on the factory pattern you're using. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:35: if (prompt_factory) { This is OK, but the other way to do this is to have the check for a null callback happen in HandlePermissionRequest, and store a reference to this method as a callback instead. That way HandlePermissionRequestImpl() can just invoke the callback with no further checks. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:28: using PromptFactory = base::Callback<std::unique_ptr<ExtensionInstallPrompt>()>; This should take a WebContents I think - since that's what's needed to create a *real* ExtensionInstallPrompt. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:52: PromptFactory* prompt_factory = nullptr); Don't pass PromptFactory as a pointer - pass it as a const reference, and use is_null() to see whether a callback was provided. See DeviceStatusCollector::VolumeInfoFetcher as an example of this pattern. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:135: last_prompt_ = nullptr; It's messy to keep a last_prompt_ member variable just to pass the created prompt back. It's OK to have this callback be a true factory if you expect it to be called multiple times, but if you expect it to be called only once by HandlePermissionRequest then consider just creating ProgrammableInstallPrompt in this function, and pass in a callback that just returns it. std::unique_ptr<ExtensionInstallPrompt> ReturnPrompt(std::unique_ptr<ExtensionInstallPrompt> prompt) { return prompt; } ProgrammableInstallPrompt* CallHandlePermissionRequest() { ProgrammableInstallPrompt* prompt = new ProgrammalbeInstallPrompt(web_contents()); auto factory_callback = base::Bind(&PublicSessionPermissionHelperTest::ReturnPrompt, base::WrapUnique<ExtensionInstallPrompt>(prompt)); HandlePermissionRequest(...) return prompt; }
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 isandrk@chromium.org 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...
https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:35: if (prompt_factory) { On 2017/02/10 17:17:21, Andrew T Wilson (Slow) wrote: > This is OK, but the other way to do this is to have the check for a null > callback happen in HandlePermissionRequest, and store a reference to this method > as a callback instead. That way HandlePermissionRequestImpl() can just invoke > the callback with no further checks. Done. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:28: using PromptFactory = base::Callback<std::unique_ptr<ExtensionInstallPrompt>()>; On 2017/02/10 17:17:21, Andrew T Wilson (Slow) wrote: > This should take a WebContents I think - since that's what's needed to create a > *real* ExtensionInstallPrompt. Done. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:52: PromptFactory* prompt_factory = nullptr); On 2017/02/10 17:17:21, Andrew T Wilson (Slow) wrote: > Don't pass PromptFactory as a pointer - pass it as a const reference, and use > is_null() to see whether a callback was provided. See > DeviceStatusCollector::VolumeInfoFetcher as an example of this pattern. I see. Done. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:135: last_prompt_ = nullptr; On 2017/02/10 17:17:21, Andrew T Wilson (Slow) wrote: > It's messy to keep a last_prompt_ member variable just to pass the created > prompt back. It's OK to have this callback be a true factory if you expect it to > be called multiple times, but if you expect it to be called only once by > HandlePermissionRequest then consider just creating ProgrammableInstallPrompt in > this function, and pass in a callback that just returns it. > > std::unique_ptr<ExtensionInstallPrompt> > ReturnPrompt(std::unique_ptr<ExtensionInstallPrompt> prompt) { > return prompt; > } > > ProgrammableInstallPrompt* CallHandlePermissionRequest() { > ProgrammableInstallPrompt* prompt = new > ProgrammalbeInstallPrompt(web_contents()); > auto factory_callback = > base::Bind(&PublicSessionPermissionHelperTest::ReturnPrompt, > base::WrapUnique<ExtensionInstallPrompt>(prompt)); > HandlePermissionRequest(...) > return prompt; > } Good point. Done.
Nice! A few nits and suggestions for the unittest, but largely this still looks good. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:153: if (allowed) Optional: for fanciness, PermissionIdSet& add_to_set = prompt_result == ExtensionInstallPrompt::Result::ACCEPTED ? allowed_permission_set_ : denied_permission_set_; for (const auto& permission : unprompted_permissions) { prompted_permission_set_.erase(permission.id()); add_to_set.insert(permission.id()); } I think it's marginally simpler, but mostly just a matter of preference, so up to you. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:46: class ProgrammableInstallPrompt : public ExtensionInstallPrompt { Can we use a ScopedTestDialogAutoConfirm (extensions/browser/extension_dialog_auto_confirm.h) here instead? It can help catch any errors that might not otherwise surface. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:164: // Still denied (previous choice is remembered). It would be nice to make sure we're actually remembering the choice, and that the dialog isn't shown. Right now, for instance, if the dialog is shown, we never resolve it, and thus this test would pass (though the behavior is incorrect). If we can use the scoped autoconfirm, this is pretty easy: { ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::CANCEL); CallHandlePermissionRequest(); EXPECT_TRUE(allowed_permissions_.back().Equals({})); } { // Even though we would accept the prompt, the choice is persisted and the prompt is never shown. ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::ACCEPT); CallHandlePermissionRequest(); EXPECT_TRUE(allowed_permissions_.back().Equals({})); } https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:178: CallHandlePermissionRequest(extension_a_, {permission_b}); Here, too, it would be nice to assert that no prompt is actually shown. https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... File extensions/common/permissions/api_permission_set.cc (right): https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... extensions/common/permissions/api_permission_set.cc:201: const std::initializer_list<APIPermission::ID>& permissions) { nit: if we make this a pass-by-value (or rvalue ref, if you prefer) initializer_list, we could then also just do PermissionIDSet::PermissionIDSet( std::initializer_list<ID> permissions) : permissions_(std::move(permissions)) {} https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... File extensions/common/permissions/api_permission_set.h (right): https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... extensions/common/permissions/api_permission_set.h:119: PermissionIDSet(const std::initializer_list<APIPermission::ID>& permissions); nit: initializer_lists are almost exclusively constructed via an inlined {} initialization (i.e., PermissionIdSet({foo, bar, baz}), so we should probably just pass this by value to allow for the move ctor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:53: const PromptFactory& prompt_factory); nit: document that callers can pass in a null prompt_factory callback to get the default behavior (i.e. it's only for tests) https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:60: void SetExtensionInstallPromptForTesting( This function is no longer necessary, correct? https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:139: return prompt; This looks great - one thing to keep in mind is that prompt *can* get freed before you return (so you'll return a freed pointer) in the case that you call HandlePermissionRequest() for a set of permissions that are already granted. Not a problem for your existing tests, but if there's ever a bug introduced, you might end up getting a crashed test instead of a nice failed expectation when the caller calls Resolve() on a freed pointer, which is mildly more difficult to diagnose/flaky. I think it's fine as-is (I personally wouldn't do anything further). But an alternative would be to use weak pointers to ensure you never return a pointer into freed memory (only a nullptr if the callback was already invoked and the PermissionInstallPrompt was already freed). But I probably wouldn't bother since you'll be getting a crashed test either way.
The CQ bit was checked by isandrk@chromium.org 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...
Drew, Devlin: ptal again! I think everything should be good looking now. https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2552203007/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:153: if (allowed) On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > Optional: for fanciness, > PermissionIdSet& add_to_set = > prompt_result == ExtensionInstallPrompt::Result::ACCEPTED ? > allowed_permission_set_ : denied_permission_set_; > for (const auto& permission : unprompted_permissions) { > prompted_permission_set_.erase(permission.id()); > add_to_set.insert(permission.id()); > } > > I think it's marginally simpler, but mostly just a matter of preference, so up > to you. Seems nice. Done. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:53: const PromptFactory& prompt_factory); On 2017/02/11 09:23:24, Andrew T Wilson (Slow) wrote: > nit: document that callers can pass in a null prompt_factory callback to get the > default behavior (i.e. it's only for tests) Done. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:60: void SetExtensionInstallPromptForTesting( On 2017/02/11 09:23:24, Andrew T Wilson (Slow) wrote: > This function is no longer necessary, correct? Correct, removed. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:46: class ProgrammableInstallPrompt : public ExtensionInstallPrompt { On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > Can we use a ScopedTestDialogAutoConfirm > (extensions/browser/extension_dialog_auto_confirm.h) here instead? It can help > catch any errors that might not otherwise surface. I used it in the first iteration of the unittest, but then realised that I cannot do more complicated tests where I show two prompts and then resolve them in random order, or ALLOW one of them and DENY the other. I could technically use ScopedTestDialogAutoConfirm for some tests, and ProgrammableInstallPrompt for others, but that just seems like complicating things too much. If you know of any errors that can surface and which aren't caught with this approach, I may consider using the mixed approach :) https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:139: return prompt; On 2017/02/11 09:23:24, Andrew T Wilson (Slow) wrote: > This looks great - one thing to keep in mind is that prompt *can* get freed > before you return (so you'll return a freed pointer) in the case that you call > HandlePermissionRequest() for a set of permissions that are already granted. > > Not a problem for your existing tests, but if there's ever a bug introduced, you > might end up getting a crashed test instead of a nice failed expectation when > the caller calls Resolve() on a freed pointer, which is mildly more difficult to > diagnose/flaky. > > I think it's fine as-is (I personally wouldn't do anything further). But an > alternative would be to use weak pointers to ensure you never return a pointer > into freed memory (only a nullptr if the callback was already invoked and the > PermissionInstallPrompt was already freed). But I probably wouldn't bother since > you'll be getting a crashed test either way. It did cross my mind, but I didn't worry too much about it (because tests - maybe I should worry a bit more tho?). I've implemented the WeakPtr approach since it also gives me the ability to tell whether the prompt was called or not. https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:164: // Still denied (previous choice is remembered). On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > It would be nice to make sure we're actually remembering the choice, and that > the dialog isn't shown. Right now, for instance, if the dialog is shown, we > never resolve it, and thus this test would pass (though the behavior is > incorrect). If we can use the scoped autoconfirm, this is pretty easy: > { > ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::CANCEL); > CallHandlePermissionRequest(); > EXPECT_TRUE(allowed_permissions_.back().Equals({})); > } > { > // Even though we would accept the prompt, the choice is persisted and the > prompt is never shown. > ScopedTestDialogAutoConfirm auto_deny(ScopedTestDialogAutoConfirm::ACCEPT); > CallHandlePermissionRequest(); > EXPECT_TRUE(allowed_permissions_.back().Equals({})); > } I made some changes and now I can catch the dialog being shown when it shouldn't. Looks better now? https://codereview.chromium.org/2552203007/diff/620001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:178: CallHandlePermissionRequest(extension_a_, {permission_b}); On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > Here, too, it would be nice to assert that no prompt is actually shown. Done. https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... File extensions/common/permissions/api_permission_set.cc (right): https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... extensions/common/permissions/api_permission_set.cc:201: const std::initializer_list<APIPermission::ID>& permissions) { On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > nit: if we make this a pass-by-value (or rvalue ref, if you prefer) > initializer_list, we could then also just do > PermissionIDSet::PermissionIDSet( > std::initializer_list<ID> permissions) > : permissions_(std::move(permissions)) {} This doesn't really compile. I've tried getting it to work before, the problem is that this class stores a set<PermissionID>, while we pass in a list of APIPermission::ID. Maybe I'm missing something, this is the error output from the compiler https://paste.googleplex.com/5486718933270528 https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... File extensions/common/permissions/api_permission_set.h (right): https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... extensions/common/permissions/api_permission_set.h:119: PermissionIDSet(const std::initializer_list<APIPermission::ID>& permissions); On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > nit: initializer_lists are almost exclusively constructed via an inlined {} > initialization (i.e., PermissionIdSet({foo, bar, baz}), so we should probably > just pass this by value to allow for the move ctor. Done.
Description was changed from ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=689478 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* ========== to ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=689478 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* UNITTEST=PublicSessionPermissionHelperTest.* ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm again with a suggestion for the unittest to make it a bit clearer. https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... File extensions/common/permissions/api_permission_set.cc (right): https://codereview.chromium.org/2552203007/diff/620001/extensions/common/perm... extensions/common/permissions/api_permission_set.cc:201: const std::initializer_list<APIPermission::ID>& permissions) { On 2017/02/13 13:18:20, Ivan Šandrk wrote: > On 2017/02/10 19:54:46, Devlin (OOO feb 8 and feb 9) wrote: > > nit: if we make this a pass-by-value (or rvalue ref, if you prefer) > > initializer_list, we could then also just do > > PermissionIDSet::PermissionIDSet( > > std::initializer_list<ID> permissions) > > : permissions_(std::move(permissions)) {} > > This doesn't really compile. I've tried getting it to work before, the problem > is that this class stores a set<PermissionID>, while we pass in a list of > APIPermission::ID. > > Maybe I'm missing something, this is the error output from the compiler > https://paste.googleplex.com/5486718933270528 Oh, grr, you're right. Not something you should have to fix in this cl. This is fine. https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:147: // In case all permissions were already prompted, ReturnPrompt isn't called To me, this seems like it's very dependent on the implementation in HandlePermissionRequest(), almost to the point where it's more likely to break due to a refactor than to a behavior change. What about instead doing something like having ProgrammableInstallPrompt::ShowDialog() set a flag (did_show_dialog) to true, and resetting that as appropriate? (See also below.) https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:159: EXPECT_TRUE(prompt); s/EXPECT_TRUE(prompt)/EXPECT_TRUE(did_show_dialog()); https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:162: reset_did_show_dialog(); https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:167: EXPECT_FALSE(CallHandlePermissionRequest(extension_a_, {permission_a})); EXPECT_FALSE(did_show_dialog()) https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:172: EXPECT_TRUE(prompt); EXPECT_TRUE(did_show_dialog()) ... etc It's a little more verbose, but I think it's a lot more clear what you're testing, and why, so I think it's worth the extra lines in the unittest file. :)
Still LGTM after your WeakPtr changes. Please land this bird :)
Still LGTM after your WeakPtr changes. Please land this bird :)
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, gab@chromium.org, sergeyu@chromium.org, rdevlin.cronin@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2552203007/#ps680001 (title: "Git cl format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All done. Thanks for the help! https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc (right): https://codereview.chromium.org/2552203007/diff/640001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc:147: // In case all permissions were already prompted, ReturnPrompt isn't called On 2017/02/13 22:34:00, Devlin wrote: > To me, this seems like it's very dependent on the implementation in > HandlePermissionRequest(), almost to the point where it's more likely to break > due to a refactor than to a behavior change. What about instead doing something > like having ProgrammableInstallPrompt::ShowDialog() set a flag (did_show_dialog) > to true, and resetting that as appropriate? (See also below.) Good suggestion. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/extensions/api/page_capture/page_capture_api.cc:
While running git apply --index -p1;
error: patch failed:
chrome/browser/extensions/api/page_capture/page_capture_api.cc:36
error: chrome/browser/extensions/api/page_capture/page_capture_api.cc: patch
does not apply
Patch: chrome/browser/extensions/api/page_capture/page_capture_api.cc
Index: chrome/browser/extensions/api/page_capture/page_capture_api.cc
diff --git a/chrome/browser/extensions/api/page_capture/page_capture_api.cc
b/chrome/browser/extensions/api/page_capture/page_capture_api.cc
index
1e7c549d533f94e0f6f1c53a883d9c5680c85049..84c360f5641f8bebec0c0a0d65a1205febb4b9bd
100644
--- a/chrome/browser/extensions/api/page_capture/page_capture_api.cc
+++ b/chrome/browser/extensions/api/page_capture/page_capture_api.cc
@@ -8,10 +8,13 @@
#include <memory>
#include "base/bind.h"
+#include "base/bind_helpers.h"
#include "base/files/file_util.h"
+#include "base/memory/ptr_util.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profiles_state.h"
#include "content/public/browser/child_process_security_policy.h"
#include "content/public/browser/notification_details.h"
#include "content/public/browser/notification_source.h"
@@ -22,6 +25,10 @@
#include "content/public/common/mhtml_generation_params.h"
#include "extensions/common/extension_messages.h"
+#if defined(OS_CHROMEOS)
+#include
"chrome/browser/chromeos/extensions/public_session_permission_helper.h"
+#endif
+
using content::BrowserThread;
using content::ChildProcessSecurityPolicy;
using content::WebContents;
@@ -36,6 +43,9 @@ const char kFileTooBigError[] = "The MHTML file generated is
too big.";
const char kMHTMLGenerationFailedError[] = "Failed to generate MHTML.";
const char kTemporaryFileError[] = "Failed to create a temporary file.";
const char kTabClosedError[] = "Cannot find the tab for this request.";
+#if defined(OS_CHROMEOS)
+const char kUserDenied[] = "User denied request.";
+#endif
} // namespace
@@ -63,6 +73,31 @@ bool PageCaptureSaveAsMHTMLFunction::RunAsync() {
AddRef(); // Balanced in ReturnFailure/ReturnSuccess()
+ // In Public Sessions, extensions (and apps) are force-installed by admin
+ // policy so the user does not get a chance to review the permissions for
+ // these extensions. This is not acceptable from a security/privacy
+ // standpoint, so when an extension uses the PageCapture API for the first
+ // time, we show the user a dialog where they can choose whether to allow the
+ // extension access to the API.
+#if defined(OS_CHROMEOS)
+ if (profiles::IsPublicSession()) {
+ WebContents* web_contents = GetWebContents();
+ if (!web_contents) {
+ ReturnFailure(kTabClosedError);
+ return true;
+ }
+ // This Unretained is safe because this object is Released() in
+ // OnMessageReceived which gets called at some point after callback is run.
+ auto callback =
+ base::Bind(&PageCaptureSaveAsMHTMLFunction::ResolvePermissionRequest,
+ base::Unretained(this));
+ permission_helper::HandlePermissionRequest(
+ *extension(), {APIPermission::kPageCapture}, web_contents, callback,
+ permission_helper::PromptFactory());
+ return true;
+ }
+#endif
+
BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
base::Bind(&PageCaptureSaveAsMHTMLFunction::CreateTemporaryFile, this));
@@ -91,6 +126,19 @@ bool PageCaptureSaveAsMHTMLFunction::OnMessageReceived(
return true;
}
+#if defined(OS_CHROMEOS)
+void PageCaptureSaveAsMHTMLFunction::ResolvePermissionRequest(
+ const PermissionIDSet& allowed_permissions) {
+ if (allowed_permissions.ContainsID(APIPermission::kPageCapture)) {
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&PageCaptureSaveAsMHTMLFunction::CreateTemporaryFile,
this));
+ } else {
+ ReturnFailure(kUserDenied);
+ }
+}
+#endif
+
void PageCaptureSaveAsMHTMLFunction::CreateTemporaryFile() {
DCHECK_CURRENTLY_ON(BrowserThread::FILE);
bool success = base::CreateTemporaryFile(&mhtml_path_);
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, xiyuan@chromium.org, rdevlin.cronin@chromium.org, atwilson@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2552203007/#ps720001 (title: "Nitfix")
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": 720001, "attempt_start_ts": 1487087348048990,
"parent_rev": "b5a275836d850892ca952e33f5a83efc7ac070ef", "commit_rev":
"309c0997afe19ab971686abf5cca1dd7fd4f7d8b"}
Message was sent while issue was closed.
Description was changed from ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=689478 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* UNITTEST=PublicSessionPermissionHelperTest.* ========== to ========== Public Sessions - prompt the user for pageCapture requests In Public Sessions, extensions (and apps) are force-installed by admin policy so the user does not get a chance to review the permissions for these extensions. This is not acceptable from a security/privacy standpoint, so when an extension uses the pageCapture API for the first time, we show the user a dialog where they can choose whether to allow the extension access to the API. BUG=689478 BROWSERTEST=ExtensionPageCaptureApiTest.PublicSessionRequest* UNITTEST=PublicSessionPermissionHelperTest.* Review-Url: https://codereview.chromium.org/2552203007 Cr-Commit-Position: refs/heads/master@{#450383} Committed: https://chromium.googlesource.com/chromium/src/+/309c0997afe19ab971686abf5cca... ==========
Message was sent while issue was closed.
Committed patchset #34 (id:720001) as https://chromium.googlesource.com/chromium/src/+/309c0997afe19ab971686abf5cca...
Message was sent while issue was closed.
On 2017/02/14 16:58:03, commit-bot: I haz the power wrote: > Committed patchset #34 (id:720001) as > https://chromium.googlesource.com/chromium/src/+/309c0997afe19ab971686abf5cca... Woo! Thanks for your work here, Ivan. :)
Message was sent while issue was closed.
A revert of this CL (patchset #34 id:720001) has been created in https://codereview.chromium.org/2697833004/ by xlai@chromium.org. The reason for reverting is: This CL is causing many PublicSessionPermissionHelperTest unit tests to fail on Linux Chromium OS ASan LSan Tests: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... Please fix the leaks before relanding it again.. |
