Implement extension controlled indicators, starting with New Tab page

Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3

Committed: https://chromium.googlesource.com/chromium/src/+/9d3df9aeeedfdc4e57e3450f3cc6168037766609

[Dan Beam, 2016-11-16 23:32:26]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-17 00:20:00]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-11-17 01:03:30]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-17 01:03:31]

dbeam@chromium.org changed reviewers:
+ dpapad@chromium.org
- tommycli@chromium.org

[Dan Beam, 2016-11-17 01:03:50]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 01:04:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-11-17 01:16:47]

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/on_startup_page/on_startup_browser_proxy.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_browser_proxy.js:12:
OnStartupBrowserProxy.prototype = {
Do we need a new Proxy class, given that there is already
startup_urls_page_browser_proxy.js in the same folder? On a related note, I've
found the on_startup_page and startup_urls_page distinction unnecessary. Perhaps
we could simply merge those two elements to one, and then having on proxy for
both would be more natural.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:23:
extension-id="[[ntpExtension_.id]]"
Could we pass all params at once as follows?

<extension-controlled-indicator
extension="[[ntpExtension_]]"></<extension-controlled-indicator>

And rename NtpExtension typedef to just Extension?

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/on_startup_handler.cc (right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/on_startup_handler.cc:19:
OnStartupHandler::OnStartupHandler() {}
Same question here. Can we just use the existing
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...
(perhaps rename it too)?

[Dan Beam, 2016-11-17 01:33:11]

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/on_startup_page/on_startup_browser_proxy.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_browser_proxy.js:12:
OnStartupBrowserProxy.prototype = {
On 2016/11/17 01:16:46, dpapad wrote:
> Do we need a new Proxy class, given that there is already
> startup_urls_page_browser_proxy.js in the same folder? On a related note, I've
> found the on_startup_page and startup_urls_page distinction unnecessary.
Perhaps
> we could simply merge those two elements to one, and then having on proxy for
> both would be more natural.

we probably could fold together, yes.  but right now this proxy and proxy and
associated handler have a 1:1:1 mapping (and specific purpose), which I don't
find bad.  it might also load less code in tests if we change the preload URL
(or maybe not if on_startup_page.html pulls in a lot of stuff)

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:23:
extension-id="[[ntpExtension_.id]]"
On 2016/11/17 01:16:46, dpapad wrote:
> Could we pass all params at once as follows?
> 
> <extension-controlled-indicator
> extension="[[ntpExtension_]]"></<extension-controlled-indicator>
> 
> And rename NtpExtension typedef to just Extension?

not really because this information lives on

prefs.<pref_name>.extensionId
prefs.<pref_name>.controlledByName
prefs.<pref_name>.extensionCanBeDisabled

in other cases

[commit-bot: I haz the power, 2016-11-17 01:41:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 01:41:06]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-11-17 01:50:07]

LGTM with nits.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/extension_controlled_indicator.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.html:13:
min-height: 44px;
Can this be a --settings-row-min-height variable?

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/extension_controlled_indicator.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.js:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.js:31:
assert(this.extensionCanBeDisabled);
Can you verify that C++ is also checking for this at some layer? Otherwise a
user could maybe disable an extension using the dev console.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:23:
extension-id="[[ntpExtension_.id]]"
On 2016/11/17 at 01:33:11, Dan Beam wrote:
> On 2016/11/17 01:16:46, dpapad wrote:
> > Could we pass all params at once as follows?
> > 
> > <extension-controlled-indicator
> > extension="[[ntpExtension_]]"></<extension-controlled-indicator>
> > 
> > And rename NtpExtension typedef to just Extension?
> 
> not really because this information lives on
> 
> prefs.<pref_name>.extensionId
> prefs.<pref_name>.controlledByName
> prefs.<pref_name>.extensionCanBeDisabled
> 
> in other cases

Hm, ok. We could still package the 3 variables into a single object and pass it
in. I guess either way the interface of extension-controlled-indicator would
favor some cases over the others.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.js:37:
OPEN_NEW_TAB: 5,
Nit: Alphabetize? Or sort by value?

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.js:44:
attached: function() {
@override

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/on_startup_handler.cc (right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/on_startup_handler.cc:48:
->management_policy()->MustRemainEnabled(ntp_extension, nullptr));
Nit: Having the "->" on this line instead of previous seems a bit odd. Maybe run
this by clang-format?

[Dan Beam, 2016-11-17 02:50:15]

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/extension_controlled_indicator.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.html:13:
min-height: 44px;
On 2016/11/17 01:50:07, dpapad wrote:
> Can this be a --settings-row-min-height variable? 

Done.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/controls/extension_controlled_indicator.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.js:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/11/17 01:50:07, dpapad wrote:
> 2016

Done.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/controls/extension_controlled_indicator.js:31:
assert(this.extensionCanBeDisabled);
On 2016/11/17 01:50:07, dpapad wrote:
> Can you verify that C++ is also checking for this at some layer? Otherwise a
> user could maybe disable an extension using the dev console.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_serv...

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.html
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.html:23:
extension-id="[[ntpExtension_.id]]"
On 2016/11/17 01:50:07, dpapad wrote:
> On 2016/11/17 at 01:33:11, Dan Beam wrote:
> > On 2016/11/17 01:16:46, dpapad wrote:
> > > Could we pass all params at once as follows?
> > > 
> > > <extension-controlled-indicator
> > > extension="[[ntpExtension_]]"></<extension-controlled-indicator>
> > > 
> > > And rename NtpExtension typedef to just Extension?
> > 
> > not really because this information lives on
> > 
> > prefs.<pref_name>.extensionId
> > prefs.<pref_name>.controlledByName
> > prefs.<pref_name>.extensionCanBeDisabled
> > 
> > in other cases
> 
> Hm, ok. We could still package the 3 variables into a single object and pass
it
> in. I guess either way the interface of extension-controlled-indicator would
> favor some cases over the others.

Acknowledged.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/on_startup_page/on_startup_page.js
(right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.js:37:
OPEN_NEW_TAB: 5,
On 2016/11/17 01:50:07, dpapad wrote:
> Nit: Alphabetize? Or sort by value?

Done.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/on_startup_page/on_startup_page.js:44:
attached: function() {
On 2016/11/17 01:50:07, dpapad wrote:
> @override

Done.

[Dan Beam, 2016-11-17 02:51:48]

whoops, missed a couple

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/on_startup_handler.cc (right):

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/on_startup_handler.cc:19:
OnStartupHandler::OnStartupHandler() {}
On 2016/11/17 01:16:46, dpapad wrote:
> Same question here. Can we just use the existing
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...
> (perhaps rename it too)?

Acknowledged.

https://codereview.chromium.org/2507003005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/on_startup_handler.cc:48:
->management_policy()->MustRemainEnabled(ntp_extension, nullptr));
On 2016/11/17 01:50:07, dpapad wrote:
> Nit: Having the "->" on this line instead of previous seems a bit odd. Maybe
run
> this by clang-format?

Done.

[Dan Beam, 2016-11-17 02:53:29]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 02:55:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-11-17 03:09:39]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
==========

[commit-bot: I haz the power, 2016-11-17 03:30:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 03:30:26]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Dan Beam, 2016-11-17 04:14:16]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=tommycli@chromium.org
BUG=614265
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265
==========

[Dan Beam, 2016-11-17 04:55:40]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-17 05:02:56]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # multiple Android try bot failures ... when this code doesn't even
run on Android
==========

[Dan Beam, 2016-11-17 05:03:09]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-11-17 05:03:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2507003005/#ps60001
(title: "git cl format")

[commit-bot: I haz the power, 2016-11-17 05:03:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 05:10:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 05:10:41]

Failed to apply patch for
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:69
  error:
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:
patch does not apply

Patch:      
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
Index:
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
diff --git
a/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
index
af6faf9b5ae7dc62c9d20f28b08b0a41eaaeb4be..08c10a5ae63a34a0990d2131263e7b30cc25fd2f
100644
--- a/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
+++ b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
@@ -69,19 +69,20 @@ void AddLocalizedStringsBulk(content::WebUIDataSource*
html_source,
 void AddCommonStrings(content::WebUIDataSource* html_source, Profile* profile)
{
   LocalizedString localized_strings[] = {
       {"add", IDS_ADD},
+      {"advancedPageTitle", IDS_SETTINGS_ADVANCED},
       {"back", IDS_ACCNAME_BACK},
+      {"basicPageTitle", IDS_SETTINGS_BASIC},
       {"cancel", IDS_CANCEL},
       {"close", IDS_CLOSE},
       {"confirm", IDS_CONFIRM},
+      {"controlledByExtension", IDS_SETTINGS_CONTROLLED_BY_EXTENSION},
       {"disable", IDS_DISABLE},
       {"learnMore", IDS_LEARN_MORE},
+      {"menuButtonLabel", IDS_SETTINGS_MENU_BUTTON_LABEL},
       {"ok", IDS_OK},
+      {"restart", IDS_SETTINGS_RESTART},
       {"save", IDS_SAVE},
-      {"advancedPageTitle", IDS_SETTINGS_ADVANCED},
-      {"basicPageTitle", IDS_SETTINGS_BASIC},
       {"settings", IDS_SETTINGS_SETTINGS},
-      {"restart", IDS_SETTINGS_RESTART},
-      {"menuButtonLabel", IDS_SETTINGS_MENU_BUTTON_LABEL},
   };
   AddLocalizedStringsBulk(html_source, localized_strings,
                           arraysize(localized_strings));

[Dan Beam, 2016-11-17 05:15:22]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # multiple Android try bot failures ... when this code doesn't even
run on Android
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-17 05:28:04]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3
==========

[commit-bot: I haz the power, 2016-11-17 05:37:09]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3

Committed: https://crrev.com/9d3df9aeeedfdc4e57e3450f3cc6168037766609
Cr-Commit-Position: refs/heads/master@{#432764}
==========

[commit-bot: I haz the power, 2016-11-17 05:37:11]

Patchset 4 (id:??) landed as
https://crrev.com/9d3df9aeeedfdc4e57e3450f3cc6168037766609
Cr-Commit-Position: refs/heads/master@{#432764}

[Dan Beam, 2016-11-17 05:38:40]

Description was changed from

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3

Committed: https://crrev.com/9d3df9aeeedfdc4e57e3450f3cc6168037766609
Cr-Commit-Position: refs/heads/master@{#432764}
==========

to

==========
Implement extension controlled indicators, starting with New Tab page

R=dpapad@chromium.org
BUG=614265

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
NOTRY=true  # fixed a merge, see patchset 3

Committed:
https://chromium.googlesource.com/chromium/src/+/9d3df9aeeedfdc4e57e3450f3cc6...
==========

[Dan Beam, 2016-11-17 05:38:42]

Committed patchset #4 (id:80001) manually as
9d3df9aeeedfdc4e57e3450f3cc6168037766609 (presubmit successful).