Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801173002:
arc: Provide API to control Play Store state from autotests (Closed)
Descriptionarc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will deprecate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
Review-Url: https://codereview.chromium.org/2801173002
Cr-Commit-Position: refs/heads/master@{#464735}
Committed: https://chromium.googlesource.com/chromium/src/+/d127ade1e89dd1b221e85170e0dbe4c32072c7c5
Patch Set 1 #Patch Set 2 : fix browser test #
Total comments: 8
Patch Set 3 : handle attempt to enabled policy disabled ARC, nits #
Total comments: 2
Patch Set 4 : use callbacks instead of direct return #
Total comments: 14
Patch Set 5 : js functions updated #Patch Set 6 : changed JS API from isEnabled/Managed to getState #
Total comments: 7
Patch Set 7 : make enabled/managed optional fields #
Total comments: 2
Patch Set 8 : nit #
Total comments: 10
Patch Set 9 : use generated structs instead of direct access #Patch Set 10 : return boolean for SetArcPlayStoreEnabledForProfile #
Total comments: 12
Patch Set 11 : nits #Messages
Total messages: 54 (27 generated)
The CQ bit was checked by khmel@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
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will depricate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually
==========
to
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will depricate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
==========
Description was changed from
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will depricate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
==========
to
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will deprecate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
==========
khmel@chromium.org changed reviewers: + isherman@chromium.org, lhchavez@chromium.org
khmel@chromium.org changed reviewers: + finnur@chromium.org
Hi, PTAL Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by khmel@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...
histograms.xml lgtm
https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:388: Profile* profile = ProfileManager::GetActiveUserProfile(); qq: is Profile* guaranteed to be non-null at this point always? https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:389: enabled = arc::IsArcAllowedForProfile(profile) && nit: This is redundant. https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:418: return RespondNow(NoArguments()); Don't you want to return success/failure? Otherwise document this is a no-op if autotestPrivate.isPlayStoreEnabled() returns false.
https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (arc::IsArcAllowedForProfile(profile)) Do you also want the semantics of this actually enabling the play store even if the setting is managed? Can people on their Chromebooks use this API to circumvent policy?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for reviewing and comments, PTAL updated version https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:388: Profile* profile = ProfileManager::GetActiveUserProfile(); On 2017/04/07 01:57:50, Luis Héctor Chávez wrote: > qq: is Profile* guaranteed to be non-null at this point always? I think no, but arc::IsArcAllowedForProfile(profile) handles null profiles https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:389: enabled = arc::IsArcAllowedForProfile(profile) && On 2017/04/07 01:57:50, Luis Héctor Chávez wrote: > nit: This is redundant. Removed. https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (arc::IsArcAllowedForProfile(profile)) On 2017/04/07 02:01:53, Luis Héctor Chávez wrote: > Do you also want the semantics of this actually enabling the play store even if > the setting is managed? Can people on their Chromebooks use this API to > circumvent policy? This is good point. I will update arc::SetArcPlayStoreEnabledForProfile https://codereview.chromium.org/2801173002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:418: return RespondNow(NoArguments()); On 2017/04/07 01:57:50, Luis Héctor Chávez wrote: > Don't you want to return success/failure? Otherwise document this is a no-op if > autotestPrivate.isPlayStoreEnabled() returns false. Added returning the value
https://codereview.chromium.org/2801173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2801173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:118: if (enabled && !IsArcPlayStoreEnabledForProfile(profile)) { IIUC (but I'm not entirely sure) this was added for testing purposes. Can you run `git cl test`? If that fails, please move this check to the autotest api.
The CQ bit was checked by khmel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
it seems direct return is not allowed and only one valid way to return via callback. I updated interfaces to this approach https://codereview.chromium.org/2801173002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2801173002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:118: if (enabled && !IsArcPlayStoreEnabledForProfile(profile)) { On 2017/04/07 16:09:05, Luis Héctor Chávez wrote: > IIUC (but I'm not entirely sure) this was added for testing purposes. Can you > run `git cl test`? If that fails, please move this check to the autotest api. I remember I added this to enable restart ARC for managed case. No regression was found during the 'git cl try'. Failure was due autotest browser test
https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:118: if (enabled && !IsArcPlayStoreEnabledForProfile(profile)) { nit: This should be |enabled != IsArcPlayStoreEnabledForProfile(profile)|. We also don't want people disabling this if it was enabled by policy :P
https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_util.cc (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_util.cc:118: if (enabled && !IsArcPlayStoreEnabledForProfile(profile)) { On 2017/04/07 22:30:11, Luis Héctor Chávez wrote: > nit: This should be |enabled != IsArcPlayStoreEnabledForProfile(profile)|. We > also don't want people disabling this if it was enabled by policy :P Not true actually. User may close opt-in window and we have to stop ARC . Once user clicks PlayStore again ARC should be started.
finnur@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - finnur@chromium.org
I'm probably borderline on even reviewing changes to existing extensions code, but new APIs should probably go through a member of the extensions team. Devlin?
I don't have a ton of context here; stevenjb@, does this look right to you? https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:65: callback EnabledCallback = void (boolean enabled); nit: just use one: callback BooleanCallback = void(boolean result) https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:122: // Return true if Play Store is currently enabled on device. Function comments should be descriptive, not imperative. Also, should be `if *the* Play Store is currently enabled on *the* device`. (emphasis just to call out differences) https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:128: // Enable or disable Play Store on device. Return true in callback if Play This seems like the result is basically just whether or not the call succeeded. If the call didn't succeed, we should probably return an error instead.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:417: was_enabled = arc::IsArcPlayStoreEnabledForProfile(profile); I think this should probably be: if (!arc::IsArcPlayStoreEnabledForProfile(profile)) return RespondNow(Error("Unable to enable ARC")); Instead of returning a bool. https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:126: static void isPlayStoreManaged(ManagedCallback callback); Do we anticipate any additional play store related state that might be useful in an auto test? It might be better to create a sing getPlayStoreState function that returns a dictionary with 'enabled' and 'managed'. Then if we need additional state later we could easily add it. (That said, I can't think of what else we might care about... maybe settings_app_installed?) https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:128: // Enable or disable Play Store on device. Return true in callback if Play On 2017/04/10 21:53:54, Devlin wrote: > This seems like the result is basically just whether or not the call succeeded. > If the call didn't succeed, we should probably return an error instead. Agreed. Specifically, make this a void callback and set an error on failure.
Thank you for your comments. PTAL updated version https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:65: callback EnabledCallback = void (boolean enabled); On 2017/04/10 21:53:54, Devlin wrote: > nit: just use one: > callback BooleanCallback = void(boolean result) Done. https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:122: // Return true if Play Store is currently enabled on device. On 2017/04/10 21:53:54, Devlin wrote: > Function comments should be descriptive, not imperative. Also, should be `if > *the* Play Store is currently enabled on *the* device`. (emphasis just to call > out differences) Tried to rephrase. https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:126: static void isPlayStoreManaged(ManagedCallback callback); On 2017/04/11 15:58:56, stevenjb wrote: > Do we anticipate any additional play store related state that might be useful in > an auto test? It might be better to create a sing getPlayStoreState function > that returns a dictionary with 'enabled' and 'managed'. Then if we need > additional state later we could easily add it. (That said, I can't think of what > else we might care about... maybe settings_app_installed?) I think this is not very applicable for current situation. If we follow this approach we have to return enabled, disabled, enabled_managed, disabled_managed and probably not_avaialable. It seems this complicates implementation and is in contradiction how we use this internally. These js functions just map internal implementation. https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:128: // Enable or disable Play Store on device. Return true in callback if Play On 2017/04/11 15:58:56, stevenjb wrote: > On 2017/04/10 21:53:54, Devlin wrote: > > This seems like the result is basically just whether or not the call > succeeded. > > If the call didn't succeed, we should probably return an error instead. > > Agreed. Specifically, make this a void callback and set an error on failure. Good points, thanks
https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:126: static void isPlayStoreManaged(ManagedCallback callback); On 2017/04/11 21:16:28, khmel wrote: > On 2017/04/11 15:58:56, stevenjb wrote: > > Do we anticipate any additional play store related state that might be useful > in > > an auto test? It might be better to create a sing getPlayStoreState function > > that returns a dictionary with 'enabled' and 'managed'. Then if we need > > additional state later we could easily add it. (That said, I can't think of > what > > else we might care about... maybe settings_app_installed?) > > I think this is not very applicable for current situation. If we follow this > approach we have to return enabled, disabled, enabled_managed, disabled_managed > and probably not_avaialable. It seems this complicates implementation and is in > contradiction how we use this internally. These js functions just map internal > implementation. I meant that we can return a dictionary: dictionary PlayStoreState { boolean enabled; boolean managed; boolean available; boolean settings_app_installed; } static void getPlayStoreState(GetPlayStoreState callback);
https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/60001/chrome/common/extension... chrome/common/extensions/api/autotest_private.idl:126: static void isPlayStoreManaged(ManagedCallback callback); On 2017/04/11 21:36:05, stevenjb wrote: > On 2017/04/11 21:16:28, khmel wrote: > > On 2017/04/11 15:58:56, stevenjb wrote: > > > Do we anticipate any additional play store related state that might be > useful > > in > > > an auto test? It might be better to create a sing getPlayStoreState function > > > that returns a dictionary with 'enabled' and 'managed'. Then if we need > > > additional state later we could easily add it. (That said, I can't think of > > what > > > else we might care about... maybe settings_app_installed?) > > > > I think this is not very applicable for current situation. If we follow this > > approach we have to return enabled, disabled, enabled_managed, > disabled_managed > > and probably not_avaialable. It seems this complicates implementation and is > in > > contradiction how we use this internally. These js functions just map internal > > implementation. > > I meant that we can return a dictionary: > > dictionary PlayStoreState { > boolean enabled; > boolean managed; > boolean available; > boolean settings_app_installed; > } > > static void getPlayStoreState(GetPlayStoreState callback); Sorry, did not get you that time. Yes, this sounds better. Done except settings_app_installed which is currently not needed.
lgtm w/ optional suggestions https://codereview.chromium.org/2801173002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:392: play_store_state_value->SetBoolean("allowed", false); nit: Maybe just set "allowed" to false? The other properties should be optional and ignored in that case. https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:68: boolean enabled; You can make this optional with: boolean? enabled; https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:72: boolean allowed; nit: logically and alphabetically I would expect this to be first. https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:133: static void getPlayStoreState(PlayStoreStateCallback callback); Thanks!
Thank you Steven for review! https://codereview.chromium.org/2801173002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:392: play_store_state_value->SetBoolean("allowed", false); On 2017/04/13 00:23:47, stevenjb wrote: > nit: Maybe just set "allowed" to false? The other properties should be optional > and ignored in that case. Makes sense https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:68: boolean enabled; On 2017/04/13 00:23:48, stevenjb wrote: > You can make this optional with: > > boolean? enabled; Done. https://codereview.chromium.org/2801173002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:72: boolean allowed; On 2017/04/13 00:23:48, stevenjb wrote: > nit: logically and alphabetically I would expect this to be first. Done.
The CQ bit was checked by khmel@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 with a nit https://codereview.chromium.org/2801173002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:389: new base::DictionaryValue); nit: play_store_state_value = base::MakeUnique<base::DictionaryValue>();
khmel@chromium.org changed reviewers: + jochen@chromium.org
Thank you Luis for review, Presubmit is still unhappy. Adding jochen@ for approval. PTAL https://codereview.chromium.org/2801173002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:389: new base::DictionaryValue); On 2017/04/13 14:18:26, Luis Héctor Chávez wrote: > nit: play_store_state_value = base::MakeUnique<base::DictionaryValue>(); Good catch, copy paste from this file fooled me :).
https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:387: DVLOG(1) << "AutotestPrivateGetPlayStoreStateFunction"; Do you need to keep these logs? https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:394: play_store_state_value->SetBoolean("allowed", true); Please use the generated types rather than using a custom-built value. https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (params->enabled != arc::IsArcPlayStoreEnabledForProfile(profile)) { This would be cleaner if we just had SetArcPlayStoreEnabledForProfile() return a boolean indicating success. https://codereview.chromium.org/2801173002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:67: // Is the Play Store allowed for the current user? nit: Let's phrase these as "Whether the Play Store is..."
Thanks Devlin for comments. Please find updated version. https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:387: DVLOG(1) << "AutotestPrivateGetPlayStoreStateFunction"; On 2017/04/13 21:49:52, Devlin wrote: > Do you need to keep these logs? Most of the functions here use similar structure: for example L 366 https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:394: play_store_state_value->SetBoolean("allowed", true); On 2017/04/13 21:49:52, Devlin wrote: > Please use the generated types rather than using a custom-built value. Done. https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (params->enabled != arc::IsArcPlayStoreEnabledForProfile(profile)) { On 2017/04/13 21:49:52, Devlin wrote: > This would be cleaner if we just had SetArcPlayStoreEnabledForProfile() return a > boolean indicating success. You mean static boolean SetArcPlayStoreEnabledForProfile() ? I tried this in my implementation. When I do like this I cannot use SetArcPlayStoreEnabledForProfile in test.js because I always getting error: [FAIL] setPlayStoreEnabled: uncaught exception: Parameter 1 (setPlayStoreEnabled) is required.: Error: Parameter 1 (setPlayStoreEnabled) is required. for chrome.autotestPrivate.setPlayStoreEnabled(false); chrome.test.succeed(); I also did not find good example that has inline boolean as return type. PS. In your previous comment you suggested to return error: >> If the call didn't succeed, we should probably return an error instead. https://codereview.chromium.org/2801173002/diff/140001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:67: // Is the Play Store allowed for the current user? On 2017/04/13 21:49:52, Devlin wrote: > nit: Let's phrase these as "Whether the Play Store is..." Done.
https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (params->enabled != arc::IsArcPlayStoreEnabledForProfile(profile)) { On 2017/04/13 23:02:51, khmel wrote: > On 2017/04/13 21:49:52, Devlin wrote: > > This would be cleaner if we just had SetArcPlayStoreEnabledForProfile() return > a > > boolean indicating success. > > You mean static boolean SetArcPlayStoreEnabledForProfile() ? > > I tried this in my implementation. When I do like this I cannot use > SetArcPlayStoreEnabledForProfile in test.js because I always getting error: > > [FAIL] setPlayStoreEnabled: uncaught exception: Parameter 1 > (setPlayStoreEnabled) is required.: Error: Parameter 1 (setPlayStoreEnabled) is > required. > > for > chrome.autotestPrivate.setPlayStoreEnabled(false); > chrome.test.succeed(); > > I also did not find good example that has inline boolean as return type. > > PS. In your previous comment you suggested to return error: > >> If the call didn't succeed, we should probably return an error instead. > Ah, sorry, I can see how that would be confusing. I mean changing the signature of arc::SetArcPlayStoreEnabledForProfile() defined in arc_util.cc. If that could return a boolean indicating whether or not the set was successful, we wouldn't have to then check the state here. i.e., this becomes: if (!arc::SetArcPlayStoreEnabledForProfile(profile, params->enabled)) { Respond(Error("ARC state cannot be changed for the current user")); }
https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:415: if (params->enabled != arc::IsArcPlayStoreEnabledForProfile(profile)) { On 2017/04/13 23:06:00, Devlin wrote: > On 2017/04/13 23:02:51, khmel wrote: > > On 2017/04/13 21:49:52, Devlin wrote: > > > This would be cleaner if we just had SetArcPlayStoreEnabledForProfile() > return > > a > > > boolean indicating success. > > > > You mean static boolean SetArcPlayStoreEnabledForProfile() ? > > > > I tried this in my implementation. When I do like this I cannot use > > SetArcPlayStoreEnabledForProfile in test.js because I always getting error: > > > > [FAIL] setPlayStoreEnabled: uncaught exception: Parameter 1 > > (setPlayStoreEnabled) is required.: Error: Parameter 1 (setPlayStoreEnabled) > is > > required. > > > > for > > chrome.autotestPrivate.setPlayStoreEnabled(false); > > chrome.test.succeed(); > > > > I also did not find good example that has inline boolean as return type. > > > > PS. In your previous comment you suggested to return error: > > >> If the call didn't succeed, we should probably return an error instead. > > > > Ah, sorry, I can see how that would be confusing. I mean changing the signature > of arc::SetArcPlayStoreEnabledForProfile() defined in arc_util.cc. If that > could return a boolean indicating whether or not the set was successful, we > wouldn't have to then check the state here. i.e., this becomes: > > if (!arc::SetArcPlayStoreEnabledForProfile(profile, params->enabled)) { > Respond(Error("ARC state cannot be changed for the current user")); > } Got you :), done.
lgtm % nits https://codereview.chromium.org/2801173002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:408: EXTENSION_FUNCTION_VALIDATE(params.get()); nit: this can just be EXTENSION_FUNCTION_VALIDATE(params); (unique_ptr has an operator bool) https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:67: // Whether the Play Store allowed for the current user? nit: Replace '?' with a '.' (we want to avoid "questions" in API documentation.) https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:69: // Is the Play Store currently enabled? For this one, too, s/Is/Whether, s/?/. Whether the Play Store is currently enabled. https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:72: boolean? managed; ditto: Whether the Play Store is managed by policy. https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/autotest_private/test.js (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/autotest_private/test.js:117: chrome.test.assertEq(state.enabled, undefined); nit: assertEq(expected, actual), so this should be assertEq(undefined, state.enabled) https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/autotest_private/test.js:118: chrome.test.assertEq(state.managed, undefined); ditto
Thank you Devlin for review! As PS: We probably need to refactor autotest API because most comments for this CL can be addressed to other functionality. https://codereview.chromium.org/2801173002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/autotest_private/autotest_private_api.cc (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/autotest_private/autotest_private_api.cc:408: EXTENSION_FUNCTION_VALIDATE(params.get()); On 2017/04/14 14:34:24, Devlin wrote: > nit: this can just be EXTENSION_FUNCTION_VALIDATE(params); (unique_ptr has an > operator bool) Done. https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... File chrome/common/extensions/api/autotest_private.idl (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:67: // Whether the Play Store allowed for the current user? On 2017/04/14 14:34:24, Devlin wrote: > nit: Replace '?' with a '.' (we want to avoid "questions" in API documentation.) Done. https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:69: // Is the Play Store currently enabled? On 2017/04/14 14:34:24, Devlin wrote: > For this one, too, s/Is/Whether, s/?/. > Whether the Play Store is currently enabled. Done. https://codereview.chromium.org/2801173002/diff/180001/chrome/common/extensio... chrome/common/extensions/api/autotest_private.idl:72: boolean? managed; On 2017/04/14 14:34:24, Devlin wrote: > ditto: > Whether the Play Store is managed by policy. Done. https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/autotest_private/test.js (right): https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/autotest_private/test.js:117: chrome.test.assertEq(state.enabled, undefined); On 2017/04/14 14:34:24, Devlin wrote: > nit: assertEq(expected, actual), so this should be assertEq(undefined, > state.enabled) Done. https://codereview.chromium.org/2801173002/diff/180001/chrome/test/data/exten... chrome/test/data/extensions/api_test/autotest_private/test.js:118: chrome.test.assertEq(state.managed, undefined); On 2017/04/14 14:34:25, Devlin wrote: > ditto Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, stevenjb@chromium.org, lhchavez@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2801173002/#ps200001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492184378848050,
"parent_rev": "50549382727806e2ae44c45444827d02270b5d57", "commit_rev":
"d127ade1e89dd1b221e85170e0dbe4c32072c7c5"}
Message was sent while issue was closed.
Description was changed from
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will deprecate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
==========
to
==========
arc: Provide API to control Play Store state from autotests
This provides required API to read and set Play Store state from
autotests since old settings page will deprecate soon and new MD
settings don't provide reliable way to access Play Store settings.
BUG=694081
TEST=Manually, together with
https://chromium-review.googlesource.com/c/470707/
Review-Url: https://codereview.chromium.org/2801173002
Cr-Commit-Position: refs/heads/master@{#464735}
Committed:
https://chromium.googlesource.com/chromium/src/+/d127ade1e89dd1b221e85170e0db...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/d127ade1e89dd1b221e85170e0db... |
