Add tooltips to cr-policy-indicator

Add tooltips to cr-policy-indicator

Currently paper-tooltips are fairly broken, so use built-in tooltips while we solidify the design and file bugs against paper-tooltip (e.g. https://github.com/PolymerElements/paper-tooltip/issues/34)

BUG=521791
Committed: https://crrev.com/3019d610e251d001db1a77a18f2c46ec108aaf79
Cr-Commit-Position: refs/heads/master@{#350866}

[stevenjb, 2015-09-18 22:40:15]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org, kalman@chromium.org

[stevenjb, 2015-09-18 22:40:16]

kalman@ for small settings_private API change

https://codereview.chromium.org/1342033002/diff/80001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (left):

https://codereview.chromium.org/1342033002/diff/80001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:3161: </message>
(Unreferenced anywhere in the code)

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js
(right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js:35:
Polymer.IronIconsetSvg = Polymer({
I have a pull request to fix this here:
https://github.com/PolymerElements/iron-iconset-svg/pull/20

Is it reasonable to commit this locally in the meanwhile?
Is there another step required to document this somewhere?
Thanks.

[not at google - send to devlin, 2015-09-18 22:42:38]

lgtm

[michaelpg, 2015-09-19 03:04:08]

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js
(right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js:35:
Polymer.IronIconsetSvg = Polymer({
On 2015/09/18 22:40:15, stevenjb wrote:
> I have a pull request to fix this here:
> https://github.com/PolymerElements/iron-iconset-svg/pull/20
> 
> Is it reasonable to commit this locally in the meanwhile?
> Is there another step required to document this somewhere?
> Thanks.

yes, add it to the chromium.patch referenced in
src/third_party/polymer/v1_0/reproduce.sh

[Dan Beam, 2015-09-21 21:50:47]

https://codereview.chromium.org/1342033002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(left):

https://codereview.chromium.org/1342033002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:130:
void AddCommonStrings(content::WebUIDataSource* html_source) {
this was alphabetized

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
File third_party/closure_compiler/externs/settings_private.js (right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
third_party/closure_compiler/externs/settings_private.js:60: *  
policySourceName: (string|undefined),
nit: under policySource or as a member of it?

https://codereview.chromium.org/1342033002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (left):

https://codereview.chromium.org/1342033002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:96:
assertNotReached();
why did you remove this assertNotReached()?

[michaelpg, 2015-09-22 23:57:46]

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

[michaelpg, 2015-09-22 23:57:47]

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js
(right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js:35:
Polymer.IronIconsetSvg = Polymer({
i just discovered this:

@type {IronIconsetSvgElement}

the polymer closure pass automatically creates types (foo-bar -> FooBarElement).

Unless the Polymer call is assigned to a constructor, then that constructor is
used as the type (but in PolymerElements, almost all of the elements don't do
this).

[stevenjb, 2015-09-23 18:46:24]

PTAL

https://codereview.chromium.org/1342033002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(left):

https://codereview.chromium.org/1342033002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:130:
void AddCommonStrings(content::WebUIDataSource* html_source) {
On 2015/09/21 21:50:47, Dan Beam wrote:
> this was alphabetized

I realize, but putting "Common" at top and moving the basic/advanced titles into
it made more sense to me. The rest are page based. I can revert it if you feel
strongly.

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
File third_party/closure_compiler/externs/settings_private.js (right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
third_party/closure_compiler/externs/settings_private.js:60: *  
policySourceName: (string|undefined),
On 2015/09/21 21:50:47, Dan Beam wrote:
> nit: under policySource or as a member of it?

Moved it in the .idl. (PolicySource is an enum)

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js
(right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/iron-iconset-svg/iron-iconset-svg-extracted.js:35:
Polymer.IronIconsetSvg = Polymer({
On 2015/09/22 23:57:46, michaelpg wrote:
> i just discovered this:
> 
> @type {IronIconsetSvgElement}
> 
> the polymer closure pass automatically creates types (foo-bar ->
FooBarElement).
> 
> Unless the Polymer call is assigned to a constructor, then that constructor is
> used as the type (but in PolymerElements, almost all of the elements don't do
> this).

Removed as per discussion.

https://codereview.chromium.org/1342033002/diff/80001/ui/webui/resources/cr_e...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (left):

https://codereview.chromium.org/1342033002/diff/80001/ui/webui/resources/cr_e...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:96:
assertNotReached();
On 2015/09/21 21:50:47, Dan Beam wrote:
> why did you remove this assertNotReached()?

|type| could be undefined here. The assert didn't seem useful so I went with
code simplicity instead.

[michaelpg, 2015-09-23 20:06:32]

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:41: * Polymer
iron-meta instance for icon images
nit: end comment w/ period

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:120:
iconset.applyIcon(this.$.indicator, iconName);
what's the advantage of doing all this instead of using an <iron-icon> and
changing the icon property?

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:134: * @param
{chrome.settingsPrivate.PrefObject} pref
nit: {!c.s.PrefObject}

[stevenjb, 2015-09-23 20:47:20]

PTAL

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:41: * Polymer
iron-meta instance for icon images
On 2015/09/23 20:06:32, michaelpg wrote:
> nit: end comment w/ period

Done.

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:120:
iconset.applyIcon(this.$.indicator, iconName);
On 2015/09/23 20:06:32, michaelpg wrote:
> what's the advantage of doing all this instead of using an <iron-icon> and
> changing the icon property?

You know, I don't remember now. I think that I had thought that I needed to use
<img> to get a built-in tooltip, but then I discovered that I could set title=
on a <div>, and it somehow didn't occur to me that I could just wrap the
iron-icon (which doesn't support title=) with a <div>.

Done.

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:134: * @param
{chrome.settingsPrivate.PrefObject} pref
On 2015/09/23 20:06:32, michaelpg wrote:
> nit: {!c.s.PrefObject}

Done.

[michaelpg, 2015-09-23 20:49:42]

lgtm after comment

https://codereview.chromium.org/1342033002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html
(right):

https://codereview.chromium.org/1342033002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html:12:
<iron-icon id="indicator" icon="[[getIcon_(indicatorType)]]">
dedupe IDs

[Dan Beam, 2015-09-23 20:54:51]

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:120:
iconset.applyIcon(this.$.indicator, iconName);
On 2015/09/23 20:47:20, stevenjb wrote:
> On 2015/09/23 20:06:32, michaelpg wrote:
> > what's the advantage of doing all this instead of using an <iron-icon> and
> > changing the icon property?
> 
> You know, I don't remember now. I think that I had thought that I needed to
use
> <img> to get a built-in tooltip, but then I discovered that I could set title=
> on a <div>, and it somehow didn't occur to me that I could just wrap the
> iron-icon (which doesn't support title=) with a <div>.
> 
> Done.

why doesn't <iron-icon> support title=?
http://jsfiddle.net/dtx40uny/1/

[stevenjb, 2015-09-23 21:12:20]

ptal

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/140001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:120:
iconset.applyIcon(this.$.indicator, iconName);
On 2015/09/23 20:54:51, Dan Beam wrote:
> On 2015/09/23 20:47:20, stevenjb wrote:
> > On 2015/09/23 20:06:32, michaelpg wrote:
> > > what's the advantage of doing all this instead of using an <iron-icon> and
> > > changing the icon property?
> > 
> > You know, I don't remember now. I think that I had thought that I needed to
> use
> > <img> to get a built-in tooltip, but then I discovered that I could set
title=
> > on a <div>, and it somehow didn't occur to me that I could just wrap the
> > iron-icon (which doesn't support title=) with a <div>.
> > 
> > Done.
> 
> why doesn't <iron-icon> support title=?
> http://jsfiddle.net/dtx40uny/1/

I *swear* I tried that at first...

Whatever, removed the div. Thanks.

https://codereview.chromium.org/1342033002/diff/180001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html
(right):

https://codereview.chromium.org/1342033002/diff/180001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.html:12:
<iron-icon id="indicator" icon="[[getIcon_(indicatorType)]]">
On 2015/09/23 20:49:42, michaelpg wrote:
> dedupe IDs

Done (eliminated both).

[Dan Beam, 2015-09-25 01:47:36]

lgtm

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
File third_party/closure_compiler/externs/settings_private.js (right):

https://codereview.chromium.org/1342033002/diff/80001/third_party/closure_com...
third_party/closure_compiler/externs/settings_private.js:60: *  
policySourceName: (string|undefined),
On 2015/09/23 18:46:24, stevenjb wrote:
> On 2015/09/21 21:50:47, Dan Beam wrote:
> > nit: under policySource or as a member of it?
> 
> Moved it in the .idl. (PolicySource is an enum)
> 

ah, looked like a @struct (object), my bad

https://codereview.chromium.org/1342033002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:131: return
this.i18n_('controlledSettingRecommendedDiffers');
nit: don't else after an if return

[stevenjb, 2015-09-25 16:45:07]

https://codereview.chromium.org/1342033002/diff/200001/ui/webui/resources/cr_...
File ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js (right):

https://codereview.chromium.org/1342033002/diff/200001/ui/webui/resources/cr_...
ui/webui/resources/cr_elements/v1_0/policy/cr_policy_indicator.js:131: return
this.i18n_('controlledSettingRecommendedDiffers');
On 2015/09/25 01:47:36, Dan Beam wrote:
> nit: don't else after an if return

Done.

[stevenjb, 2015-09-25 16:45:11]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-09-25 16:45:11]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
michaelpg@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1342033002/#ps220001
(title: "Feedback")

[commit-bot: I haz the power, 2015-09-25 16:46:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342033002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342033002/220001

[commit-bot: I haz the power, 2015-09-25 17:37:51]

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

[commit-bot: I haz the power, 2015-09-25 17:37:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2015-09-25 17:45:02]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-09-25 17:45:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342033002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342033002/220001

[commit-bot: I haz the power, 2015-09-25 18:19:55]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-25 18:20:30]

Patchset 12 (id:??) landed as
https://crrev.com/3019d610e251d001db1a77a18f2c46ec108aaf79
Cr-Commit-Position: refs/heads/master@{#350866}