Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686233006:
Add managed device signal (Closed)
|
Created:
3 years, 10 months ago by Wenzhao (Colin) Zang Modified:
3 years, 10 months ago Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd managed device signal (managed or not managed). It'll be used by the Get Help app.
Use fake install attributes for browser tests.
BUG=682837
Review-Url: https://codereview.chromium.org/2686233006
Cr-Commit-Position: refs/heads/master@{#452193}
Committed: https://chromium.googlesource.com/chromium/src/+/3777167162feb5b4fbd24e8386e92ae92fe354c8
Patch Set 1 #Patch Set 2 : Add browser tests #Patch Set 3 : Change comments #Patch Set 4 : Use InstallAttributes #Patch Set 5 : Minor comments change #
Total comments: 18
Patch Set 6 : Add callback #
Total comments: 5
Patch Set 7 : Minor changes in comments #
Messages
Total messages: 48 (27 generated)
Description was changed from ========== Add managed device signal, which has three values: active, unmanaged and deprovisioned. It'll be used by the Get Help app. BUG=682837 ========== to ========== Add managed device signal It has three values: active, unmanaged and deprovisioned. It'll be used by the Get Help app. BUG=682837 ==========
Description was changed from ========== Add managed device signal It has three values: active, unmanaged and deprovisioned. It'll be used by the Get Help app. BUG=682837 ========== to ========== Add managed device signal, which has three values: active, unmanaged and deprovisioned. It'll be used by the Get Help app. BUG=682837 ==========
wzang@chromium.org changed reviewers: + alemate@chromium.org
Description was changed from ========== Add managed device signal, which has three values: active, unmanaged and deprovisioned. It'll be used by the Get Help app. BUG=682837 ========== to ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Need ideas on how to write browser tests. The test case should be able to update and fetch policy, and is a subclass of ExtensionApiTest. Have tried something similar to cloud_policy_browsertest.cc but it didn't work with the extension api test setup. It'd be much appreciated if you can suggest on this. BUG=682837 ==========
Description was changed from ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Need ideas on how to write browser tests. The test case should be able to update and fetch policy, and is a subclass of ExtensionApiTest. Have tried something similar to cloud_policy_browsertest.cc but it didn't work with the extension api test setup. It'd be much appreciated if you can suggest on this. BUG=682837 ========== to ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Need ideas on how to write browser tests. The test class should be able to update and fetch policy, and is a subclass of ExtensionApiTest. Have tried something similar to CloudPolicyTest in cloud_policy_browsertest.cc but it didn't work with the extension api test setup. It'd be much appreciated if you can suggest on this. BUG=682837 ==========
On 2017/02/13 19:21:49, Wenzhao (Colin) Zang wrote: > Description was changed from > > ========== > Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by > the Get Help app. > > Need ideas on how to write browser tests. The test case should be able to update > and fetch policy, and is a subclass of ExtensionApiTest. Have tried something > similar to cloud_policy_browsertest.cc but it didn't work with the extension api > test setup. > > It'd be much appreciated if you can suggest on this. > > BUG=682837 > ========== > > to > > ========== > Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by > the Get Help app. > > Need ideas on how to write browser tests. The test class should be able to > update and fetch policy, and is a subclass of ExtensionApiTest. Have tried > something similar to CloudPolicyTest in cloud_policy_browsertest.cc but it > didn't work with the extension api test setup. > > It'd be much appreciated if you can suggest on this. > > BUG=682837 > ========== Looks good, but messages to reviewers should be placed in comments. Otherwise this full text will go to CQ.
On 2017/02/14 12:16:20, Alexander Alekseev wrote: > On 2017/02/13 19:21:49, Wenzhao (Colin) Zang wrote: > > Description was changed from > > > > ========== > > Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used > by > > the Get Help app. > > > > Need ideas on how to write browser tests. The test case should be able to > update > > and fetch policy, and is a subclass of ExtensionApiTest. Have tried something > > similar to cloud_policy_browsertest.cc but it didn't work with the extension > api > > test setup. > > > > It'd be much appreciated if you can suggest on this. > > > > BUG=682837 > > ========== > > > > to > > > > ========== > > Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used > by > > the Get Help app. > > > > Need ideas on how to write browser tests. The test class should be able to > > update and fetch policy, and is a subclass of ExtensionApiTest. Have tried > > something similar to CloudPolicyTest in cloud_policy_browsertest.cc but it > > didn't work with the extension api test setup. > > > > It'd be much appreciated if you can suggest on this. > > > > BUG=682837 > > ========== > > Looks good, but messages to reviewers should be placed in comments. > Otherwise this full text will go to CQ. There are browser tests that inherit from ExtensionApiTest and exercise the policy subsystem. See for example this search: https://cs.chromium.org/search/?q=ExtensionApiTest+policy+package:%5Echromium...
Description was changed from ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Need ideas on how to write browser tests. The test class should be able to update and fetch policy, and is a subclass of ExtensionApiTest. Have tried something similar to CloudPolicyTest in cloud_policy_browsertest.cc but it didn't work with the extension api test setup. It'd be much appreciated if you can suggest on this. BUG=682837 ========== to ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Add a policy_data setter in DeviceSettingsService, and mark it as used only by tests. It is necessary for this test because policy fetch is beyond the scope of this CL and has already been tested in multiple unit tests. BUG=682837 ==========
wzang@chromium.org changed reviewers: + tnagel@chromium.org
Description was changed from ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED. It'll be used by the Get Help app. Add a policy_data setter in DeviceSettingsService, and mark it as used only by tests. It is necessary for this test because policy fetch is beyond the scope of this CL and has already been tested in multiple unit tests. BUG=682837 ========== to ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED). It'll be used by the Get Help app. Add a policy_data setter in DeviceSettingsService, and mark it as used only by tests. It is necessary for this test because policy fetch is beyond the scope of this CL and has already been tested in multiple unit tests. BUG=682837 ==========
wzang@chromium.org changed reviewers: - alemate@chromium.org
Hi Thiemo, this is Colin from MTV. Sorry for the short notice but would you please take a look at this CL? Because this is part of M58 which has feature freeze tomorrow. Thanks so much.
wzang@chromium.org changed reviewers: + alemate@chromium.org
The canonical source of enrollment information is InstallAttributes. I'd suggest to use InstallAttributes unless you've got a compelling reason against it. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/settings/install...
wzang@chromium.org changed reviewers: - alemate@chromium.org
Thanks. I've switched to InstallAttributes, and it no longer requires your review. Thanks so much.
Description was changed from ========== Add managed device signal (ACTIVE, UNMANAGED and DEPROVISIONED). It'll be used by the Get Help app. Add a policy_data setter in DeviceSettingsService, and mark it as used only by tests. It is necessary for this test because policy fetch is beyond the scope of this CL and has already been tested in multiple unit tests. BUG=682837 ========== to ========== Add managed device signal (managed or not managed). It'll be used by the Get Help app. Use fake install attributes for browser tests. BUG=682837 ==========
wzang@chromium.org changed reviewers: + alemate@chromium.org, steel@chromium.org - tnagel@chromium.org
This is part of M58, please take a look. Thanks.
lgtm
wzang@chromium.org changed reviewers: - steel@chromium.org
The CQ bit was checked by wzang@chromium.org to run a CQ dry run
steel@chromium.org changed reviewers: + steel@chromium.org
https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_apitest.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:136: IN_PROC_BROWSER_TEST_F(ChromeOSManagedDeviceInfoPrivateTest, Managed) { Nit: You want to test all the code paths here. This only checks if a device is managed so you should add a check for unmanaged also.
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/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_apitest.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:136: IN_PROC_BROWSER_TEST_F(ChromeOSManagedDeviceInfoPrivateTest, Managed) { On 2017/02/22 00:00:13, Rahul Chaturvedi wrote: > Nit: You want to test all the code paths here. This only checks if a device is > managed so you should add a check for unmanaged also. The unmanaged value is default and the code path has been covered by TestGetAndSet, and the unmanaged test requires different setup (i.e. shouldn't have policy::BrowserPolicyConnectorChromeOS::SetInstallAttributesForTesting()), so it has to be put in a different class I think? Because we cannot reverse what has been done in SetUpInProcessBrowserTestFixture()? I tried putting the setup in the test body but it didn't work.
wzang@chromium.org changed reviewers: + mkearney@chromium.org, mkwst@chromium.org, tbarzic@chromium.org
This is part of M58, please take a look at your earlier conveience. Thanks.
wzang@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - mkearney@chromium.org, mkwst@chromium.org
This is part of M58, please take a look at your earliest convenience. Thanks. @rdevlin.cronin Please see chrome/common/extensions/api/chromeos_info_private.json @tbarzic Please see the rest of the files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_api.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:141: // Key which corresponds to the "not mananged" value of the ManagedDeviceStatus nit: This is not really a key - it's a managedDeviceStatus property value indicating an unmanaged device. https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:145: // Key which corresponds to the "managed" value of the ManagedDeviceStatus ditto https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:287: } else { nit: no else after return, and no {} https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_apitest.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:6: #include "chrome/browser/browser_process.h" nit: this is not needed, is it? https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:133: } nit: add DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/chromeos_info_private/basic/background.js (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/basic/background.js:26: // ManagedDeviceStatus by default should be not managed. nit: I'd remove this comment - it doesn't add too much value https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:14: function(values) { You can use chrome.test.callbackPass(function(values {...}); here (in which case chrome.test.succeed is not needed at the end) https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:17: } else if (testName == 'arc not-available') { nit: maybe switch this to switch {case:}
https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_api.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:141: // Key which corresponds to the "not mananged" value of the ManagedDeviceStatus On 2017/02/22 02:18:35, tbarzic wrote: > nit: This is not really a key - it's a managedDeviceStatus property value > indicating an unmanaged device. Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:145: // Key which corresponds to the "managed" value of the ManagedDeviceStatus On 2017/02/22 02:18:35, tbarzic wrote: > ditto Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_api.cc:287: } else { On 2017/02/22 02:18:35, tbarzic wrote: > nit: no else after return, and no {} Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/info_private_apitest.cc (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:6: #include "chrome/browser/browser_process.h" On 2017/02/22 02:18:35, tbarzic wrote: > nit: this is not needed, is it? Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/info_private_apitest.cc:133: } On 2017/02/22 02:18:35, tbarzic wrote: > nit: add DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/chromeos_info_private/basic/background.js (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/basic/background.js:26: // ManagedDeviceStatus by default should be not managed. On 2017/02/22 02:18:35, tbarzic wrote: > nit: I'd remove this comment - it doesn't add too much value Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js (right): https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:14: function(values) { On 2017/02/22 02:18:35, tbarzic wrote: > You can use chrome.test.callbackPass(function(values {...}); here > (in which case chrome.test.succeed is not needed at the end) Done. https://codereview.chromium.org/2686233006/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:17: } else if (testName == 'arc not-available') { On 2017/02/22 02:18:35, tbarzic wrote: > nit: maybe switch this to switch {case:} Done.
lgtm https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/info_private_api.cc (right): https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/info_private_api.cc:141: // Property indicating an unmanaged device, which corresponds to the "not how about: "Value to which managedDeviceStatus property is set for unmanaged devices." https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/info_private_api.cc:285: if (connector->IsEnterpriseManaged()) { nit: no {} here https://codereview.chromium.org/2686233006/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js (right): https://codereview.chromium.org/2686233006/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/chromeos_info_private/extended/background.js:12: chrome.chromeosInfoPrivate.get( nit: reformat this as: chrome.chromeosInfoPrivate.get([ 'sessionType', 'playStoreStatus', 'managedDeviceStatus' ], chrome.test.callbackPass(function(){ switch ... }));
chrome/common/extensions/api/chromeos_info_private.json lgtm
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, rdevlin.cronin@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2686233006/#ps120001 (title: "Minor changes in comments")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by wzang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/info_private_api.cc (right): https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/info_private_api.cc:141: // Property indicating an unmanaged device, which corresponds to the "not On 2017/02/22 06:55:27, tbarzic wrote: > how about: > "Value to which managedDeviceStatus property is set for unmanaged devices." Done. https://codereview.chromium.org/2686233006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/info_private_api.cc:285: if (connector->IsEnterpriseManaged()) { On 2017/02/22 06:55:27, tbarzic wrote: > nit: no {} here Done. Out of curiosity, is there a reason why in general we don't want {} here?
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487795071251990,
"parent_rev": "50b649bfceba9e8c03ea94ecce93a9c57389369e", "commit_rev":
"3777167162feb5b4fbd24e8386e92ae92fe354c8"}
Message was sent while issue was closed.
Description was changed from ========== Add managed device signal (managed or not managed). It'll be used by the Get Help app. Use fake install attributes for browser tests. BUG=682837 ========== to ========== Add managed device signal (managed or not managed). It'll be used by the Get Help app. Use fake install attributes for browser tests. BUG=682837 Review-Url: https://codereview.chromium.org/2686233006 Cr-Commit-Position: refs/heads/master@{#452193} Committed: https://chromium.googlesource.com/chromium/src/+/3777167162feb5b4fbd24e8386e9... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3777167162feb5b4fbd24e8386e9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
