Add a policy to hide the Web Store on new tabs.

Add a policy to hide the Web Store on new tabs.

This change adds a new policy and boolean pref named "HideWebStoreIcon".
When true, neither the "Web Store" button in the footer of the New Tab
page nor the "Chrome Web Store" app icon on the Apps page is visible.
This policy can be dynamically refreshed.

Contributed by dconnelly@chromium.org

BUG=89360
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184454

[dconnelly, 2013-01-18 13:11:55]

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:3883: 'supported_on': ['chrome.*:26-'],
Not sure what version to use here

[Joao da Silva, 2013-01-18 14:02:50]

Looks good! I'd just ask for a policy_browsertest for this :-)

Add a new test at chrome/browser/policy/policy_browsertest.cc. There are lots of
tests there with copy-paste-able snippets to do this:

- open NTP
- inject javascript into the page that queries for $('chrome-web-store-link')
and verifies that it is not hidden
- set the policy
- reload the NTP
- inject javascript that verifies that the link is now hidden

So if the UI team ever rewrites the NTP again then this will catch regressions
in this policy.

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:3883: 'supported_on': ['chrome.*:26-'],
On 2013/01/18 13:11:55, dconnelly wrote:
> Not sure what version to use here

26 is fine. This also affects the NTP on chromeos, but chromeos also shows the
webstore icon on the launcher. I'd leave that for a subsequent CL, and then we
can change this version.

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:3886: 'per_profile': False,
per_profile is True. This pref is stored per profile, so it's possible to have
multiple profiles with different settings each for this.

https://codereview.chromium.org/11859029/diff/1/chrome/app/policy/policy_temp...
chrome/app/policy/policy_templates.json:3888: 'future': False,
This can be removed

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
chrome/browser/resources/ntp4/new_tab.js:166:
$('chrome-web-store-link').style.display = 'none';
I think $('chrome-web-store-link').hidden = true; is the preferred style for
this.

[dconnelly, 2013-01-18 17:26:20]

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
chrome/browser/resources/ntp4/new_tab.js:166:
$('chrome-web-store-link').style.display = 'none';
I actually think removing the element from the DOM altogether makes more sense. 
What do you think?

On 2013/01/18 14:02:50, Joao da Silva wrote:
> I think $('chrome-web-store-link').hidden = true; is the preferred style for
> this.

[Joao da Silva, 2013-01-21 07:44:50]

lgtm after fixing the remaining nits. Thanks for the test! :-)

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/11859029/diff/1/chrome/browser/resources/ntp4...
chrome/browser/resources/ntp4/new_tab.js:166:
$('chrome-web-store-link').style.display = 'none';
On 2013/01/18 17:26:20, dconnelly wrote:
> I actually think removing the element from the DOM altogether makes more
sense. 
> What do you think?
> 

SGTM but I'd wait for Evan's approval on this :-)

