Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255613003:
Introduce session type parameter to extension features (Closed)
|
Created:
4 years, 4 months ago by tbarzic Modified:
4 years, 3 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce session type parameter to extension features
This adds session type to extension features grammar. The session
type refers to the type of user active in the session in which
an extension is running. The parameter will enable making a feature
available depending on type of the session in which the extension/app
is running.
Feature session types are declared as list of individual session types
in which a feature is available (undefined list means no restrictions).
Currently, values supported for session type are as follows:
* 'kiosk' - kiosk app session
* 'regular' - non-kiosk user session
The list of supported values is expected to be expanded in future.
Depends on https://codereview.chromium.org/2241203003/ for plumbing
of current session type to feature availability checks
BUG=606417
Committed: https://crrev.com/64bf38deace6fc500ab416cae16f9d3ea0629411
Cr-Commit-Position: refs/heads/master@{#418938}
Patch Set 1 #Patch Set 2 : Introduce session type parameter to extension features #
Total comments: 1
Patch Set 3 : Introduce session type parameter to extension features #Patch Set 4 : Introduce session type parameter to extension features #Patch Set 5 : Introduce session type parameter to extension features #Patch Set 6 : Introduce session type parameter to extension features #
Total comments: 15
Patch Set 7 : . #Patch Set 8 : Introduce session type parameter to extension features #Patch Set 9 : . #Patch Set 10 : Introduce session type parameter to extension features #
Total comments: 17
Patch Set 11 : Introduce session type parameter to extension features #Patch Set 12 : Introduce session type parameter to extension features #
Total comments: 10
Patch Set 13 : feedback #Patch Set 14 : . #Patch Set 15 : Introduce session type parameter to extension features #Patch Set 16 : win build warning #Patch Set 17 : Introduce session type parameter to extension features #
Total comments: 4
Patch Set 18 : Introduce session type parameter to extension features #
Total comments: 6
Patch Set 19 : . #Patch Set 20 : Introduce session type parameter to extension features #Messages
Total messages: 55 (34 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...
tbarzic@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
tbarzic@chromium.org changed reviewers: + rkc@chromium.org
Which of these CLs comes first? Can you add one as a dependent of the other to make it more clear? https://codereview.chromium.org/2255613003/diff/20001/chrome/common/extension... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/20001/chrome/common/extension... chrome/common/extensions/api/common_extension_api_unittest.cc:194: Feature::SESSION_TYPE_UNSPECIFIED, GURL()}, Does this CL need to be rebased? This seems different from master.
On 2016/08/17 16:51:07, Devlin wrote: > Which of these CLs comes first? > > Can you add one as a dependent of the other to make it more clear? > > https://codereview.chromium.org/2255613003/diff/20001/chrome/common/extension... > File chrome/common/extensions/api/common_extension_api_unittest.cc (right): > > https://codereview.chromium.org/2255613003/diff/20001/chrome/common/extension... > chrome/common/extensions/api/common_extension_api_unittest.cc:194: > Feature::SESSION_TYPE_UNSPECIFIED, GURL()}, > Does this CL need to be rebased? This seems different from master. https://codereview.chromium.org/2241203003/ comes first
Description was changed from ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Plumbing of current session type to feature availability checks added in https://codereview.chromium.org/2241203003/ BUG=606417 ========== to ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Depends on https://codereview.chromium.org/2241203003/ for plumbing of current session type to feature availability checks BUG=606417 ==========
tbarzic@chromium.org changed reviewers: + xiyuan@chromium.org
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm just nits https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:316: for (size_t i = 0; i < arraysize(test_data); ++i) { Change test data to a vector (or std::array) then use for (const auto& test: test_data) { ... } https://codereview.chromium.org/2255613003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:103: "session_types": ["regular"] Can we add a test for no session type, and both session types specified? i.e., "session_types": [] and "session_types": ["kiosk", "regular"] It would be nice if we could also add a few negative tests. Something like "session_types": ["garbage"] https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:471: for (size_t i = 0; i < arraysize(test_data); ++i) { Make test_data a vector, then use, for (const auto& test : test_data) { ... }
lgtm https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:301: struct { nit: const https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:305: } test_data[]{{"kiosk_only", true, FeatureSessionType::KIOSK}, nit: } kTestData[] = {... https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:316: for (size_t i = 0; i < arraysize(test_data); ++i) { On 2016/08/18 20:35:28, rkc wrote: > Change test data to a vector (or std::array) then use for (const auto& test: > test_data) { > ... > } Think this is fine as many tests are using test data this way, e.g. https://cs.chromium.org/chromium/src/net/base/parse_number_unittest.cc?rcl=0&... https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:421: struct { nit: const https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:426: } test_data[] = {{"kiosk_feature in kiosk session", nit: test_data -> kTestData
https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:301: struct { On 2016/08/18 at 21:24:00, xiyuan wrote: > nit: const Done https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:305: } test_data[]{{"kiosk_only", true, FeatureSessionType::KIOSK}, On 2016/08/18 at 21:24:00, xiyuan wrote: > nit: } kTestData[] = {... Done. https://codereview.chromium.org/2255613003/diff/100001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:316: for (size_t i = 0; i < arraysize(test_data); ++i) { On 2016/08/18 at 20:35:28, rkc wrote: > Change test data to a vector (or std::array) then use for (const auto& test: test_data) { > ... > } Done https://codereview.chromium.org/2255613003/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:103: "session_types": ["regular"] On 2016/08/18 at 20:35:28, rkc wrote: > Can we add a test for no session type, and both session types specified? > i.e., "session_types": [] and "session_types": ["kiosk", "regular"] > > It would be nice if we could also add a few negative tests. Something like "session_types": ["garbage"] Done https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:421: struct { On 2016/08/18 at 21:24:00, xiyuan wrote: > nit: const Done. https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:426: } test_data[] = {{"kiosk_feature in kiosk session", On 2016/08/18 at 21:24:00, xiyuan wrote: > nit: test_data -> kTestData Done. https://codereview.chromium.org/2255613003/diff/100001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:471: for (size_t i = 0; i < arraysize(test_data); ++i) { On 2016/08/18 at 20:35:28, rkc wrote: > Make test_data a vector, then use, > for (const auto& test : test_data) { > ... > } Done.
Also please update the documentation at c/c/extensions/api/_features.md. https://codereview.chromium.org/2255613003/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/180001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:320: {"invalid_session_types", false, FeatureSessionType::KIOSK}, Does this feature exist? https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:110: "all_session_types": { "all" is doomed to be obsoleted, unless it uses "all". :) https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:113: "session_types": ["kiosk", "regular"] How would one specify "unknown"? https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.cc:338: void SimpleFeature::Parse(const base::DictionaryValue* dictionary) { This badly needs to be rebased. https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.cc:571: "'%s' is not allowed in %s session", name().c_str(), Let's structure this in the fashion of the other warnings https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.h:232: bool session_types_set_; I'd prefer we handle this differently, e.g. by not allowing an empty list of session types - in which case !empty() signifies set. https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:45: bool ignore_session_types; Why do we need this? It seems like we should never be ignoring session types. https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:419: manifest.SetInteger("manifest_version", 21); ? This didn't throw an error?
https://codereview.chromium.org/2255613003/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/180001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:320: {"invalid_session_types", false, FeatureSessionType::KIOSK}, On 2016/08/23 at 20:49:06, Devlin wrote: > Does this feature exist? no, removed https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:110: "all_session_types": { On 2016/08/23 at 20:49:06, Devlin wrote: > "all" is doomed to be obsoleted, unless it uses "all". :) yeah, I agree, fixed https://codereview.chromium.org/2255613003/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:113: "session_types": ["kiosk", "regular"] On 2016/08/23 at 20:49:06, Devlin wrote: > How would one specify "unknown"? it's not possible, by design - "unknown" session type is used internally a a value for session type against which feature session type should be matched in order to determine feature availability (it represents a session type that cannot be declared in "session_types" list) https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.cc:571: "'%s' is not allowed in %s session", name().c_str(), On 2016/08/23 at 20:49:06, Devlin wrote: > Let's structure this in the fashion of the other warnings Isn't it already (except of missing trailing .)? https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.h (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.h:232: bool session_types_set_; On 2016/08/23 at 20:49:06, Devlin wrote: > I'd prefer we handle this differently, e.g. by not allowing an empty list of session types - in which case !empty() signifies set. Done. https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:45: bool ignore_session_types; On 2016/08/23 at 20:49:06, Devlin wrote: > Why do we need this? It seems like we should never be ignoring session types. it was named a bit poorly, but it's used to distinguish empty session_types and unset session_types when setting up test, not needed any more since empty_session type is not a valid value, and essentially gets ignored. https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:419: manifest.SetInteger("manifest_version", 21); On 2016/08/23 at 20:49:06, Devlin wrote: > ? This didn't throw an error? no
https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.cc:571: "'%s' is not allowed in %s session", name().c_str(), On 2016/09/08 18:48:55, tbarzic wrote: > On 2016/08/23 at 20:49:06, Devlin wrote: > > Let's structure this in the fashion of the other warnings > > Isn't it already (except of missing trailing .)? Follow the style of e.g. invalid context errors so that developers know which sessions something is available in. https://codereview.chromium.org/2255613003/diff/220001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/220001/chrome/common/extensio... chrome/common/extensions/api/_features.md:262: The accepted value is a list of strings from `regular`, `kiosk`. Empty list is nit: The accepted values are lists of strings from... Omit "empty list is ignored." Empty list should not be allowed, since it would imply features are not usable anywhere. https://codereview.chromium.org/2255613003/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:113: "session_types": [] This should be disallowed. https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:139: default: Prefer not using default so that future additions generate compile errors. https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:410: TEST_F(SimpleFeatureTest, SessionType) { Does this test add significant benefit over the other?
https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/180001/extensions/common/feat... extensions/common/features/simple_feature.cc:571: "'%s' is not allowed in %s session", name().c_str(), On 2016/09/12 22:33:07, Devlin wrote: > On 2016/09/08 18:48:55, tbarzic wrote: > > On 2016/08/23 at 20:49:06, Devlin wrote: > > > Let's structure this in the fashion of the other warnings > > > > Isn't it already (except of missing trailing .)? > > Follow the style of e.g. invalid context errors so that developers know which > sessions something is available in. Done. https://codereview.chromium.org/2255613003/diff/220001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/220001/chrome/common/extensio... chrome/common/extensions/api/_features.md:262: The accepted value is a list of strings from `regular`, `kiosk`. Empty list is On 2016/09/12 22:33:08, Devlin wrote: > nit: The accepted values are lists of strings from... > > Omit "empty list is ignored." Empty list should not be allowed, since it would > imply features are not usable anywhere. Done. https://codereview.chromium.org/2255613003/diff/220001/chrome/test/data/exten... File chrome/test/data/extensions/extension_api_unittest/api_features.json (right): https://codereview.chromium.org/2255613003/diff/220001/chrome/test/data/exten... chrome/test/data/extensions/extension_api_unittest/api_features.json:113: "session_types": [] On 2016/09/12 22:33:08, Devlin wrote: > This should be disallowed. Done. https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature.cc:139: default: On 2016/09/12 22:33:08, Devlin wrote: > Prefer not using default so that future additions generate compile errors. Done. https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:410: TEST_F(SimpleFeatureTest, SessionType) { On 2016/09/12 22:33:08, Devlin wrote: > Does this test add significant benefit over the other? yeah, it tests Feature::IsAvailableToContext rather than ExtensionAPI::IsAvailable (former is called by later, though). Plus, IsAvailableToManifest is not included in other test at all. (note that the other test includes parsing features' json file, instead of dealing with FeatureSessionType enums directly).
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: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
just a few last nits https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:410: TEST_F(SimpleFeatureTest, SessionType) { On 2016/09/12 23:05:10, tbarzic wrote: > On 2016/09/12 22:33:08, Devlin wrote: > > Does this test add significant benefit over the other? > > yeah, it tests Feature::IsAvailableToContext rather than > ExtensionAPI::IsAvailable (former is called by later, though). > Plus, IsAvailableToManifest is not included in other test at all. > > (note that the other test includes parsing features' json file, instead of > dealing with FeatureSessionType enums directly). IsAvailable() calls IsAvailableToContext() which calls IsAvailableToManifest(), so I'm a little dubious about this. But unittests are cheap, so if you want to keep it, that's fine. https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... chrome/common/extensions/api/_features.md:260: logged in the current session. What happens if the user is not logged in? Specifying a 'kiosk' session type on non-chromeos doesn't make sense, right? (We should mention that.) Does specifying a regular session type on non-chromeos make sense? https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:321: api.IsAvailable(test.api_name, NULL, nit nullptr in new code.
https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... File extensions/common/features/simple_feature_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/220001/extensions/common/feat... extensions/common/features/simple_feature_unittest.cc:410: TEST_F(SimpleFeatureTest, SessionType) { On 2016/09/13 16:46:23, Devlin wrote: > On 2016/09/12 23:05:10, tbarzic wrote: > > On 2016/09/12 22:33:08, Devlin wrote: > > > Does this test add significant benefit over the other? > > > > yeah, it tests Feature::IsAvailableToContext rather than > > ExtensionAPI::IsAvailable (former is called by later, though). > > Plus, IsAvailableToManifest is not included in other test at all. > > > > (note that the other test includes parsing features' json file, instead of > > dealing with FeatureSessionType enums directly). > > IsAvailable() calls IsAvailableToContext() which calls IsAvailableToManifest(), > so I'm a little dubious about this. But unittests are cheap, so if you want to > keep it, that's fine. IsAvailableToContext does not call into IsAvailableToManifest if extension is not set (which is the case in common_extension_api_unittest_). https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... chrome/common/extensions/api/_features.md:260: logged in the current session. On 2016/09/13 16:46:23, Devlin wrote: > What happens if the user is not logged in? Specifying a 'kiosk' session type on > non-chromeos doesn't make sense, right? (We should mention that.) Does > specifying a regular session type on non-chromeos make sense? They currently don't make sense on non-chromeos, I'll mention that. (we might eventually want to support 'regular' user on non-chromeos, though - if a need arises). https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... File chrome/common/extensions/api/common_extension_api_unittest.cc (right): https://codereview.chromium.org/2255613003/diff/320001/chrome/common/extensio... chrome/common/extensions/api/common_extension_api_unittest.cc:321: api.IsAvailable(test.api_name, NULL, On 2016/09/13 16:46:23, Devlin wrote: > nit nullptr in new code. Done.
lgtm https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:261: are only supported on Chrome OS - feature restricted to set of session types nit: s/features/feature https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:263: session types imply that a user is logged into the session. Mention what the value is when the user is *not* logged in? Also, this still makes me a little nervous, because it means things like the following break: feature: { session_types: [regular] } feature.child: { session_types: [???] // How do I get all session types? } I'm okay addressing that at a later date, but keep it in mind.
https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:261: are only supported on Chrome OS - feature restricted to set of session types On 2016/09/13 17:37:45, Devlin wrote: > nit: s/features/feature Done. https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:263: session types imply that a user is logged into the session. On 2016/09/13 17:37:45, Devlin wrote: > Mention what the value is when the user is *not* logged in? > That one is not currently supported (as there is no need to enable an API only when a user is not logged in). (except for handful of exceptions (Chromevox and easy unlock app, afaik), extensions are not allowed before a user is logged in on Chrome OS) > Also, this still makes me a little nervous, because it means things like the > following break: > feature: { > session_types: [regular] > } > feature.child: { > session_types: [???] // How do I get all session types? > } > > I'm okay addressing that at a later date, but keep it in mind. If this has to be supported, we could add another enum value 'all', or maybe set the value to session_types: <all> (this could apply to other feature properties, e.g. platforms)
https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:263: session types imply that a user is logged into the session. On 2016/09/13 17:55:12, tbarzic wrote: > On 2016/09/13 17:37:45, Devlin wrote: > > Mention what the value is when the user is *not* logged in? > > > > That one is not currently supported (as there is no need to enable an API only > when a user is not logged in). > (except for handful of exceptions (Chromevox and easy unlock app, afaik), > extensions are not allowed before a user is logged in on Chrome OS) I understand we don't offer it as a value, but it's important to note that if a user is not logged in, it will not be considered any of the session types. This is somewhat implied by the last sentence already, but I'd like it to be explicit. > > Also, this still makes me a little nervous, because it means things like the > > following break: > > feature: { > > session_types: [regular] > > } > > feature.child: { > > session_types: [???] // How do I get all session types? > > } > > > > I'm okay addressing that at a later date, but keep it in mind. > > If this has to be supported, we could add another enum value 'all', or maybe set > the value to session_types: <all> (this could apply to other feature properties, > e.g. platforms) Many properties support all - e.g. contexts. The difference is that "all" for something like platforms just encompasses the platforms that can be specified already - i.e., I can specify win, linux, chromeos, and mac. For session types, that's not the case. And I think it's very confusing for 'all' to imply more than the collection of available enums.
https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2255613003/diff/340001/chrome/common/extensio... chrome/common/extensions/api/_features.md:263: session types imply that a user is logged into the session. On 2016/09/13 18:13:50, Devlin wrote: > On 2016/09/13 17:55:12, tbarzic wrote: > > On 2016/09/13 17:37:45, Devlin wrote: > > > Mention what the value is when the user is *not* logged in? > > > > > > > That one is not currently supported (as there is no need to enable an API only > > when a user is not logged in). > > (except for handful of exceptions (Chromevox and easy unlock app, afaik), > > extensions are not allowed before a user is logged in on Chrome OS) > > I understand we don't offer it as a value, but it's important to note that if a > user is not logged in, it will not be considered any of the session types. This > is somewhat implied by the last sentence already, but I'd like it to be > explicit. Done. > > > > Also, this still makes me a little nervous, because it means things like the > > > following break: > > > feature: { > > > session_types: [regular] > > > } > > > feature.child: { > > > session_types: [???] // How do I get all session types? > > > } > > > > > > I'm okay addressing that at a later date, but keep it in mind. > > > > If this has to be supported, we could add another enum value 'all', or maybe > set > > the value to session_types: <all> (this could apply to other feature > properties, > > e.g. platforms) > > Many properties support all - e.g. contexts. The difference is that "all" for > something like platforms just encompasses the platforms that can be specified > already - i.e., I can specify win, linux, chromeos, and mac. For session types, > that's not the case. And I think it's very confusing for 'all' to imply more > than the collection of available enums. Ack
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, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2255613003/#ps380001 (title: "Introduce session type parameter to extension features")
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.
Description was changed from ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Depends on https://codereview.chromium.org/2241203003/ for plumbing of current session type to feature availability checks BUG=606417 ========== to ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Depends on https://codereview.chromium.org/2241203003/ for plumbing of current session type to feature availability checks BUG=606417 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Depends on https://codereview.chromium.org/2241203003/ for plumbing of current session type to feature availability checks BUG=606417 ========== to ========== Introduce session type parameter to extension features This adds session type to extension features grammar. The session type refers to the type of user active in the session in which an extension is running. The parameter will enable making a feature available depending on type of the session in which the extension/app is running. Feature session types are declared as list of individual session types in which a feature is available (undefined list means no restrictions). Currently, values supported for session type are as follows: * 'kiosk' - kiosk app session * 'regular' - non-kiosk user session The list of supported values is expected to be expanded in future. Depends on https://codereview.chromium.org/2241203003/ for plumbing of current session type to feature availability checks BUG=606417 Committed: https://crrev.com/64bf38deace6fc500ab416cae16f9d3ea0629411 Cr-Commit-Position: refs/heads/master@{#418938} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/64bf38deace6fc500ab416cae16f9d3ea0629411 Cr-Commit-Position: refs/heads/master@{#418938} |
