remoting: Fix JSCompiler issues blocking ES6 transition.

remoting: Fix JSCompiler issues blocking ES6 transition.

* @suppress reportUnknownTypes for code using |arguments|, which is not well-handled by the JSCompiler type system.
* Fix up ad hoc documentation into proper JSDoc annotation so the type system doesn't get hung up.

R=garykac@chromium.org
BUG=none

Committed: https://crrev.com/71f8e5677f9fc34d75a25acf94a19e7615483afa
Cr-Commit-Position: refs/heads/master@{#336044}

[garykac, 2015-06-24 22:02:39]

Could you add more detail to the description. The current "Fix issues" comment
is OK for the first-line-summary, but the body should contain more info about
what changes you actually made to fix the problem. In this case, adding suppress
and moving inline param types to use @param.

[James Hawkins, 2015-06-24 22:05:42]

On 2015/06/24 22:02:39, garykac wrote:
> Could you add more detail to the description. The current "Fix issues" comment
> is OK for the first-line-summary, but the body should contain more info about
> what changes you actually made to fix the problem. In this case, adding
suppress
> and moving inline param types to use @param.

Done.

[garykac, 2015-06-24 22:36:47]

lgtm

[James Hawkins, 2015-06-24 23:24:12]

The CQ bit was checked by jhawkins@chromium.org

[commit-bot: I haz the power, 2015-06-24 23:25:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1204023003/20001

[commit-bot: I haz the power, 2015-06-24 23:37:20]

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

[commit-bot: I haz the power, 2015-06-24 23:37:20]

Exceeded global retry quota

[Jamie, 2015-06-24 23:50:18]

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

[Jamie, 2015-06-24 23:50:18]

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
Can you add something to the CL description explaining why this is a net win? On
the face of it, we're losing type checking here; I hope that's not actually the
case, but either way, the CL should explain why we're doing it, especially with
no bug to refer to.

[James Hawkins, 2015-06-25 00:00:44]

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
On 2015/06/24 23:50:18, Jamie wrote:
> Can you add something to the CL description explaining why this is a net win?
On
> the face of it, we're losing type checking here; I hope that's not actually
the
> case, but either way, the CL should explain why we're doing it, especially
with
> no bug to refer to.

You're not losing type checking.  You're suppressing a warning for type
operations the compiler does not know about.  In every suppression in this CL,
the issue revolves around the use of |arguments|, whose type is not well-handled
by the JSCompiler (which I wrote in the description).

[James Hawkins, 2015-06-25 00:04:38]

The CQ bit was checked by jhawkins@chromium.org

[commit-bot: I haz the power, 2015-06-25 00:05:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1204023003/20001

[Jamie, 2015-06-25 00:10:49]

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/base.js (right):

https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
On 2015/06/25 00:00:44, James Hawkins wrote:
> On 2015/06/24 23:50:18, Jamie wrote:
> > Can you add something to the CL description explaining why this is a net
win?
> On
> > the face of it, we're losing type checking here; I hope that's not actually
> the
> > case, but either way, the CL should explain why we're doing it, especially
> with
> > no bug to refer to.
> 
> You're not losing type checking.  You're suppressing a warning for type
> operations the compiler does not know about.  In every suppression in this CL,
> the issue revolves around the use of |arguments|, whose type is not
well-handled
> by the JSCompiler (which I wrote in the description).

So am I right in thinking that the old compiler (or flags) did not enforce this
type check, but neither did it issue a warning,  whereas the new version issues
a warning, which we must suppress? If that's the case, then I have no objection;
it just wasn't clear without context that that's what's going on here.

[commit-bot: I haz the power, 2015-06-25 00:12:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-25 00:13:29]

Patchset 2 (id:??) landed as
https://crrev.com/71f8e5677f9fc34d75a25acf94a19e7615483afa
Cr-Commit-Position: refs/heads/master@{#336044}

[James Hawkins, 2015-06-25 16:52:26]

On 2015/06/25 00:10:49, Jamie wrote:
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> File remoting/webapp/base/js/base.js (right):
> 
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
> On 2015/06/25 00:00:44, James Hawkins wrote:
> > On 2015/06/24 23:50:18, Jamie wrote:
> > > Can you add something to the CL description explaining why this is a net
> win?
> > On
> > > the face of it, we're losing type checking here; I hope that's not
actually
> > the
> > > case, but either way, the CL should explain why we're doing it, especially
> > with
> > > no bug to refer to.
> > 
> > You're not losing type checking.  You're suppressing a warning for type
> > operations the compiler does not know about.  In every suppression in this
CL,
> > the issue revolves around the use of |arguments|, whose type is not
> well-handled
> > by the JSCompiler (which I wrote in the description).
> 
> So am I right in thinking that the old compiler (or flags) did not enforce
this
> type check, but neither did it issue a warning,  whereas the new version
issues
> a warning, which we must suppress? If that's the case, then I have no
objection;
> it just wasn't clear without context that that's what's going on here.

+tbrei, can you provide any insights here?  My understanding is that there was
no type checking for arguments in the ES5 mode, but it didn't complain either. 
In ES6 mode we still have no type checking for arguments, but for some reason or
other it shows up for the reportUnknownTypes pass.

[Tyler Breisacher (Chromium), 2015-06-25 17:09:41]

On 2015/06/25 16:52:26, James Hawkins wrote:
> On 2015/06/25 00:10:49, Jamie wrote:
> >
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> > File remoting/webapp/base/js/base.js (right):
> > 
> >
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> > remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
> > On 2015/06/25 00:00:44, James Hawkins wrote:
> > > On 2015/06/24 23:50:18, Jamie wrote:
> > > > Can you add something to the CL description explaining why this is a net
> > win?
> > > On
> > > > the face of it, we're losing type checking here; I hope that's not
> actually
> > > the
> > > > case, but either way, the CL should explain why we're doing it,
especially
> > > with
> > > > no bug to refer to.
> > > 
> > > You're not losing type checking.  You're suppressing a warning for type
> > > operations the compiler does not know about.  In every suppression in this
> CL,
> > > the issue revolves around the use of |arguments|, whose type is not
> > well-handled
> > > by the JSCompiler (which I wrote in the description).
> > 
> > So am I right in thinking that the old compiler (or flags) did not enforce
> this
> > type check, but neither did it issue a warning,  whereas the new version
> issues
> > a warning, which we must suppress? If that's the case, then I have no
> objection;
> > it just wasn't clear without context that that's what's going on here.
> 
> +tbrei, can you provide any insights here?  My understanding is that there was
> no type checking for arguments in the ES5 mode, but it didn't complain either.

> In ES6 mode we still have no type checking for arguments, but for some reason
or
> other it shows up for the reportUnknownTypes pass.

I'll look into this. It's possible that we get slightly less-than-optimal
typechecking coverage after ES6-to-ES5 transpilation. But in this case you're
not using any ES6 features yet so that shouldn't happen.

In any case I don't think this is cause for concern. You will still get
typechecking for the vast majority of the code, even in the methods that have
that @suppress.

[Tyler Breisacher (Chromium), 2015-06-25 17:12:03]

On 2015/06/25 17:09:41, Tyler Breisacher (Chromium) wrote:
> On 2015/06/25 16:52:26, James Hawkins wrote:
> > On 2015/06/25 00:10:49, Jamie wrote:
> > >
> >
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> > > File remoting/webapp/base/js/base.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1204023003/diff/20001/remoting/webapp/base/js...
> > > remoting/webapp/base/js/base.js:68: * @suppress {reportUnknownTypes}
> > > On 2015/06/25 00:00:44, James Hawkins wrote:
> > > > On 2015/06/24 23:50:18, Jamie wrote:
> > > > > Can you add something to the CL description explaining why this is a
net
> > > win?
> > > > On
> > > > > the face of it, we're losing type checking here; I hope that's not
> > actually
> > > > the
> > > > > case, but either way, the CL should explain why we're doing it,
> especially
> > > > with
> > > > > no bug to refer to.
> > > > 
> > > > You're not losing type checking.  You're suppressing a warning for type
> > > > operations the compiler does not know about.  In every suppression in
this
> > CL,
> > > > the issue revolves around the use of |arguments|, whose type is not
> > > well-handled
> > > > by the JSCompiler (which I wrote in the description).
> > > 
> > > So am I right in thinking that the old compiler (or flags) did not enforce
> > this
> > > type check, but neither did it issue a warning,  whereas the new version
> > issues
> > > a warning, which we must suppress? If that's the case, then I have no
> > objection;
> > > it just wasn't clear without context that that's what's going on here.
> > 
> > +tbrei, can you provide any insights here?  My understanding is that there
was
> > no type checking for arguments in the ES5 mode, but it didn't complain
either.
> 
> > In ES6 mode we still have no type checking for arguments, but for some
reason
> or
> > other it shows up for the reportUnknownTypes pass.
> 
> I'll look into this. It's possible that we get slightly less-than-optimal
> typechecking coverage after ES6-to-ES5 transpilation. But in this case you're
> not using any ES6 features yet so that shouldn't happen.
> 
> In any case I don't think this is cause for concern. You will still get
> typechecking for the vast majority of the code, even in the methods that have
> that @suppress.

Filed https://github.com/google/closure-compiler/issues/1021