Add policy indicator for protocol handlers

Add policy indicator for protocol handlers

This CL adds policy indicators to show that a protocol handler is installed by policy.

BUG=384361
Committed: https://crrev.com/0fbe5d023244f05d6c5c1e03eac8576740dac38b
Cr-Commit-Position: refs/heads/master@{#292882}

[kaliamoorthi, 2014-08-29 11:19:40]

kaliamoorthi@chromium.org changed reviewers:
+ bartfab@chromium.org

[kaliamoorthi, 2014-08-29 11:20:02]

Please do a preliminary review.

[bartfab (slow), 2014-08-29 11:23:00]

https://codereview.chromium.org/518673003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/handler_options_list.js (right):

https://codereview.chromium.org/518673003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:169:
indicator.setAttribute('controlled-by', 'hasRecommendation');
You should send the indicator a fake pref change event. This will trigger all
the code that sets it up correctly.

[kaliamoorthi, 2014-08-29 15:07:14]

kaliamoorthi@chromium.org changed reviewers:
+ jhawkins@chromium.org

[kaliamoorthi, 2014-08-29 15:07:14]

PTAL

[James Hawkins, 2014-08-29 20:33:28]

lgtm

[kaliamoorthi, 2014-09-01 08:28:58]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-09-01 08:32:50]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/518673003/20001

[bartfab (slow), 2014-09-01 09:02:05]

The CQ bit was unchecked by bartfab@chromium.org

[bartfab (slow), 2014-09-01 09:38:37]

As discussed offline, an indicator should always be shown when there is a
policy-recommended value (potentially in addition to the "remove" link):

- If there is only a policy-recommended value, clicking on the icon will tell
the user that he/she is following the admin's recommendation.

- If there is a user-set value in addition to the policy-recommended one,
clicking on the icon will offer the user to remove all user-set values and
return to the recommendation.

This is how indicators work for other settings and protocol handlers should be
consistent with it.

https://codereview.chromium.org/518673003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/handler_options_list.js (right):

https://codereview.chromium.org/518673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:154: // Remove link.
Nit: Move the comment inside the conditional. It applies to the first branch
only.

https://codereview.chromium.org/518673003/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:167: var indicator =
new options.ControlledSettingIndicator();
Nit: Add a comment that explains this is a policy indicator.

[kaliamoorthi, 2014-09-01 14:26:18]

PTAL

[bartfab (slow), 2014-09-01 14:56:44]

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry.h:205: // by user.
Nit: s/user/the user/

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry.h:208: // Returns true,
if the scheme has at-least one handler that is registered by
Nit 1: s/,//
Nit 2: s/at-least/at least/

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/handler_options_list.js (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:156: // recommendation.
Nit: s/recommendation/recommendations/

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:160: // indicates that
the user setting has overridden policy
1: Nit: s/policy/a policy/

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:167: // which is
required for a return notification.
Nit: What is a "return notification"?

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:168: // The user
registered handlers are removed in a loop. Note, if a
Nit 1: s/user registered/user-registered/
Nit 2: s/Note,/Note that/

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/handler_options_handler.cc:113:
"registered_by_user",
Nit: It is hard to figure out what the exact semantics of "registered_by_user"
are. I had to ping you offline to undertand. How about something like
"is_default_handler_set_set_by_user"?

[kaliamoorthi, 2014-09-01 15:11:58]

PTAL

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry.h:205: // by user.
On 2014/09/01 14:56:43, bartfab wrote:
> Nit: s/user/the user/

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/custom_ha...
chrome/browser/custom_handlers/protocol_handler_registry.h:208: // Returns true,
if the scheme has at-least one handler that is registered by
On 2014/09/01 14:56:43, bartfab wrote:
> Nit 1: s/,//
> Nit 2: s/at-least/at least/

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/handler_options_list.js (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:156: // recommendation.
On 2014/09/01 14:56:44, bartfab wrote:
> Nit: s/recommendation/recommendations/

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:160: // indicates that
the user setting has overridden policy
On 2014/09/01 14:56:44, bartfab wrote:
> 1: Nit: s/policy/a policy/

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:167: // which is
required for a return notification.
On 2014/09/01 14:56:44, bartfab wrote:
> Nit: What is a "return notification"?

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/handler_options_list.js:168: // The user
registered handlers are removed in a loop. Note, if a
On 2014/09/01 14:56:44, bartfab wrote:
> Nit 1: s/user registered/user-registered/
> Nit 2: s/Note,/Note that/

Done.

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

https://codereview.chromium.org/518673003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/handler_options_handler.cc:113:
"registered_by_user",
On 2014/09/01 14:56:44, bartfab wrote:
> Nit: It is hard to figure out what the exact semantics of "registered_by_user"
> are. I had to ping you offline to undertand. How about something like
> "is_default_handler_set_set_by_user"?

Done.

[kaliamoorthi, 2014-09-01 15:16:45]

PTAL

[bartfab (slow), 2014-09-01 15:54:31]

lgtm

[kaliamoorthi, 2014-09-01 15:56:23]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-09-01 15:57:08]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/518673003/10...

[commit-bot: I haz the power, 2014-09-01 16:55:03]

Committed patchset #4 (id:100001) as 84719860e050bc1f2784daefced432aa8932a24e

[commit-bot: I haz the power, 2014-09-10 03:17:10]

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

[Vitaly Pavlenko, 2014-09-12 02:08:15]

vitalyp@chromium.org changed reviewers:
+ vitalyp@chromium.org

[Vitaly Pavlenko, 2014-09-12 02:08:16]

https://codereview.chromium.org/518673003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/handler_options_list.js (right):

https://codereview.chromium.org/518673003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/handler_options_list.js:172:
delegate.removeHandler(value, data.handlers[i]);
I'm compiling this source with Closure Compiler. Looks like at this point
|value| is not defined. Did you mean |index| or missed the |value| declaration?

[kaliamoorthi, 2014-09-12 08:08:49]

On 2014/09/12 02:08:16, Vitaly Pavlenko wrote:
>
https://codereview.chromium.org/518673003/diff/100001/chrome/browser/resource...
> File chrome/browser/resources/options/handler_options_list.js (right):
> 
>
https://codereview.chromium.org/518673003/diff/100001/chrome/browser/resource...
> chrome/browser/resources/options/handler_options_list.js:172:
> delegate.removeHandler(value, data.handlers[i]);
> I'm compiling this source with Closure Compiler. Looks like at this point
> |value| is not defined. Did you mean |index| or missed the |value|
declaration?

Yeah, we missed it, value is undefined here. It can be |i|.