> On 2013/01/18 14:02:50, Joao da Silva wrote:
> > I think $('chrome-web-store-link').hidden = true; is the preferred style for
> > this.
>

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1100: bool
ContainsElement(content::WebContents* contents, const std::string id) {
Helper functions like this come before the tests, in the anonymous namespace.
Place this earlier in the file, right after FlushBlacklistPolicy().

Also, pass |id| by const &.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1104:
"domAutomationController.send(!!document.getElementById(\"" + id + "\"))",
Single quotes could be used instead of \"; just a suggestion.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1115:
EXPECT_TRUE(ContainsElement(contents, "ahfgeienlihckogmohjhadlkjgocpleb"));
What does that ID refer to? Please comment it (I'm guessing it's the
extension-ID of the webstore app?)

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1118:
EXPECT_TRUE(ContainsElement(contents, "chrome-web-store-link"));
nit: indent (2 spaces instead of 4)

[dconnelly, 2013-01-21 09:39:03]

Fixed browser test on Chromium OS where there is no apps page.  Per Joao's
suggestion, removing the Web Store app from the launcher will be a separate CL.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1100: bool
ContainsElement(content::WebContents* contents, const std::string id) {
On 2013/01/21 07:44:51, Joao da Silva wrote:
> Helper functions like this come before the tests, in the anonymous namespace.
> Place this earlier in the file, right after FlushBlacklistPolicy().
> 
> Also, pass |id| by const &.

Done.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1104:
"domAutomationController.send(!!document.getElementById(\"" + id + "\"))",
On 2013/01/21 07:44:51, Joao da Silva wrote:
> Single quotes could be used instead of \"; just a suggestion.

Done.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1115:
EXPECT_TRUE(ContainsElement(contents, "ahfgeienlihckogmohjhadlkjgocpleb"));
On 2013/01/21 07:44:51, Joao da Silva wrote:
> What does that ID refer to? Please comment it (I'm guessing it's the
> extension-ID of the webstore app?)

Done.

https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:1118:
EXPECT_TRUE(ContainsElement(contents, "chrome-web-store-link"));
On 2013/01/21 07:44:51, Joao da Silva wrote:
> nit: indent (2 spaces instead of 4)

Done.

[dconnelly, 2013-01-24 10:03:11]

Ping

On 2013/01/21 09:39:03, dconnelly wrote:
> Fixed browser test on Chromium OS where there is no apps page.  Per Joao's
> suggestion, removing the Web Store app from the launcher will be a separate
CL.
> 
>
https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
> File chrome/browser/policy/policy_browsertest.cc (right):
> 
>
https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
> chrome/browser/policy/policy_browsertest.cc:1100: bool
> ContainsElement(content::WebContents* contents, const std::string id) {
> On 2013/01/21 07:44:51, Joao da Silva wrote:
> > Helper functions like this come before the tests, in the anonymous
namespace.
> > Place this earlier in the file, right after FlushBlacklistPolicy().
> > 
> > Also, pass |id| by const &.
> 
> Done.
> 
>
https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
> chrome/browser/policy/policy_browsertest.cc:1104:
> "domAutomationController.send(!!document.getElementById(\"" + id + "\"))",
> On 2013/01/21 07:44:51, Joao da Silva wrote:
> > Single quotes could be used instead of \"; just a suggestion.
> 
> Done.
> 
>
https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
> chrome/browser/policy/policy_browsertest.cc:1115:
> EXPECT_TRUE(ContainsElement(contents, "ahfgeienlihckogmohjhadlkjgocpleb"));
> On 2013/01/21 07:44:51, Joao da Silva wrote:
> > What does that ID refer to? Please comment it (I'm guessing it's the
> > extension-ID of the webstore app?)
> 
> Done.
> 
>
https://codereview.chromium.org/11859029/diff/5003/chrome/browser/policy/poli...
> chrome/browser/policy/policy_browsertest.cc:1118:
> EXPECT_TRUE(ContainsElement(contents, "chrome-web-store-link"));
> On 2013/01/21 07:44:51, Joao da Silva wrote:
> > nit: indent (2 spaces instead of 4)
> 
> Done.

[Evan Stade, 2013-01-24 18:44:31]

like I said in email, I think the NTP itself should be left alone until we have
a good solution for the app list centering problem.

[dconnelly1, 2013-01-25 15:51:08]

See https://codereview.chromium.org/12038090/ for proposed NTP footer
layout solution.

On Thu, Jan 24, 2013 at 7:44 PM,  <estade@chromium.org> wrote:
> like I said in email, I think the NTP itself should be left alone until we
> have
> a good solution for the app list centering problem.
>
> https://codereview.chromium.org/11859029/

[dconnelly, 2013-01-25 15:58:33]

Hi James,

This is the other CL.  If you're not the right person, could you suggest someone
else?

Thanks!

[James Hawkins, 2013-01-25 17:11:20]

On 2013/01/25 15:58:33, dconnelly wrote:
> Hi James,
> 
> This is the other CL.  If you're not the right person, could you suggest
someone
> else?
> 
> Thanks!

Evan is defacto owner of NTP, so you're going to have to convince him of what
you're working on.

[dconnelly, 2013-02-13 11:49:20]

On 2013/01/24 18:44:31, Evan Stade wrote:
> like I said in email, I think the NTP itself should be left alone until we
have
> a good solution for the app list centering problem.

Hi Evan,

Now that the NTP footer CL is submitted, could you take a look at this?

Thanks!

[Evan Stade, 2013-02-13 22:40:08]

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
chrome/browser/resources/ntp4/apps_page.js:671: // Don't show the web store icon
if policy so dictates.
it seems like this should be handled at the C++ layer.

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
chrome/browser/resources/ntp4/new_tab.js:175: // The next NTP has no footer.
"Next" isn't very descriptive since its definition changes frequently.

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:410:
load_time_data.SetBoolean("isWebStoreIconHidden",
make this one's name match the above pref, i.e.  showWebStoreIcon

[Evan Stade, 2013-02-14 01:58:10]

since I'm going to be OOO till Tuesday (again, sorry), Dan Beam can continue
this review in my absence.

[dconnelly, 2013-02-14 10:26:57]

Cool, thanks Evan.

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
chrome/browser/resources/ntp4/apps_page.js:671: // Don't show the web store icon
if policy so dictates.
On 2013/02/13 22:40:08, Evan Stade wrote:
> it seems like this should be handled at the C++ layer.

Done.

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/res...
chrome/browser/resources/ntp4/new_tab.js:175: // The next NTP has no footer.
On 2013/02/13 22:40:08, Evan Stade wrote:
> "Next" isn't very descriptive since its definition changes frequently.

Done.

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/30001/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:410:
load_time_data.SetBoolean("isWebStoreIconHidden",
On 2013/02/13 22:40:08, Evan Stade wrote:
> make this one's name match the above pref, i.e.  showWebStoreIcon

Done.

[Dan Beam, 2013-02-14 21:46:43]

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:4284: 'desc': '''Hide the Chrome Web
Store icons from the New Tab Page.
s/icons/Chrome Web Store app and footer link/?

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
chrome/common/pref_names.cc:1685: 
why isn't this pref in the ntp. dictionary?

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/test/data/p...
File chrome/test/data/policy/policy_test_cases.json (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/test/data/p...
chrome/test/data/policy/policy_test_cases.json:1462: "os": ["win", "linux",
"mac", "chromeos"],
is this relevant on mobile?

[dconnelly, 2013-02-15 08:44:45]

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:4284: 'desc': '''Hide the Chrome Web
Store icons from the New Tab Page.
On 2013/02/14 21:46:43, Dan Beam wrote:
> s/icons/Chrome Web Store app and footer link/?

Done.

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
chrome/common/pref_names.cc:1685: 
On 2013/02/14 21:46:43, Dan Beam wrote:
> why isn't this pref in the ntp. dictionary?

Because it applies to the ChromeOS app launcher, too.  I have another review
pending that hides the web store there.  I updated the comment above and the
policy description file to mention the launcher.

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/test/data/p...
File chrome/test/data/policy/policy_test_cases.json (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/test/data/p...
chrome/test/data/policy/policy_test_cases.json:1462: "os": ["win", "linux",
"mac", "chromeos"],
On 2013/02/14 21:46:43, Dan Beam wrote:
> is this relevant on mobile?

No, for two reasons (thanks Joao):
- there's no policy on mobile
- this is just a test file

[Dan Beam, 2013-02-15 23:50:27]

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/39001/chrome/common/pref...
chrome/common/pref_names.cc:1685: 
On 2013/02/15 08:44:45, dconnelly wrote:
> On 2013/02/14 21:46:43, Dan Beam wrote:
> > why isn't this pref in the ntp. dictionary?
> 
> Because it applies to the ChromeOS app launcher, too.  I have another review
> pending that hides the web store there.  I updated the comment above and the
> policy description file to mention the launcher.

Ok, makes sense.

https://chromiumcodereview.appspot.com/11859029/diff/41012/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/41012/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:357: if (extension &&
ShouldDisplayInNewTabPage(extension, prefs)) {
I don't see the diff to remove ::ShouldDisplayInNewTabPage from the old location
-- where is this?

[dconnelly, 2013-02-18 09:35:54]

https://chromiumcodereview.appspot.com/11859029/diff/41012/chrome/browser/ui/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://chromiumcodereview.appspot.com/11859029/diff/41012/chrome/browser/ui/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:357: if (extension &&
ShouldDisplayInNewTabPage(extension, prefs)) {
On 2013/02/15 23:50:27, Dan Beam wrote:
> I don't see the diff to remove ::ShouldDisplayInNewTabPage from the old
location
> -- where is this?

That's still in the Extension class (chrome/common/extensions/extension.h).  I
didn't remove or change it there because it's a property of the extension from
its JSON manifest, and I thought this policy-testing should be here in the
presentation logic.  I added the above  ShouldDisplayInNewTabPage function that
tests against the policy.  If you think the policy-testing logic should go in
the Extension class I'll move it.

[Dan Beam, 2013-02-19 17:52:14]

If there's another valid use of that code (and it's not dead code now), then
lgtm. If there's not a reason for it to exist any longer, I'd remove it before
landing.

[commit-bot: I haz the power, 2013-02-21 14:45:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dconnelly@chromium.org/11859029/63001

[commit-bot: I haz the power, 2013-02-21 14:45:22]

Presubmit check for 11859029-63001 failed and returned exit status 1.

INFO:root:Found 11 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/app/policy/PRESUBMIT.py
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 2.5s to calculate.

[dconnelly1, 2013-02-21 15:25:50]

Whoops.  Evan, are you back in the office?  Dan gave me a review and
lgtm but he's not an OWNER so I can't commit.

On Thu, Feb 21, 2013 at 3:45 PM,  <commit-bot@chromium.org> wrote:
> Presubmit check for 11859029-63001 failed and returned exit status 1.
>
> INFO:root:Found 11 file(s).
>
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/chrome/app/policy/PRESUBMIT.py
> Finished checking
> /b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
> errors, 0 warnings.
> Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
> Running
> /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
> Running
> /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py
>
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui
>
> Presubmit checks took 2.5s to calculate.
>
>
>
> https://chromiumcodereview.appspot.com/11859029/

[Dan Beam, 2013-02-21 20:55:04]

you need someone in chrome/browser/ui/OWNERS (ben@, pkasting@, or sky@)

[dconnelly, 2013-02-25 09:09:12]

Hi Ben,

I need a chrome/browser/ui OWNER to review this CL.  If you have a chance could
you take a look?

Thanks!

[Ben Goodger (Google), 2013-02-25 16:03:38]

owner lgtm provided someone more familiar with this area has given it a review.

[commit-bot: I haz the power, 2013-02-25 16:52:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dconnelly@chromium.org/11859029/74002

[commit-bot: I haz the power, 2013-02-25 19:09:23]

Change committed as 184454