Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12038067:
Filter Web Store app from ChromeOS launcher according to policy. (Closed)
|
Created:
7 years, 11 months ago by dconnelly Modified:
7 years, 9 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFilter Web Store app from ChromeOS launcher according to policy.
Depends on review https://codereview.chromium.org/11859029/
Contributed by dconnelly@chromium.org
BUG=89360
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184978
Patch Set 1 #
Total comments: 4
Patch Set 2 : style per tfarina comments #
Total comments: 8
Patch Set 3 : Move policy check to own method and filter enterprise web store #Patch Set 4 : rebase #Patch Set 5 : use string#find instead of string== to avoid flaky tests #Patch Set 6 : stop reusing AppListModel::Apps in test #
Messages
Total messages: 18 (0 generated)
Hi Xiyuan, I'm a noogler on the Chrome Enterprise team and I've implemented a policy to hide the Chrome Web Store app. I have two CLs, the first of which (https://codereview.chromium.org/11859029) hides the app from Chrome's New Tab Page, and the second (https://codereview.chromium.org/12038067) hides the app from the Chrome OS launcher. git-cl suggested you as an owner on the second CL. Would you mind reviewing my change, or suggesting someone else? Thanks!
https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right): https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:102: profile_.get()->GetPrefs()->SetBoolean(prefs::kHideWebStoreIcon, true); profile_->GetPrefs()
https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... File chrome/browser/ui/app_list/apps_model_builder.cc (right): https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... chrome/browser/ui/app_list/apps_model_builder.cc:83: (*app)->id() == extension_misc::kWebStoreAppId && indent two more spaces.
Thank you! On Thu, Jan 24, 2013 at 3:15 PM, <tfarina@chromium.org> wrote: > > https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... > File chrome/browser/ui/app_list/apps_model_builder.cc (right): > > https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... > chrome/browser/ui/app_list/apps_model_builder.cc:83: (*app)->id() == > extension_misc::kWebStoreAppId && > indent two more spaces. > > https://codereview.chromium.org/12038067/
https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... File chrome/browser/ui/app_list/apps_model_builder.cc (right): https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... chrome/browser/ui/app_list/apps_model_builder.cc:83: (*app)->id() == extension_misc::kWebStoreAppId && On 2013/01/24 14:15:58, tfarina wrote: > indent two more spaces. Done. https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right): https://codereview.chromium.org/12038067/diff/1/chrome/browser/ui/app_list/ap... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:102: profile_.get()->GetPrefs()->SetBoolean(prefs::kHideWebStoreIcon, true); On 2013/01/24 14:15:23, tfarina wrote: > profile_->GetPrefs() Done.
What if kHideWebStoreIcon is changed while app launcher is in display? Do we need to worry about this case?
Currently nothing will happen because AppsModelBuilder#Build() is only called when the launcher is hidden/shown. We aren't worrying about this case for the New Tab Page--when the policy changes, the new tab page isn't force-refreshed. On Thu, Jan 24, 2013 at 6:42 PM, <xiyuan@chromium.org> wrote: > What if kHideWebStoreIcon is changed while app launcher is in display? Do we > need to worry about this case? > > https://codereview.chromium.org/12038067/
Looks mostly good to me, see inline. On 2013/01/24 17:42:49, xiyuan wrote: > What if kHideWebStoreIcon is changed while app launcher is in display? Do we > need to worry about this case? These sort of policies are often configured once and that's it. It's not worth the added complexity to handle that extremely rare case (note that policies are refreshed after sign-in and then every 3 hours, so it'd be really hard to hit that case).
https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... File chrome/browser/ui/app_list/apps_model_builder.cc (right): https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder.cc:83: (*app)->id() == extension_misc::kWebStoreAppId && #include "chrome/common/extensions/extension_constants.h" https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder.cc:85: if ((*app)->ShouldDisplayInAppLauncher() && !blocked_by_policy) I suggest adding a new method to do these checks, so that here the code becomes: if (ShouldDisplayInAppLauncher(*app)) apps->push_back(...); The new method checks for app->ShouldDisplayInAppLauncher and the policy. There is also an enterprise webstore app, which has a different AppID; you should also block extension_misc::kEnterpriseWebStoreAppId. https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right): https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:78: DictionaryValue value; #include "base/values.h" https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:88: std::string(extension_misc::kWebStoreAppId), #include "chrome/common/extensions/extension_constants.h"
https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... File chrome/browser/ui/app_list/apps_model_builder.cc (right): https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder.cc:83: (*app)->id() == extension_misc::kWebStoreAppId && On 2013/01/25 09:10:12, Joao da Silva wrote: > #include "chrome/common/extensions/extension_constants.h" Done. https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder.cc:85: if ((*app)->ShouldDisplayInAppLauncher() && !blocked_by_policy) On 2013/01/25 09:10:12, Joao da Silva wrote: > I suggest adding a new method to do these checks, so that here the code becomes: > > if (ShouldDisplayInAppLauncher(*app)) > apps->push_back(...); > > The new method checks for app->ShouldDisplayInAppLauncher and the policy. > > There is also an enterprise webstore app, which has a different AppID; you > should also block extension_misc::kEnterpriseWebStoreAppId. Done. https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right): https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:78: DictionaryValue value; On 2013/01/25 09:10:12, Joao da Silva wrote: > #include "base/values.h" Done. https://codereview.chromium.org/12038067/diff/6001/chrome/browser/ui/app_list... chrome/browser/ui/app_list/apps_model_builder_unittest.cc:88: std::string(extension_misc::kWebStoreAppId), On 2013/01/25 09:10:12, Joao da Silva wrote: > #include "chrome/common/extensions/extension_constants.h" Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038067/12001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038067/16003
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038067/23005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dconnelly@chromium.org/12038067/23005
Message was sent while issue was closed.
Change committed as 184978 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
