[MediaRouter UI] Compile MR WebUI js code with Closure.

[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
Committed: https://crrev.com/812954945f854271f645194b7cad101bd5142ab6
Cr-Commit-Position: refs/heads/master@{#369839}

[imcheng, 2015-12-10 21:37:20]

imcheng@chromium.org changed reviewers:
+ dbeam@chromium.org

[imcheng, 2015-12-10 21:37:22]

Hey Dan,

I am trying to use Closure compiler to do typechecking for our Media Router
WebUI code, and have a few questions for you in comments below. PTAL, thanks!

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/compiled_resources.gyp (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/compiled_resources.gyp:10:
'../../../../third_party/polymer/v1_0/components-chromium/iron-a11y-keys-behavior/iron-a11y-keys-behavior-extracted.js',
Do I really need to include the transitive closure of all polymer html imports
in my code? Or can it be done in a cleaner way?

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:193: * @param
{number} iconType the type of icon for the sink.
The compiler won't let me use media_router.SinkIconType here. Looking at other
projects' code it seems it is WAI?

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
File
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js
(right):

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js:253:
_getValidity: function(ignored) {
This function overrides _getValidity from IronValidatableBehavior which takes an
Object param. I had to make this change to get rid of compiler error. Perhaps
changes in this file should be done separately? What's the process?

[Dan Beam, 2015-12-10 23:41:36]

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(left):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:433:
* @param {media_router.MediaRouterView} view The current view.
why are you undoing this?

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router.js (left):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router.js:63: * @param {{detail:
{sinkId: string, selectedCastModeValue: number}}} data
same question: why are you undoing this?

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router.js:53: *   helpPageId
(number) - the numeric help center ID.
this is not better than what you had before...

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:58: };
^ just move these out of the cr.define(), and if you want them to stay
namespaced

cr.exportPath('media_router');

/** @enum {number} */
media_router.SinkIconType = {...};

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:111: var
STOP_CASTING_BUTTON_TEXT = loadTimeData.getString('stopCastingButton');
why are these better?  also, have you see I18nBehavior?

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
File
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js
(right):

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js:253:
_getValidity: function(ignored) {
On 2015/12/10 21:37:21, imcheng1 wrote:
> This function overrides _getValidity from IronValidatableBehavior which takes
an
> Object param. I had to make this change to get rid of compiler error. Perhaps
> changes in this file should be done separately? What's the process?

don't change things that get overwritten every time we roll polymer ;) send them
a PR instead

[imcheng, 2015-12-11 01:14:59]

Thanks. I had one more question about including the polymer js files in .gyp.

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(left):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:433:
* @param {media_router.MediaRouterView} view The current view.
On 2015/12/10 23:41:34, Dan Beam wrote:
> why are you undoing this?

The compiler does not recognize the type. MediaRouterView is an @enum {string}
so I replaced it with string. See also comment in media_router_data.js L193.

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router.js (left):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router.js:63: * @param {{detail:
{sinkId: string, selectedCastModeValue: number}}} data
On 2015/12/10 23:41:34, Dan Beam wrote:
> same question: why are you undoing this?

See above

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router.js:53: *   helpPageId
(number) - the numeric help center ID.
On 2015/12/10 23:41:34, Dan Beam wrote:
> this is not better than what you had before...

Okay. But Event is not the same as {{detail: {id: string, actionType: number,
helpPageId: number}}} because it has other properties besides 'detail', which
causes compiler to complain. I tried to compensate for that by adding the types
here though they're worse than annotations.

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:58: };
On 2015/12/10 23:41:34, Dan Beam wrote:
> ^ just move these out of the cr.define(), and if you want them to stay
> namespaced
> 
> cr.exportPath('media_router');
> 
> /** @enum {number} */
> media_router.SinkIconType = {...};

Done.

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:111: var
STOP_CASTING_BUTTON_TEXT = loadTimeData.getString('stopCastingButton');
On 2015/12/10 23:41:34, Dan Beam wrote:
> why are these better?  also, have you see I18nBehavior?

Calling loadtimeData from Polymer properties doesn't work: "ERROR - No
properties on this expression"

I could make the value be function() { return loadtimeData.getString(...); }.
But since the string is more of a static constant I wonder if that's the right
thing to do? Or it doesn't matter since media-router-container is supposedly a
singleton?

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:193: * @param
{number} iconType the type of icon for the sink.
On 2015/12/10 21:37:21, imcheng1 wrote:
> The compiler won't let me use media_router.SinkIconType here. Looking at other
> projects' code it seems it is WAI?

Moving the enum out of cr.define and using cr.export solves the problem.

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
File
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js
(right):

https://codereview.chromium.org/1521433002/diff/1/third_party/polymer/v1_0/co...
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js:253:
_getValidity: function(ignored) {
On 2015/12/10 23:41:36, Dan Beam wrote:
> On 2015/12/10 21:37:21, imcheng1 wrote:
> > This function overrides _getValidity from IronValidatableBehavior which
takes
> an
> > Object param. I had to make this change to get rid of compiler error.
Perhaps
> > changes in this file should be done separately? What's the process?
> 
> don't change things that get overwritten every time we roll polymer ;) send
them
> a PR instead

Acknowledged. It looks like somebody already has a PR but it's not so easy :-\
https://github.com/PolymerElements/paper-dropdown-menu/pull/65

I will leave this here for now but I won't check this in.

[Dan Beam, 2015-12-11 04:58:46]

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:111: var
STOP_CASTING_BUTTON_TEXT = loadTimeData.getString('stopCastingButton');
On 2015/12/11 01:14:59, imcheng1 wrote:
> On 2015/12/10 23:41:34, Dan Beam wrote:
> > why are these better?  also, have you see I18nBehavior?
> 
> Calling loadtimeData from Polymer properties doesn't work: "ERROR - No
> properties on this expression"
> 
> I could make the value be function() { return loadtimeData.getString(...); }.
> But since the string is more of a static constant I wonder if that's the right
> thing to do? Or it doesn't matter since media-router-container is supposedly a
> singleton?

you just need to include load_time_data.js, string.js, and i18n_template.js
before this template

also, yes: you could defer creating the property with:

  properties: {
    i18n_: function() {
      return {
        MSG1: loadTimeData.getString('msg1'),
        ...
      },
    },
  },

and bind to

  [[i18n_.MSG1]]

as well as the single value format like you mentioned:

  properties: {
    MSG1: {
      type: String,
      value: function() { return loadTimeData.getString('msg1'); },
    },
  },

https://codereview.chromium.org/1521433002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js
(right):

https://codereview.chromium.org/1521433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:64:
* @param {media_router.MediaRouterView} view The current view.
can you add ? or ! to be more explicit about the nullability here?

[imcheng, 2015-12-11 20:00:46]

I will follow up on the pull request.

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/1/chrome/browser/resources/me...
chrome/browser/resources/media_router/media_router_data.js:111: var
STOP_CASTING_BUTTON_TEXT = loadTimeData.getString('stopCastingButton');
On 2015/12/11 04:58:46, Dan Beam wrote:
> On 2015/12/11 01:14:59, imcheng1 wrote:
> > On 2015/12/10 23:41:34, Dan Beam wrote:
> > > why are these better?  also, have you see I18nBehavior?
> > 
> > Calling loadtimeData from Polymer properties doesn't work: "ERROR - No
> > properties on this expression"
> > 
> > I could make the value be function() { return loadtimeData.getString(...);
}.
> > But since the string is more of a static constant I wonder if that's the
right
> > thing to do? Or it doesn't matter since media-router-container is supposedly
a
> > singleton?
> 
> you just need to include load_time_data.js, string.js, and i18n_template.js
> before this template
> 
> also, yes: you could defer creating the property with:
> 
>   properties: {
>     i18n_: function() {
>       return {
>         MSG1: loadTimeData.getString('msg1'),
>         ...
>       },
>     },
>   },
> 
> and bind to
> 
>   [[i18n_.MSG1]]
> 
> as well as the single value format like you mentioned:
> 
>   properties: {
>     MSG1: {
>       type: String,
>       value: function() { return loadTimeData.getString('msg1'); },
>     },
>   },

Done. Went with latter.

https://codereview.chromium.org/1521433002/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js
(right):

https://codereview.chromium.org/1521433002/diff/40001/chrome/browser/resource...
chrome/browser/resources/media_router/elements/media_router_header/media_router_header.js:64:
* @param {media_router.MediaRouterView} view The current view.
On 2015/12/11 04:58:46, Dan Beam wrote:
> can you add ? or ! to be more explicit about the nullability here?

Done.

[Dan Beam, 2015-12-12 01:43:02]

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router.js:49: * @param {!Event} data
i would change this back to what it was because you're using this information as
data, not as an event

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:184: /** @type
{media_router.SinkIconType} */
nit: ? or !

https://codereview.chromium.org/1521433002/diff/60001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js
(right):

https://codereview.chromium.org/1521433002/diff/60001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js:253:
_getValidity: function(ignored) {
lgtm if you revert this

[imcheng, 2015-12-14 22:46:13]

Description was changed from

==========
[MediaRouter UI / WIP] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=TBD
==========

to

==========
[MediaRouter UI / WIP] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

[imcheng, 2015-12-14 22:47:01]

Thanks! I will wait for the polymer fix before submitting.

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router.js:49: * @param {!Event} data
On 2015/12/12 01:43:02, Dan Beam (ooo till 12-26) wrote:
> i would change this back to what it was because you're using this information
as
> data, not as an event

This function expects an Event. What I had before was not correct:

## .../media_router.js:33: ERROR - actual parameter 2 of
Node.prototype.addEventListener does not match formal parameter
## found   : function ({detail: {actionType: number, helpPageId: number, id:
string}}): undefined
## required: (EventListener|function (Event): (boolean|undefined)|null)
##     container.addEventListener('issue-action-click', onIssueActionClick);

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/media_router/media_router_data.js (right):

https://codereview.chromium.org/1521433002/diff/60001/chrome/browser/resource...
chrome/browser/resources/media_router/media_router_data.js:184: /** @type
{media_router.SinkIconType} */
On 2015/12/12 01:43:02, Dan Beam (ooo till 12-26) wrote:
> nit: ? or !

Done.

https://codereview.chromium.org/1521433002/diff/60001/third_party/polymer/v1_...
File
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js
(right):

https://codereview.chromium.org/1521433002/diff/60001/third_party/polymer/v1_...
third_party/polymer/v1_0/components-chromium/paper-dropdown-menu/paper-dropdown-menu-extracted.js:253:
_getValidity: function(ignored) {
On 2015/12/12 01:43:02, Dan Beam (ooo till 12-26) wrote:
> lgtm if you revert this

Done.

[Dan Beam, 2015-12-29 01:25:38]

lgtm

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:540:
return /** @type {string} */(sink.description);
nit: if you're sure this will never be null

  return assert(sink.description);

otherwise, @return {?string}

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router.js:55: function
onIssueActionClick(data) {
I would just cast in these cases, i.e.

  /** @type {{id: string, actionType: number, helpPageId: number}} */
  var details = data.detail;

OR

  var details =
      /** @type {{id: string, actionType: number, helpPageId: number}} */
          (data.details);

if required.

otherwise, how does this compile?  does the compiler not complain about
.detail.* missing?

[imcheng, 2016-01-04 20:02:04]

Thanks. Addressed comments. How can I roll the Polymer fix into Chromium?

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js
(right):

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/elements/media_router_container/media_router_container.js:540:
return /** @type {string} */(sink.description);
On 2015/12/29 01:25:38, Dan Beam wrote:
> nit: if you're sure this will never be null
> 
>   return assert(sink.description);
> 
> otherwise, @return {?string}

Done.

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/media_router/media_router.js (right):

https://codereview.chromium.org/1521433002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media_router/media_router.js:55: function
onIssueActionClick(data) {
On 2015/12/29 01:25:38, Dan Beam wrote:
> I would just cast in these cases, i.e.
> 
>   /** @type {{id: string, actionType: number, helpPageId: number}} */
>   var details = data.detail;
> 
> OR
> 
>   var details =
>       /** @type {{id: string, actionType: number, helpPageId: number}} */
>           (data.details);
> 
> if required.
> 
> otherwise, how does this compile?  does the compiler not complain about
> .detail.* missing?

I am not sure why compiler doesn't complain. Is it because data.detail is just a
generic Object so it doesn't know what properties it has?

[Dan Beam, 2016-01-06 00:27:05]

On 2016/01/04 20:02:04, imcheng1 wrote:
> Thanks. Addressed comments. How can I roll the Polymer fix into Chromium?

third_party/polymer/v1_0/reproduce.sh

[imcheng, 2016-01-15 18:33:54]

Description was changed from

==========
[MediaRouter UI / WIP] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

to

==========
[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

[imcheng, 2016-01-15 20:06:23]

Patchset #10 (id:180001) has been deleted

[imcheng, 2016-01-15 21:00:19]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2016-01-15 21:00:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1521433002/#ps200001
(title: "Rebase again")

[commit-bot: I haz the power, 2016-01-15 21:03:03]

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

[commit-bot: I haz the power, 2016-01-15 21:08:46]

Description was changed from

==========
[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

to

==========
[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

[commit-bot: I haz the power, 2016-01-15 21:08:47]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-01-15 21:10:24]

Description was changed from

==========
[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698
==========

to

==========
[MediaRouter UI] Compile MR WebUI js code with Closure.

Also fix code errors discovered by the compilation.

BUG=569698

Committed: https://crrev.com/812954945f854271f645194b7cad101bd5142ab6
Cr-Commit-Position: refs/heads/master@{#369839}
==========

[commit-bot: I haz the power, 2016-01-15 21:10:25]

Patchset 10 (id:??) landed as
https://crrev.com/812954945f854271f645194b7cad101bd5142ab6
Cr-Commit-Position: refs/heads/master@{#369839}