Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149953002:
Enable login screen apps. (Closed)
|
Created:
4 years, 5 months ago by achuithb Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable login screen apps.
ChromeProcessManagerDelegate:
* Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed.
* IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy.
ExtensionService:
* Do not load saved extensions in the ChromeOS signin profile.
ProfileManager:
* Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set.
ExternalProviderImpl:
* In CreateExternalProviders, create an ExternalPolicyLoader for login
screen apps in the chromeos signin profile.
* Switch --enable-login-screen-apps to enable apps in the chromeos signin profile.
BUG=576464
Review-Url: https://codereview.chromium.org/2149953002
Cr-Commit-Position: refs/heads/master@{#459190}
Committed: https://chromium.googlesource.com/chromium/src/+/d3da4f0a03a09f326f9e0ddf871bb268599ffc70
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rename is_normal_session #Patch Set 3 : rebase #Patch Set 4 : Update IsBackgroundPageAllowed #Patch Set 5 : Don't load saved extensions. #Patch Set 6 : Disable background pages for non-policy apps #Patch Set 7 : Fix is_normal_session logic #
Total comments: 11
Patch Set 8 : Max feedback #Patch Set 9 : Early return in IsBackgroundPageAllowed #Patch Set 10 : Additional IsBackgroundPageAllowed check #
Total comments: 20
Patch Set 11 : Devlin and Maksim feedback #
Total comments: 13
Patch Set 12 : Devlin feedback + rebase #
Total comments: 3
Patch Set 13 : Fix unittest #
Total comments: 6
Patch Set 14 : Devlin comments #
Total comments: 2
Patch Set 15 : Stefan feedback #Messages
Total messages: 88 (60 generated)
The CQ bit was checked by achuith@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.
achuith@chromium.org changed reviewers: + emaxx@chromium.org
https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_process_manager_delegate.cc:62: !ExtensionManagementFactory::GetForBrowserContext(profile) nit: The function accetps BrowserContext*, so there is no reason to pass profile here instead of context. https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: (is_signin_profile && login_apps_enabled && login_apps_installed); I think it's better to use another boolean variable for this login-related stuff, as the name "is_normal_session" doesn't really correspond to the case of the login screen.
Please excuse the rebase. https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_process_manager_delegate.cc:62: !ExtensionManagementFactory::GetForBrowserContext(profile) On 2016/07/15 16:31:09, emaxx wrote: > nit: The function accetps BrowserContext*, so there is no reason to pass profile > here instead of context. Done. https://codereview.chromium.org/2149953002/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: (is_signin_profile && login_apps_enabled && login_apps_installed); On 2016/07/15 16:31:09, emaxx wrote: > I think it's better to use another boolean variable for this login-related > stuff, as the name "is_normal_session" doesn't really correspond to the case of > the login screen. Done.
achuith@chromium.org changed reviewers: + antrim@chromium.org
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
achuith@chromium.org changed reviewers: - antrim@chromium.org
PTAL Max. This CL no longer creates unnecessary background hosts in the signin profile.
Patchset #7 (id:220001) has been deleted
Some early feedback. https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:51: bool is_normal_session = !profile->IsGuestSession() && As per offline discussion and as per previous iterations, I'd personally find the version with separate branches more understandable: if (is_signin_profile) { ... } else { ... // consider non-signin profile: calculate is_normal_profile } https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: const bool login_screen_apps_enabled = I'm still feeling that this check here is excessive: we already have the check against the enterprise policy (login_screen_apps_installed, is_login_screen_app). As we may trust to the policy stack that the policy can't be spoofed (its signature is validated), and as the policy won't be exposed to anyone until we implement everything, I don't think there's much value in keeping this condition here. https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:80: is_login_screen_app = login_screen_apps_list->HasKey(extension->id()); nit: #include "extensions/common/extension.h" https://codereview.chromium.org/2149953002/diff/240001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/common/chrome_s... chrome/common/chrome_switches.h:118: extern const char kEnableLoginScreenApps[]; nit: There's an OS_CHROMEOS block below, better put this one into it.
PTAL Max https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:51: bool is_normal_session = !profile->IsGuestSession() && On 2017/03/06 17:04:03, emaxx wrote: > As per offline discussion and as per previous iterations, I'd personally find > the version with separate branches more understandable: > if (is_signin_profile) { > ... > } else { > ... // consider non-signin profile: calculate is_normal_profile > } I've re-added the else statment. My objection to this is that booleans and if-else statements are easy to get wrong, if you forget to update a global boolean in all paths, as we just did; and I didn't spot the error after looking at this code dozens of times, and only saw it after printing out all the booleans in this function, and observing that is_normal_session is true for the signin screen. The other benefit is that we preserve the old behavior. I'm interested in limiting the changes we make to the logic in this delegate function. https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: const bool login_screen_apps_enabled = On 2017/03/06 17:04:03, emaxx wrote: > I'm still feeling that this check here is excessive: we already have the check > against the enterprise policy (login_screen_apps_installed, > is_login_screen_app). As we may trust to the policy stack that the policy can't > be spoofed (its signature is validated), and as the policy won't be exposed to > anyone until we implement everything, I don't think there's much value in > keeping this condition here. I prefer to keep this for the following reasons: 1. It's a failsafe in case something else in our logic is wrong. If somehow a login screen app gets side-installed, this will at least prevent it's background page from launching. Our understanding of the various pathways in the extensions code is still not complete, so I think being a bit more paranoid is fine. 2. There's no downside to this check, and we can have faith that all the rest of the logic after this can be easily turned off if something goes wrong. 3. This is a temporary flag, so this is just a short time nuisance at worst. https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:80: is_login_screen_app = login_screen_apps_list->HasKey(extension->id()); On 2017/03/06 17:04:04, emaxx wrote: > nit: #include "extensions/common/extension.h" Done. https://codereview.chromium.org/2149953002/diff/240001/chrome/common/chrome_s... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/common/chrome_s... chrome/common/chrome_switches.h:118: extern const char kEnableLoginScreenApps[]; On 2017/03/06 17:04:04, emaxx wrote: > nit: There's an OS_CHROMEOS block below, better put this one into it. Done.
https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:51: bool is_normal_session = !profile->IsGuestSession() && On 2017/03/06 18:21:05, achuithb wrote: > On 2017/03/06 17:04:03, emaxx wrote: > > As per offline discussion and as per previous iterations, I'd personally find > > the version with separate branches more understandable: > > if (is_signin_profile) { > > ... > > } else { > > ... // consider non-signin profile: calculate is_normal_profile > > } > > I've re-added the else statment. My objection to this is that booleans and > if-else statements are easy to get wrong, if you forget to update a global > boolean in all paths, as we just did; and I didn't spot the error after looking > at this code dozens of times, and only saw it after printing out all the > booleans in this function, and observing that is_normal_session is true for the > signin screen. > > The other benefit is that we preserve the old behavior. I'm interested in > limiting the changes we make to the logic in this delegate function. It's your call anyway, but in the end I think we both agree on the same goal: make the signin profile case and the normal user case as independent as possible (because the flags and conditions that make sense for one don't for the other). The other option I may suggest is sacrificing the single returning point, and doing the return statement from inside the "is_signin_profile" block directly. Then we won't have to take care of is_normal_session inside this block, and won't have to modify the code outside this block. WDYT? https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: const bool login_screen_apps_enabled = On 2017/03/06 18:21:05, achuithb wrote: > On 2017/03/06 17:04:03, emaxx wrote: > > I'm still feeling that this check here is excessive: we already have the check > > against the enterprise policy (login_screen_apps_installed, > > is_login_screen_app). As we may trust to the policy stack that the policy > can't > > be spoofed (its signature is validated), and as the policy won't be exposed to > > anyone until we implement everything, I don't think there's much value in > > keeping this condition here. > > I prefer to keep this for the following reasons: > 1. It's a failsafe in case something else in our logic is wrong. If somehow a > login screen app gets side-installed, this will at least prevent it's background > page from launching. Our understanding of the various pathways in the extensions > code is still not complete, so I think being a bit more paranoid is fine. > 2. There's no downside to this check, and we can have faith that all the rest of > the logic after this can be easily turned off if something goes wrong. > 3. This is a temporary flag, so this is just a short time nuisance at worst. Acknowledged.
https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:51: bool is_normal_session = !profile->IsGuestSession() && On 2017/03/07 01:36:42, emaxx wrote: > On 2017/03/06 18:21:05, achuithb wrote: > > On 2017/03/06 17:04:03, emaxx wrote: > > > As per offline discussion and as per previous iterations, I'd personally > find > > > the version with separate branches more understandable: > > > if (is_signin_profile) { > > > ... > > > } else { > > > ... // consider non-signin profile: calculate is_normal_profile > > > } > > > > I've re-added the else statment. My objection to this is that booleans and > > if-else statements are easy to get wrong, if you forget to update a global > > boolean in all paths, as we just did; and I didn't spot the error after > looking > > at this code dozens of times, and only saw it after printing out all the > > booleans in this function, and observing that is_normal_session is true for > the > > signin screen. > > > > The other benefit is that we preserve the old behavior. I'm interested in > > limiting the changes we make to the logic in this delegate function. > > It's your call anyway, but in the end I think we both agree on the same goal: > make the signin profile case and the normal user case as independent as possible > (because the flags and conditions that make sense for one don't for the other). > > The other option I may suggest is sacrificing the single returning point, and > doing the return statement from inside the "is_signin_profile" block directly. > Then we won't have to take care of is_normal_session inside this block, and > won't have to modify the code outside this block. WDYT? I like this idea. It isolates our changes in this function and makes it less likely that this CL have some side effect. Done.
PTAL Max
LGTM!
achuith@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Devlin, this is the follow-up CL that actually downloads and enables apps in the chromeos signin profile. Please take a look.
Description was changed from ========== Apply login apps device policy. ChromeProcessManagerDelegate: * Enable background pages for apps in the login profile when command-line switch --enable-login-apps is set. ExtensionSystemImpl: * Enable extensions in login profile when command-line switch --enable-login-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login apps in the login profile. * Switch --enable-login-apps to enable login apps. BUG=576464 ========== to ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Enable background pages for apps in the login profile when command-line switch --enable-login-apps is set. ExtensionSystemImpl: * Enable extensions in login profile when command-line switch --enable-login-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login apps in the login profile. * Switch --enable-login-apps to enable login apps. BUG=576464 ==========
Description was changed from ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Enable background pages for apps in the login profile when command-line switch --enable-login-apps is set. ExtensionSystemImpl: * Enable extensions in login profile when command-line switch --enable-login-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login apps in the login profile. * Switch --enable-login-apps to enable login apps. BUG=576464 ========== to ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Enable background pages for apps in the login profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionSystemImpl: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ==========
The CQ bit was checked by achuith@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.
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Patchset #10 (id:300001) has been deleted
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 achuith@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:320001) has been deleted
The CQ bit was checked by achuith@chromium.org to run a CQ dry run
Patchset #10 (id:340001) has been deleted
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:360001) has been deleted
The CQ bit was checked by achuith@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 with nits https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:65: if (extension) { Maybe reduce the nested level here by inverting this into if (!extension) return false; ? Or you don't like introducing one more exit point here? https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:22: // |context|. nit: Please update the comment to talk about |extension| too. (Including that it can be null and what the function returns in that case.)
https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:65: if (extension) { When can |extension| be null? https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:77: login_screen_apps_list->HasKey(extension->id()); I would expect CreateBackgroundHost() to only be called for loaded extensions. How would the extension get loaded if it's not on the list of allowed extensions? https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:215: extensions_enabled = true; I'd rather we instead update the call site to pass the proper value for extensions_enabled here - it's confusing to have multiple places determine it. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:315: // Don't load saved extensions in signin profile. This seems like the wrong place to put this... we should probably just not call LoadAllExtensions(). Ideally, we'd even avoid saving any in the profile, but that might break stuff. https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:22: // |context|. On 2017/03/08 17:11:25, emaxx wrote: > nit: Please update the comment to talk about |extension| too. (Including that it > can be null and what the function returns in that case.) Can it be null?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, Devlin and Max https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:65: if (extension) { On 2017/03/08 17:11:25, emaxx wrote: > Maybe reduce the nested level here by inverting this into > if (!extension) > return false; > ? Or you don't like introducing one more exit point here? Done. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:65: if (extension) { On 2017/03/08 17:24:32, Devlin wrote: > When can |extension| be null? I'll add a comment to the line where it's null https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:77: login_screen_apps_list->HasKey(extension->id()); On 2017/03/08 17:24:32, Devlin wrote: > I would expect CreateBackgroundHost() to only be called for loaded extensions. > How would the extension get loaded if it's not on the list of allowed > extensions? I don't entirely follow your question. We're actually blocking CreateBackgroundHost for all loaded extensions in the signin profile except login screen apps specified by policy. This is the old behavior. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/extension_system_impl.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/extension_system_impl.cc:215: extensions_enabled = true; On 2017/03/08 17:24:32, Devlin wrote: > I'd rather we instead update the call site to pass the proper value for > extensions_enabled here - it's confusing to have multiple places determine it. Done. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:315: // Don't load saved extensions in signin profile. On 2017/03/08 17:24:32, Devlin wrote: > This seems like the wrong place to put this... we should probably just not call > LoadAllExtensions(). Ideally, we'd even avoid saving any in the profile, but > that might break stuff. I've tried to keep as much of the old behavior as possible. This function LoadAllExtensions is doing a bunch of stuff, and I can't imagine it's safe to simply not call it. The model I've used here is that of extensions previously loaded via command line. These are still saved, but not loaded on restart, which is exactly the behavior we want for signin profile apps. I've modified this code to make that a little clearer. PTAL. https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager.cc:391: if (delegate && !delegate->IsBackgroundPageAllowed(browser_context_, nullptr)) This is where extension can be null. We considered removing this check altogether, but not sure if that's safe. https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager.cc:712: delegate->IsBackgroundPageAllowed(browser_context_, extension.get())) We've added this additional check because extension=null allows all extensions in the signin profile, and this check only allows the ones specified by policy. https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:22: // |context|. On 2017/03/08 17:11:25, emaxx wrote: > nit: Please update the comment to talk about |extension| too. (Including that it > can be null and what the function returns in that case.) Done. https://codereview.chromium.org/2149953002/diff/380001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:22: // |context|. On 2017/03/08 17:24:32, Devlin wrote: > On 2017/03/08 17:11:25, emaxx wrote: > > nit: Please update the comment to talk about |extension| too. (Including that > it > > can be null and what the function returns in that case.) > > Can it be null? Yes, there's 2 places where IsBackgroundPageAllowed was called (now 3), and there's no extension available in one of them.
https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:77: login_screen_apps_list->HasKey(extension->id()); On 2017/03/09 14:31:05, achuithb wrote: > On 2017/03/08 17:24:32, Devlin wrote: > > I would expect CreateBackgroundHost() to only be called for loaded extensions. > > > How would the extension get loaded if it's not on the list of allowed > > extensions? > > I don't entirely follow your question. > > We're actually blocking CreateBackgroundHost for all loaded extensions in the > signin profile except login screen apps specified by policy. This is the old > behavior. To rephrase my question: If this is the signin profile, which (my understanding is) currently doesn't allow extensions, then why would we have loaded an extension in that profile *unless* it is allowed by this policy? Wouldn't that make this check redundant? https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:315: // Don't load saved extensions in signin profile. On 2017/03/09 14:31:06, achuithb wrote: > On 2017/03/08 17:24:32, Devlin wrote: > > This seems like the wrong place to put this... we should probably just not > call > > LoadAllExtensions(). Ideally, we'd even avoid saving any in the profile, but > > that might break stuff. > > I've tried to keep as much of the old behavior as possible. This function > LoadAllExtensions is doing a bunch of stuff, and I can't imagine it's safe to > simply not call it. > > The model I've used here is that of extensions previously loaded via command > line. These are still saved, but not loaded on restart, which is exactly the > behavior we want for signin profile apps. I've modified this code to make that a > little clearer. PTAL. LoadAllExtensions() basically iterates over the profile prefs, grabs the extensions mentioned there, and tries to load them. For a signin profile, do we want to persist these preferences? Does the signin profile have stored state? Additionally, this isn't quite the same as command line extensions, since that's a per-extension decision, whereas this is a per-profile decision. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:369: !delegate->IsBackgroundPageAllowed(browser_context_, extension)) We check this in three different places now, making it a bit redundant. :)
The CQ bit was checked by achuith@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...
Let me know your thoughts, Devlin. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:77: login_screen_apps_list->HasKey(extension->id()); On 2017/03/13 19:39:35, Devlin wrote: > On 2017/03/09 14:31:05, achuithb wrote: > > On 2017/03/08 17:24:32, Devlin wrote: > > > I would expect CreateBackgroundHost() to only be called for loaded > extensions. > > > > > How would the extension get loaded if it's not on the list of allowed > > > extensions? > > > > I don't entirely follow your question. > > > > We're actually blocking CreateBackgroundHost for all loaded extensions in the > > signin profile except login screen apps specified by policy. This is the old > > behavior. > > To rephrase my question: If this is the signin profile, which (my understanding > is) currently doesn't allow extensions, then why would we have loaded an > extension in that profile *unless* it is allowed by this policy? Wouldn't that > make this check redundant? Component extensions are loaded into the signin profile. So this branch is relevant for component extensions. Examples are gnubby extension, chromevox, etc. Background pages of component extensions are not allowed to execute here. They are allowed to run in the OTR profile of the signin profile. https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/380001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:315: // Don't load saved extensions in signin profile. On 2017/03/13 19:39:35, Devlin wrote: > On 2017/03/09 14:31:06, achuithb wrote: > > On 2017/03/08 17:24:32, Devlin wrote: > > > This seems like the wrong place to put this... we should probably just not > > call > > > LoadAllExtensions(). Ideally, we'd even avoid saving any in the profile, > but > > > that might break stuff. > > > > I've tried to keep as much of the old behavior as possible. This function > > LoadAllExtensions is doing a bunch of stuff, and I can't imagine it's safe to > > simply not call it. > > > > The model I've used here is that of extensions previously loaded via command > > line. These are still saved, but not loaded on restart, which is exactly the > > behavior we want for signin profile apps. I've modified this code to make that > a > > little clearer. PTAL. > > LoadAllExtensions() basically iterates over the profile prefs, grabs the > extensions mentioned there, and tries to load them. For a signin profile, do we > want to persist these preferences? Does the signin profile have stored state? > > Additionally, this isn't quite the same as command line extensions, since that's > a per-extension decision, whereas this is a per-profile decision. There's state in the signin profile - for example the list of signed in users, the network connection, language/keyboard, accessibility settings (high contrast, large cursor), etc. I'm not 100% sure if these are stored as preferences or in Local State. Even though the signin profile is not associated with a particular user, it's not an incognito profile. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:369: !delegate->IsBackgroundPageAllowed(browser_context_, extension)) On 2017/03/13 19:39:35, Devlin wrote: > We check this in three different places now, making it a bit redundant. :) It does seem that way :/ What would you propose? https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:391: if (delegate && !delegate->IsBackgroundPageAllowed(browser_context_, nullptr)) We could potentially drop this check since we check again in CreateStartupBackgroundHosts. I'm just worried about changing a lot of behavior here since this code is quite tricky, and does different things in different contexts on different platforms.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the explanation. I'm quite hazy on signin profiles, so sorry for the back and forth. :) https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/external_provider_impl.cc (right): https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/external_provider_impl.cc:511: Extension::FROM_WEBSTORE | Extension::WAS_INSTALLED_BY_DEFAULT)); We should add a comment explaining why these flags are correct. https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:319: if (chromeos::ProfileHelper::IsSigninProfile(profile)) We should just early out of this whole function, or not call it in the first place. The only point is to load extensions from prefs, so if we're not doing that, there's nothing to do. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:391: if (delegate && !delegate->IsBackgroundPageAllowed(browser_context_, nullptr)) On 2017/03/15 00:40:25, achuithb wrote: > We could potentially drop this check since we check again in > CreateStartupBackgroundHosts. > > I'm just worried about changing a lot of behavior here since this code is quite > tricky, and does different things in different contexts on different platforms. This one is somewhat beneficial, since it helps us early-out if no background pages are allowed. But we should change it to AreBackgroundPagesAllowedForContext() (see also comment in the delegate file). https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:712: delegate->IsBackgroundPageAllowed(browser_context_, extension.get())) This one should be able to be avoided - we'll check whether or not its allowed when we try to create it in CreateBackgroundHost(). https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:23: virtual bool IsBackgroundPageAllowed(content::BrowserContext* context, Let's break this into two functions: bool AreBackgroundPagesAllowedForContext(content::BrowserContext* context) and bool IsExtensionBackgroundPageAllowed(content::BrowserContext* context, const Extension& extension) That way, it's clear what the semantics and meaning of each is, and it prevents us from having to deal with null extensions. (And for the non-chromeos version of IsExtensionBackgroundPageAllowed(), it would just return AreBackgroundPagesAllowedForContext())
Patchset #12 (id:420001) has been deleted
PTAL Devlin. Please excuse the rebase. https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/external_provider_impl.cc (right): https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/external_provider_impl.cc:511: Extension::FROM_WEBSTORE | Extension::WAS_INSTALLED_BY_DEFAULT)); On 2017/03/17 16:37:23, Devlin wrote: > We should add a comment explaining why these flags are correct. Done. https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2149953002/diff/400001/chrome/browser/extensi... chrome/browser/extensions/installed_loader.cc:319: if (chromeos::ProfileHelper::IsSigninProfile(profile)) On 2017/03/17 16:37:23, Devlin wrote: > We should just early out of this whole function, or not call it in the first > place. The only point is to load extensions from prefs, so if we're not doing > that, there's nothing to do. Done. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:391: if (delegate && !delegate->IsBackgroundPageAllowed(browser_context_, nullptr)) On 2017/03/17 16:37:23, Devlin wrote: > On 2017/03/15 00:40:25, achuithb wrote: > > We could potentially drop this check since we check again in > > CreateStartupBackgroundHosts. > > > > I'm just worried about changing a lot of behavior here since this code is > quite > > tricky, and does different things in different contexts on different > platforms. > > This one is somewhat beneficial, since it helps us early-out if no background > pages are allowed. But we should change it to > AreBackgroundPagesAllowedForContext() (see also comment in the delegate file). Done. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager.cc:712: delegate->IsBackgroundPageAllowed(browser_context_, extension.get())) On 2017/03/17 16:37:23, Devlin wrote: > This one should be able to be avoided - we'll check whether or not its allowed > when we try to create it in CreateBackgroundHost(). Done. https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/400001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:23: virtual bool IsBackgroundPageAllowed(content::BrowserContext* context, On 2017/03/17 16:37:23, Devlin wrote: > Let's break this into two functions: > bool AreBackgroundPagesAllowedForContext(content::BrowserContext* context) > and > bool IsExtensionBackgroundPageAllowed(content::BrowserContext* context, const > Extension& extension) > > That way, it's clear what the semantics and meaning of each is, and it prevents > us from having to deal with null extensions. (And for the non-chromeos version > of IsExtensionBackgroundPageAllowed(), it would just return > AreBackgroundPagesAllowedForContext()) Done. https://codereview.chromium.org/2149953002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2149953002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:453: OnLoadedInstalledExtensions(); // Callback from LoadAllExtensions. This starts the updater and must be called. https://codereview.chromium.org/2149953002/diff/440001/chrome/browser/extensi... File chrome/browser/extensions/external_provider_impl.cc (right): https://codereview.chromium.org/2149953002/diff/440001/chrome/browser/extensi... chrome/browser/extensions/external_provider_impl.cc:507: // extensions. Let me know if you have better wording for this or would like me to move these comments elsewhere. https://codereview.chromium.org/2149953002/diff/440001/extensions/browser/pro... File extensions/browser/process_manager.cc (right): https://codereview.chromium.org/2149953002/diff/440001/extensions/browser/pro... extensions/browser/process_manager.cc:375: DVLOG(1) << "CreateBackgroundHost " << extension->id(); This comment is preferable here rather than line 109 because when we see this line printed, we're guaranteed that an extension host has been created.
The CQ bit was checked by achuith@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_tsan_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 achuith@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 ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Enable background pages for apps in the login profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionSystemImpl: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ========== to ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed. * IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionService: * Do not load saved extensions in the ChromeOS signin profile. ProfileManager: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ==========
Description was changed from ========== Apply login screen apps device policy. ChromeProcessManagerDelegate: * Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed. * IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionService: * Do not load saved extensions in the ChromeOS signin profile. ProfileManager: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ========== to ========== Enable login screen apps. ChromeProcessManagerDelegate: * Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed. * IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionService: * Do not load saved extensions in the ChromeOS signin profile. ProfileManager: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits. Thank you for your patience! https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: Profile* profile = static_cast<Profile*>(context); nitty nit: slightly prefer Profile::FromBrowserContext(context); https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:453: OnLoadedInstalledExtensions(); // Callback from LoadAllExtensions. nit: elaborate a bit on this comment. Also, this flow is somewhat silly (unrelated to this CL) - mind adding a TODO(me) to clean it up? if (load_saved_extensions) { extensions::InstalledLoader(this).LoadAllExtensions(); } else { // InstalledLoader::LoadAllExtensions normally calls OnLoadedInstalledExtensions // itself, but here we circumvent that path. Call OnLoadedInstalledExtensions // directly. // TODO(devlin): LoadInstalledExtensions() is synchronous - we can simplify // this. OnLoadedInstalledExtensions(); } https://codereview.chromium.org/2149953002/diff/460001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/460001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:30: const Extension* extension) const = 0; |extension| can no longer be null, right? Can be we make it a const& to reflect that?
Thanks for your patience and feedback, Devlin! https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:67: Profile* profile = static_cast<Profile*>(context); On 2017/03/22 22:57:39, Devlin wrote: > nitty nit: slightly prefer Profile::FromBrowserContext(context); Done for this function and the other 2 delegate functions for consistency. https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2149953002/diff/460001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:453: OnLoadedInstalledExtensions(); // Callback from LoadAllExtensions. On 2017/03/22 22:57:39, Devlin wrote: > nit: elaborate a bit on this comment. Also, this flow is somewhat silly > (unrelated to this CL) - mind adding a TODO(me) to clean it up? > > if (load_saved_extensions) { > extensions::InstalledLoader(this).LoadAllExtensions(); > } else { > // InstalledLoader::LoadAllExtensions normally calls > OnLoadedInstalledExtensions > // itself, but here we circumvent that path. Call OnLoadedInstalledExtensions > // directly. > // TODO(devlin): LoadInstalledExtensions() is synchronous - we can simplify > // this. > OnLoadedInstalledExtensions(); > } Done. I thought about refactoring this, but I think a follow-up CL would be better. https://codereview.chromium.org/2149953002/diff/460001/extensions/browser/pro... File extensions/browser/process_manager_delegate.h (right): https://codereview.chromium.org/2149953002/diff/460001/extensions/browser/pro... extensions/browser/process_manager_delegate.h:30: const Extension* extension) const = 0; On 2017/03/22 22:57:39, Devlin wrote: > |extension| can no longer be null, right? Can be we make it a const& to reflect > that? Done.
achuith@chromium.org changed reviewers: + skuhne@chromium.org
Stefan, could I get an owner stamp for profile_manager?
The CQ bit was checked by achuith@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.
profile_manager lgtm https://codereview.chromium.org/2149953002/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:75: switches::kEnableLoginScreenApps); You might consider to early out here if false.
Thanks Stefan! https://codereview.chromium.org/2149953002/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/chrome_process_manager_delegate.cc (right): https://codereview.chromium.org/2149953002/diff/480001/chrome/browser/extensi... chrome/browser/extensions/chrome_process_manager_delegate.cc:75: switches::kEnableLoginScreenApps); On 2017/03/23 14:38:37, Mr4D wrote: > You might consider to early out here if false. Done.
The CQ bit was checked by achuith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org, rdevlin.cronin@chromium.org, skuhne@chromium.org Link to the patchset: https://codereview.chromium.org/2149953002/#ps500001 (title: "Stefan feedback")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by achuith@chromium.org
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": 500001, "attempt_start_ts": 1490295553573810,
"parent_rev": "0c4e3a7bfd9d60ec8dbaa6bc947cdd2d790631a9", "commit_rev":
"d3da4f0a03a09f326f9e0ddf871bb268599ffc70"}
Message was sent while issue was closed.
Description was changed from ========== Enable login screen apps. ChromeProcessManagerDelegate: * Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed. * IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionService: * Do not load saved extensions in the ChromeOS signin profile. ProfileManager: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 ========== to ========== Enable login screen apps. ChromeProcessManagerDelegate: * Rename IsBackgroundPageAllowed into AreBackgroundPagesAllowedForContext and introduce IsExtensionBackgroundPageAllowed. * IsExtensionBackgroundPageAllowed allows background pages for apps in the ChromeOS signin profile when command-line switch --enable-login-screen-apps is set, limited to apps specified by device policy. ExtensionService: * Do not load saved extensions in the ChromeOS signin profile. ProfileManager: * Enable extensions in chromeos signin profile when command-line switch --enable-login-screen-apps is set. ExternalProviderImpl: * In CreateExternalProviders, create an ExternalPolicyLoader for login screen apps in the chromeos signin profile. * Switch --enable-login-screen-apps to enable apps in the chromeos signin profile. BUG=576464 Review-Url: https://codereview.chromium.org/2149953002 Cr-Commit-Position: refs/heads/master@{#459190} Committed: https://chromium.googlesource.com/chromium/src/+/d3da4f0a03a09f326f9e0ddf871b... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:500001) as https://chromium.googlesource.com/chromium/src/+/d3da4f0a03a09f326f9e0ddf871b... |
