Added a policy to control the visibility of the apps shortcut.

Added a policy to control the visibility of the apps shortcut.

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

[Joao da Silva, 2014-06-03 12:10:12]

Please have a look:

bartfab: policy code

asvitkine: cocoa & histograms

sky: bookmarks code

Thanks!

[Alexei Svitkine (slow), 2014-06-03 14:29:03]

lgtm

[sky, 2014-06-03 16:15:07]

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:268: 
nit: only one newline.

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:77:
TEST_F(BookmarkBarViewInstantExtendedTest,
Your test has nothing to do with instant, so making it part
BookmarkBarViewInstantExtendedTest is confusing. Can you use
BrowserWithTestWindowTest directly and name the test more appropriately. I'm
fine with a generic BookmarkBarViewTest if you want.

[Joao da Silva, 2014-06-03 22:20:53]

@sky: PTAL

@bartfab: please have a look at the policy changes

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:263:
NoAppsShortcutWithoutInstantExtended);
FYI, this test doesn't seem to exist anymore.

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:268: 
On 2014/06/03 16:15:07, sky wrote:
> nit: only one newline.

Done.

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/312673006/diff/1/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:77:
TEST_F(BookmarkBarViewInstantExtendedTest,
On 2014/06/03 16:15:07, sky wrote:
> Your test has nothing to do with instant, so making it part
> BookmarkBarViewInstantExtendedTest is confusing. Can you use
> BrowserWithTestWindowTest directly and name the test more appropriately. I'm
> fine with a generic BookmarkBarViewTest if you want.

Done.

[sky, 2014-06-03 23:08:50]

LGTM

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:75: class
BookmarkBarViewTest : public BrowserWithTestWindowTest {
Move this inside ifdef. Also, we typically use a typdef for this, eg:

typedef BrowserWithTestWindowTest BookmarkBarViewTest;

[bartfab (slow), 2014-06-04 10:55:20]

policy lgtm

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
(right):

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:343:
std::vector<const BookmarkNode*>());
Nit: #include "components/bookmarks/browser/bookmark_node.h"

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:345: //
By default the pref is not managed and the command is enabled.
Nit: s/default/default,/

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:354:
new base::FundamentalValue(false));
Nit: #include "base/values.h"

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:89: // By
default the pref is not managed and the apps shortcut is shown.
Nit: s/default/default,/

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:97: new
base::FundamentalValue(false));
Nit: #include "base/values.h"

https://codereview.chromium.org/312673006/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/312673006/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:3229: If this setting is not
set then the user can choose to show or hide the apps shortcut from the bookmark
bar context menu.
Nit: The wording is inconsistent with what we normally use (albeit somewhat
inconsistently as well): We normally say "If you enable this setting" or "If
this policy is set," but never "If this setting is set" or "If this setting is
configured."

[Joao da Silva, 2014-06-04 11:13:42]

Thanks for the reviews!

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
File chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc
(right):

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:343:
std::vector<const BookmarkNode*>());
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: #include "components/bookmarks/browser/bookmark_node.h"

Done.

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:345: //
By default the pref is not managed and the command is enabled.
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: s/default/default,/

Done.

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/bookma...
chrome/browser/ui/bookmarks/bookmark_context_menu_controller_unittest.cc:354:
new base::FundamentalValue(false));
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:75: class
BookmarkBarViewTest : public BrowserWithTestWindowTest {
On 2014/06/03 23:08:50, sky wrote:
> Move this inside ifdef. Also, we typically use a typdef for this, eg:
> 
> typedef BrowserWithTestWindowTest BookmarkBarViewTest;

Done.

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:89: // By
default the pref is not managed and the apps shortcut is shown.
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: s/default/default,/

Done.

https://codereview.chromium.org/312673006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:97: new
base::FundamentalValue(false));
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/312673006/diff/20001/components/policy/resour...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/312673006/diff/20001/components/policy/resour...
components/policy/resources/policy_templates.json:3229: If this setting is not
set then the user can choose to show or hide the apps shortcut from the bookmark
bar context menu.
On 2014/06/04 10:55:20, bartfab wrote:
> Nit: The wording is inconsistent with what we normally use (albeit somewhat
> inconsistently as well): We normally say "If you enable this setting" or "If
> this policy is set," but never "If this setting is set" or "If this setting is
> configured."

Done.

[Joao da Silva, 2014-06-04 11:13:45]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-04 11:14:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/312673006/30001

[commit-bot: I haz the power, 2014-06-04 14:33:51]

Change committed as 274822