Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241203003:
Pass user session type to extension feature checks (Closed)
|
Created:
4 years, 4 months ago by tbarzic Modified:
4 years, 3 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), extensions-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass user session type to extension feature checks
This is part of logic needed for adding an extension feature
parameter that makes the feature available depending on
session type (i.e. whether if current user is kiosk app).
The issue fixed here is determining the current session type, against which
a feature availability should be checked. Feature
availability checks are run both in browser and renderer process, but
session type is known only by browser. This means it has to be passed
to the renderer in order for feature availability to be correctly
determined.
This CL accomplishes that by sending the session type is to the
renderer by browser process as part of the renderer process start-up (
together with channel info, in ExtensionMsg_SetSessionInfo message).
BUG=606417
Committed: https://crrev.com/c34cf3c996bf1f088086690c93e61e00f0b640dd
Cr-Commit-Position: refs/heads/master@{#417686}
Patch Set 1 #Patch Set 2 : Add session type to simple extension features #Patch Set 3 : rebase #Patch Set 4 : Add session type to simple extension features #Patch Set 5 : Add session type to simple extension features #Patch Set 6 : Add session type to simple extension features #Patch Set 7 : Add session type to simple extension features #Patch Set 8 : split out some stuff #
Total comments: 6
Patch Set 9 : . #Patch Set 10 : . #Patch Set 11 : . #
Total comments: 4
Patch Set 12 : Pass user session type to extension renderer #Patch Set 13 : Pass user session type to extension renderer #
Total comments: 14
Patch Set 14 : Pass user session type to extension renderer #Patch Set 15 : Pass user session type to extension renderer #
Total comments: 4
Patch Set 16 : nits #
Total comments: 10
Patch Set 17 : . #
Total comments: 4
Patch Set 18 : . #
Total comments: 2
Patch Set 19 : Pass user session type to extension renderer #Dependent Patchsets: Messages
Total messages: 67 (45 generated)
The CQ bit was checked by tbarzic@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by tbarzic@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbarzic@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: Exceeded global retry quota
The CQ bit was checked by tbarzic@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 ========== Add session type to simple extension features BUG=606417 ========== to ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular' so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ==========
Description was changed from ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular' so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ========== to ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular'so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ==========
The CQ bit was checked by tbarzic@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 ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular'so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ========== to ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular'so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ==========
tbarzic@chromium.org changed reviewers: + meacer@chromium.org, rdevlin.cronin@chromium.org, rkc@chromium.org
meacer for the new IPC message (extensions/common/extension_messages.h)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add session type to simple extension features Adds a feature parameter that makes a feature available depending on whether current user session is for kiosk app or not (the former is called 'regular'so session types can be extended in the future with other types). The main issue here is getting the current session type to when determining feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer as well in order for this to work - to accomplish that, the session type is sent to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Current session type is determined in browser process from chromeos::LoginState info. Alternatively, user_manager could be used, but that would require adding dependency on components/user_manager to extensions/browser. Note that on non Chrome OS platforms, session type will always be 'regular'. BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determing the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). The CL adds a method to extensions/browser/extension_util which will be used to get current session type (real implementation to be added separatelly), and plumbs it to individual simple feature availability checks (actual logic for using it there to be added separatelly). Currently only kiosk and non-kiosk sessions are supported, but others may get added later. BUG=606417 ==========
On 2016/08/16 22:41:55, tbarzic wrote: > meacer for the new IPC message (extensions/common/extension_messages.h) On rkc's suggestion, I reduced the scope of the cl (in order to trim it down a bit) Logic for determining session type and logic for using session types in features will be added in a separate cl.
The CQ bit was checked by tbarzic@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/08/17 00:23:19, tbarzic wrote: > On 2016/08/16 22:41:55, tbarzic wrote: > > meacer for the new IPC message (extensions/common/extension_messages.h) > > On rkc's suggestion, I reduced the scope of the cl (in order to trim it down a > bit) > > Logic for determining session type and logic for using session types in features > will be added in a separate cl. sg, will circle back to this after https://codereview.chromium.org/2255613003/
Couple of high-level comments that should simplify the CL significantly. https://codereview.chromium.org/2241203003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2241203003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:563: extensions::util::GetCurrentSessionType())); This should be done in RendererStartupHelper, not here. https://codereview.chromium.org/2241203003/diff/140001/extensions/common/exte... File extensions/common/extension_api.h (right): https://codereview.chromium.org/2241203003/diff/140001/extensions/common/exte... extensions/common/extension_api.h:98: Feature::SessionType session_type, The session type can't really change, right? Why doesn't feature code determine what it is rather than having it passed it? https://codereview.chromium.org/2241203003/diff/140001/extensions/renderer/sc... File extensions/renderer/script_context.h (right): https://codereview.chromium.org/2241203003/diff/140001/extensions/renderer/sc... extensions/renderer/script_context.h:233: Feature::SessionType session_type_; This isn't really related to the context - it's global to the process. I'd rather we treat this more similarly to channel, rather than trying to curry it around everywhere.
The CQ bit was checked by tbarzic@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 ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determing the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). The CL adds a method to extensions/browser/extension_util which will be used to get current session type (real implementation to be added separatelly), and plumbs it to individual simple feature availability checks (actual logic for using it there to be added separatelly). Currently only kiosk and non-kiosk sessions are supported, but others may get added later. BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). The CL adds a method to extensions/browser/extension_util which will be used to get current session type (real implementation to be added separately), and makes it available to individual simple feature availability checks (actual logic for using it there to be added separately). Currently only kiosk and non-kiosk sessions are supported, but others may get added later. BUG=606417 ==========
https://codereview.chromium.org/2241203003/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (right): https://codereview.chromium.org/2241203003/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:563: extensions::util::GetCurrentSessionType())); On 2016/08/17 at 17:07:35, Devlin wrote: > This should be done in RendererStartupHelper, not here. Done https://codereview.chromium.org/2241203003/diff/140001/extensions/common/exte... File extensions/common/extension_api.h (right): https://codereview.chromium.org/2241203003/diff/140001/extensions/common/exte... extensions/common/extension_api.h:98: Feature::SessionType session_type, On 2016/08/17 at 17:07:35, Devlin wrote: > The session type can't really change, right? Why doesn't feature code determine what it is rather than having it passed it? It can change from UNSPECIFIED when a user logs in (so, effectively it should be constant; except in signin profile on ChromeOS, for which session type should not be important). https://codereview.chromium.org/2241203003/diff/140001/extensions/renderer/sc... File extensions/renderer/script_context.h (right): https://codereview.chromium.org/2241203003/diff/140001/extensions/renderer/sc... extensions/renderer/script_context.h:233: Feature::SessionType session_type_; On 2016/08/17 at 17:07:35, Devlin wrote: > This isn't really related to the context - it's global to the process. I'd rather we treat this more similarly to channel, rather than trying to curry it around everywhere. yeah, makes sense, I probably should have noticed that before :/
https://codereview.chromium.org/2241203003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2241203003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:80: extensions::util::GetCurrentSessionType())); This can be GetCurrentFeatureSessionType() instead. https://codereview.chromium.org/2241203003/diff/200001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/200001/extensions/common/feat... extensions/common/features/feature_session_type.h:18: FEATURE_SESSION_TYPE_REGULAR, We will definitely want to add more states later, so let's keep this more specific. UNSPECIFIED - means currently any state that we don't handle, like maybe login screen, tests, etc. REGULAR should instead be LOGGED_IN_USER. We can later add things like LOGIN_SCREEN, EPHEMERAL_USER, etc. Nit: s/UNSPECIFIED/UNKNOWN
tbarzic@chromium.org changed reviewers: + xiyuan@chromium.org
+xiyuan Also, updated dependent cl https://codereview.chromium.org/2241203003/diff/200001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2241203003/diff/200001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:80: extensions::util::GetCurrentSessionType())); On 2016/08/17 at 23:07:24, rkc wrote: > This can be GetCurrentFeatureSessionType() instead. Done. https://codereview.chromium.org/2241203003/diff/200001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/200001/extensions/common/feat... extensions/common/features/feature_session_type.h:18: FEATURE_SESSION_TYPE_REGULAR, On 2016/08/17 at 23:07:24, rkc wrote: > We will definitely want to add more states later, so let's keep this more specific. > UNSPECIFIED - means currently any state that we don't handle, like maybe login screen, tests, etc. > > REGULAR should instead be LOGGED_IN_USER. > We can later add things like LOGIN_SCREEN, EPHEMERAL_USER, etc. > > Nit: s/UNSPECIFIED/UNKNOWN Done. Kept REGULAR user per offline discussion.
muuuuch better, love <200 loc CLs! :) https://codereview.chromium.org/2241203003/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extensions_browser_client.cc (right): https://codereview.chromium.org/2241203003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_extensions_browser_client.cc:84: return FEATURE_SESSION_TYPE_REGULAR; indentation https://codereview.chromium.org/2241203003/diff/240001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2241203003/diff/240001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:75: // type is determined based on the user for which the current session is omit "The session type is determined..." - that's something that should be said (or immediately apparent) from GetCurrentFeatureSessionType(). https://codereview.chromium.org/2241203003/diff/240001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:79: process->Send( IPCs aren't free, and I think it's probably cheaper to just put this along with Channel, since they are used for very similar reasons and both always sent. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:11: const extensions::FeatureSessionType kDefaultSessionType = I think this variable is probably unnecessary. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:13: extensions::FeatureSessionType g_current_session_type = kDefaultSessionType; Just put the anonymous namespace inside the extensions namespace so we don't have extensions::. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:25: session_type == g_current_session_type); I'm assuming the motivation here is to not let the session type be set more than once? Maybe add a comment if that's the case? https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.h:15: enum FeatureSessionType { enum class https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.h:36: class ScopedCurrentFeatureSessionType { We can omit this class and just do: base::AutoReset<FeatureSessionType> ScopedCurrentFeatureSessionType(type); base::AutoReset<FeatureSessionType> ScopedCurrentFeatureSessionType(type) { return base::AutoReset<FeatureSessionType>(&g_session_type, type); }
Description was changed from ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). The CL adds a method to extensions/browser/extension_util which will be used to get current session type (real implementation to be added separately), and makes it available to individual simple feature availability checks (actual logic for using it there to be added separately). Currently only kiosk and non-kiosk sessions are supported, but others may get added later. BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Currently only kiosk and regular user sessions are supported, but others may get added later. BUG=606417 ==========
I realized that previous cl did not update session type when the log in state changes (instead it was done on browser process startup, which is too early). Changed this to set session type when active user gets set in user manager. https://codereview.chromium.org/2241203003/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extensions_browser_client.cc (right): https://codereview.chromium.org/2241203003/diff/240001/chrome/browser/extensi... chrome/browser/extensions/chrome_extensions_browser_client.cc:84: return FEATURE_SESSION_TYPE_REGULAR; On 2016/08/18 at 17:19:15, Devlin wrote: > indentation Done, but I realized I have to move this a bit later - it has to be set when a user logs in rather than when browser process starts up. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:11: const extensions::FeatureSessionType kDefaultSessionType = On 2016/08/18 at 17:19:15, Devlin wrote: > I think this variable is probably unnecessary. It's used to test whether the value has changed from the default one. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:13: extensions::FeatureSessionType g_current_session_type = kDefaultSessionType; On 2016/08/18 at 17:19:15, Devlin wrote: > Just put the anonymous namespace inside the extensions namespace so we don't have extensions::. Done. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.cc:25: session_type == g_current_session_type); On 2016/08/18 at 17:19:15, Devlin wrote: > I'm assuming the motivation here is to not let the session type be set more than once? Maybe add a comment if that's the case? Added a comment. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.h:15: enum FeatureSessionType { On 2016/08/18 at 17:19:15, Devlin wrote: > enum class Done. https://codereview.chromium.org/2241203003/diff/240001/extensions/common/feat... extensions/common/features/feature_session_type.h:36: class ScopedCurrentFeatureSessionType { On 2016/08/18 at 17:19:15, Devlin wrote: > We can omit this class and just do: > base::AutoReset<FeatureSessionType> ScopedCurrentFeatureSessionType(type); > > base::AutoReset<FeatureSessionType> ScopedCurrentFeatureSessionType(type) { > return base::AutoReset<FeatureSessionType>(&g_session_type, type); > } Done
The CQ bit was checked by tbarzic@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.
lgtm https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... extensions/common/features/feature_session_type.h:27: MAX = KIOSK nit: s/MAX/LAST
IPC Lgtm
lgtm https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... extensions/common/features/feature_session_type.h:11: #include "base/macros.h" nit: unused?
https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... extensions/common/features/feature_session_type.h:11: #include "base/macros.h" On 2016/08/18 at 21:13:46, xiyuan wrote: > nit: unused? yep, removed https://codereview.chromium.org/2241203003/diff/280001/extensions/common/feat... extensions/common/features/feature_session_type.h:27: MAX = KIOSK On 2016/08/18 at 20:35:10, rkc wrote: > nit: s/MAX/LAST Done
https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.cc:15: extensions::FeatureSessionType g_current_session_type = kDefaultSessionType; remove extensions:: https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.cc:32: return base::WrapUnique( prefer MakeUnique https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:19: // property. Hmm... this is a little weird. If the user account has any of the other session types (e.g. child) extension features should still be available. But we set it to UNKNOWN. So what would the default be for extension features? REGULAR or UNKNOWN? This also doesn't let us differentiate between "set and is child session" vs "not set". I think I'd rather see UNKNOWN, KIOSK, and something that encompasses the rest that are treated equally. https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:38: std::unique_ptr<base::AutoReset<FeatureSessionType>> Why is this wrapped in a std::unique_ptr? Will RVO not allow returning the base::AutoReset() directly?
https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.cc:15: extensions::FeatureSessionType g_current_session_type = kDefaultSessionType; On 2016/08/18 at 23:44:48, Devlin wrote: > remove extensions:: Done https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.cc:32: return base::WrapUnique( On 2016/08/18 at 23:44:48, Devlin wrote: > prefer MakeUnique Done https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:19: // property. On 2016/08/18 at 23:44:48, Devlin wrote: > Hmm... this is a little weird. If the user account has any of the other session types (e.g. child) extension features should still be available. But we set it to UNKNOWN. So what would the default be for extension features? REGULAR or UNKNOWN? This also doesn't let us differentiate between "set and is child session" vs "not set". > The default for extension features would be all - i.e. not being restricted by current session type. > I think I'd rather see UNKNOWN, KIOSK, and something that encompasses the rest that are treated equally. I considered that, but that would make semantics behind 'session_types': ['regular'] unstable (e.g. adding a new session type, like PUBLIC_SESSION, would reduce scope of features restricted to regular session type that existed before adding the new session type). https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:38: std::unique_ptr<base::AutoReset<FeatureSessionType>> On 2016/08/18 at 23:44:48, Devlin wrote: > Why is this wrapped in a std::unique_ptr? Will RVO not allow returning the base::AutoReset() directly? yep
https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:19: // property. On 2016/08/19 00:14:05, tbarzic wrote: > On 2016/08/18 at 23:44:48, Devlin wrote: > > Hmm... this is a little weird. If the user account has any of the other > session types (e.g. child) extension features should still be available. But we > set it to UNKNOWN. So what would the default be for extension features? > REGULAR or UNKNOWN? This also doesn't let us differentiate between "set and is > child session" vs "not set". > > > > The default for extension features would be all - i.e. not being restricted by > current session type. > > > I think I'd rather see UNKNOWN, KIOSK, and something that encompasses the rest > that are treated equally. > > I considered that, but that would make semantics behind 'session_types': > ['regular'] unstable (e.g. adding a new session type, like PUBLIC_SESSION, would > reduce scope of features restricted to regular session type that existed before > adding the new session type). This is fine for now, since we're fairly limited in scope, but I might want to re-address it later. In the meantime, I do think that having a distinction between "not set" and "something else" is important. https://codereview.chromium.org/2241203003/diff/320001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2241203003/diff/320001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:74: GetCurrentChannel(), GetCurrentFeatureSessionType())); I'm not familiar with where the login flow might happen, but is it possible that it wouldn't be properly set by the time a renderer is created? https://codereview.chromium.org/2241203003/diff/320001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/320001/extensions/common/feat... extensions/common/features/feature_session_type.cc:24: // Make sure that session type stays constants after it's been initialized. It's not really that it stays constant in the case of a ScopedCurrentFeatureSessionType going out of scope, right? e.g. in a test with multiple Scoped overrides? (Also, this check is another reason that the unset vs other distinction is important)
https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... File extensions/common/features/feature_session_type.h (right): https://codereview.chromium.org/2241203003/diff/300001/extensions/common/feat... extensions/common/features/feature_session_type.h:19: // property. On 2016/08/19 at 00:43:28, Devlin wrote: > On 2016/08/19 00:14:05, tbarzic wrote: > > On 2016/08/18 at 23:44:48, Devlin wrote: > > > Hmm... this is a little weird. If the user account has any of the other > > session types (e.g. child) extension features should still be available. But we > > set it to UNKNOWN. So what would the default be for extension features? > > REGULAR or UNKNOWN? This also doesn't let us differentiate between "set and is > > child session" vs "not set". > > > > > > > The default for extension features would be all - i.e. not being restricted by > > current session type. > > > > > I think I'd rather see UNKNOWN, KIOSK, and something that encompasses the rest > > that are treated equally. > > > > I considered that, but that would make semantics behind 'session_types': > > ['regular'] unstable (e.g. adding a new session type, like PUBLIC_SESSION, would > > reduce scope of features restricted to regular session type that existed before > > adding the new session type). > > This is fine for now, since we're fairly limited in scope, but I might want to re-address it later. In the meantime, I do think that having a distinction between "not set" and "something else" is important. I'm not sure I completely agree that distinguishing two states that are not that relevant for features is that important (especially if the initial state is considered 'user-less' session rather than session-less state), but I don't see drawback of adding extra state. https://codereview.chromium.org/2241203003/diff/320001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2241203003/diff/320001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:74: GetCurrentChannel(), GetCurrentFeatureSessionType())); On 2016/08/19 at 00:43:28, Devlin wrote: > I'm not familiar with where the login flow might happen, but is it possible that it wouldn't be properly set by the time a renderer is created? It's done during profile creation, so it should be done by the time renderers are created. https://codereview.chromium.org/2241203003/diff/320001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/320001/extensions/common/feat... extensions/common/features/feature_session_type.cc:24: // Make sure that session type stays constants after it's been initialized. On 2016/08/19 at 00:43:28, Devlin wrote: > It's not really that it stays constant in the case of a ScopedCurrentFeatureSessionType going out of scope, right? e.g. in a test with multiple Scoped overrides? > > (Also, this check is another reason that the unset vs other distinction is important) Yeah, it may be different in tests, but that's kinda the point of ScopedCurrenteaturesSessionType.
lgtm % nit. Also, your cl description should be updated. https://codereview.chromium.org/2241203003/diff/340001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/340001/extensions/common/feat... extensions/common/features/feature_session_type.cc:24: // Make sure that session type stays constants after it's been initialized. typo nit: stays constant
Description was changed from ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type when checking feature availability (see simple_features.cc). Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up (newly introduced ExtensionMsg_SetUserSessionType IPC message). Currently only kiosk and regular user sessions are supported, but others may get added later. BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type, against which a feature availability should be checked. Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up. BUG=606417 ==========
Description was changed from ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type, against which a feature availability should be checked. Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up. BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type, against which a feature availability should be checked. Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up ( together with channel info, in ExtensionMsg_SetSessionInfo message). BUG=606417 ==========
The CQ bit was checked by tbarzic@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/2241203003/diff/340001/extensions/common/feat... File extensions/common/features/feature_session_type.cc (right): https://codereview.chromium.org/2241203003/diff/340001/extensions/common/feat... extensions/common/features/feature_session_type.cc:24: // Make sure that session type stays constants after it's been initialized. On 2016/08/22 at 19:24:59, Devlin wrote: > typo nit: stays constant 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 tbarzic@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, xiyuan@chromium.org, meacer@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2241203003/#ps360001 (title: "Pass user session type to extension renderer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type, against which a feature availability should be checked. Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up ( together with channel info, in ExtensionMsg_SetSessionInfo message). BUG=606417 ========== to ========== Pass user session type to extension feature checks This is part of logic needed for adding an extension feature parameter that makes the feature available depending on session type (i.e. whether if current user is kiosk app). The issue fixed here is determining the current session type, against which a feature availability should be checked. Feature availability checks are run both in browser and renderer process, but session type is known only by browser. This means it has to be passed to the renderer in order for feature availability to be correctly determined. This CL accomplishes that by sending the session type is to the renderer by browser process as part of the renderer process start-up ( together with channel info, in ExtensionMsg_SetSessionInfo message). BUG=606417 Committed: https://crrev.com/c34cf3c996bf1f088086690c93e61e00f0b640dd Cr-Commit-Position: refs/heads/master@{#417686} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/c34cf3c996bf1f088086690c93e61e00f0b640dd Cr-Commit-Position: refs/heads/master@{#417686} |
