Improve web-app type safety.

Improve web-app typing.

BUG=None
TEST=Manual

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106072

[Jamie, 2011-10-18 00:19:27]

PTAL.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (left):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:19: (function() {
JSCompiler doesn't recognise types inside anonymous functions so I've removed
most of them. They weren't buying us much anyway.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:330: * @private
This is called externally.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:151: /** @type {remoting.ClientSession}
*/ var that = this;
I don't know why the compiler can't infer the type of |that|, but it seems that
it can't. This annotation crops up a few times.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:187: if (this.plugin) {
We've stored the plugin already--not sure why we were looking it up again.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:274: var onIq = function(stanza) {
It's fiddly to add type information to anonymous functions, so I've done this
simple transformation a few times.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:310: } else if (error ==
this.plugin.ERROR_NETWORK_FAILURE) {
A genuine error! My efforts were not wasted after all.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:389: parentNode.style.left = '0';
Setting this to 0 works fine, but the property should actually a string.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:25:
remoting.DebugLog.prototype.MAX_DEBUG_LOG_SIZE = 1000;
Getting rid of the scoping function would push this into the global namespace
otherwise.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/l10n.js
File remoting/webapp/me2mom/l10n.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/l10n.js#...
remoting/webapp/me2mom/l10n.js:47: /** @type {Element} */ var element =
elements[i];
Not sure why this is needed, since the compiler knows the type of elements.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (left):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:39: remoting.EMAIL = 'email';
This is no longer used.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:25: /** @type {string} */ remoting.username =
'';
This while file needs refactoring, but for now just pre-declaring these globals
make the intent more explicit that having them created at first use.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:455: var tmp = /** @type {{description:
string}} */ error;
This is horrible, but I couldn't find any other way of adding type annotations
to an exception.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:633: if (host.data && host.data.jabberId &&
host.data.publicKey) {
I've updated the test to match the type cast above so that it's safe.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/xhr.js
File remoting/webapp/me2mom/xhr.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/xhr.js#n...
remoting/webapp/me2mom/xhr.js:54: /** @type {XMLHttpRequest} */
One might think that the compiler could infer this, but no...

[simonmorris, 2011-10-18 00:55:36]

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (left):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:19: (function() {
On 2011/10/18 00:19:27, Jamie wrote:
> JSCompiler doesn't recognise types inside anonymous functions so I've removed
> most of them. They weren't buying us much anyway.

OK.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:151: /** @type {remoting.ClientSession}
*/ var that = this;
On 2011/10/18 00:19:27, Jamie wrote:
> I don't know why the compiler can't infer the type of |that|, but it seems
that
> it can't. This annotation crops up a few times.

Probably because |this| is a keyword, not a variable.
If this function is called from as a callback, I think
|this| needn't be a remoting.ClientSession.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:151: /** @type {remoting.ClientSession}
*/ var that = this;
No newline between "...*/" and "var ..."?

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:187: if (this.plugin) {
On 2011/10/18 00:19:27, Jamie wrote:
> We've stored the plugin already--not sure why we were looking it up again.

Are you certain |this| is always what you think it is here?
I think it depends how removePlugin() is invoked. That might
be the reason |plugin| used to be looked up - or it might
just be inadvertent, as you say.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:274: var onIq = function(stanza) {
On 2011/10/18 00:19:27, Jamie wrote:
> It's fiddly to add type information to anonymous functions, so I've done this
> simple transformation a few times.

OK.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:23: /** Maximum numer of lines to record in
the debug log. Only the most
"numer" -> "number"

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:46: 'use strict';
Is it better to have 'use strict' at the top of the file?

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:455: var tmp = /** @type {{description:
string}} */ error;
On 2011/10/18 00:19:27, Jamie wrote:
> This is horrible, but I couldn't find any other way of adding type annotations
> to an exception.

It's OK - maybe rename tmp to errorTyped, to make your
intention clearer?

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:633: if (host.data && host.data.jabberId &&
host.data.publicKey) {
On 2011/10/18 00:19:27, Jamie wrote:
> I've updated the test to match the type cast above so that it's safe.

OK.

[Jamie, 2011-10-18 01:42:01]

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:151: /** @type {remoting.ClientSession}
*/ var that = this;
On 2011/10/18 00:55:36, simonmorris wrote:
> On 2011/10/18 00:19:27, Jamie wrote:
> > I don't know why the compiler can't infer the type of |that|, but it seems
> that
> > it can't. This annotation crops up a few times.
> 
> Probably because |this| is a keyword, not a variable.
> If this function is called from as a callback, I think
> |this| needn't be a remoting.ClientSession.

Good point about the callback behaviour. I'd forgotten what an unholy hack
|this| is in Javascript.

Regarding the new-line, I've tended to put type annotations on the same line, as
they won't be in a typed languages. Where I'm annotating function parameters,
I've tended to put them above the function as there are often more than one.
That said, I suspect I haven't been very consistent about it.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:187: if (this.plugin) {
On 2011/10/18 00:55:36, simonmorris wrote:
> On 2011/10/18 00:19:27, Jamie wrote:
> > We've stored the plugin already--not sure why we were looking it up again.
> 
> Are you certain |this| is always what you think it is here?
> I think it depends how removePlugin() is invoked. That might
> be the reason |plugin| used to be looked up - or it might
> just be inadvertent, as you say.

Previously the function referred to both this.plugin and the version it had
looked up, so I'm pretty sure it's a case of different parts of the function
being written at different times.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
File remoting/webapp/me2mom/debug_log.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/debug_lo...
remoting/webapp/me2mom/debug_log.js:23: /** Maximum numer of lines to record in
the debug log. Only the most
On 2011/10/18 00:55:36, simonmorris wrote:
> "numer" -> "number"

Done.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting.js
File remoting/webapp/me2mom/remoting.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:46: 'use strict';
On 2011/10/18 00:55:36, simonmorris wrote:
> Is it better to have 'use strict' at the top of the file?

It doesn't matter, but yes, probably. Done.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/remoting...
remoting/webapp/me2mom/remoting.js:455: var tmp = /** @type {{description:
string}} */ error;
On 2011/10/18 00:55:36, simonmorris wrote:
> On 2011/10/18 00:19:27, Jamie wrote:
> > This is horrible, but I couldn't find any other way of adding type
annotations
> > to an exception.
> 
> It's OK - maybe rename tmp to errorTyped, to make your
> intention clearer?

Done.

[simonmorris, 2011-10-18 15:26:02]

LGTM

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
File remoting/webapp/me2mom/client_session.js (right):

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:151: /** @type {remoting.ClientSession}
*/ var that = this;
On 2011/10/18 01:42:01, Jamie wrote:
> On 2011/10/18 00:55:36, simonmorris wrote:
> > On 2011/10/18 00:19:27, Jamie wrote:
> > > I don't know why the compiler can't infer the type of |that|, but it seems
> > that
> > > it can't. This annotation crops up a few times.
> > 
> > Probably because |this| is a keyword, not a variable.
> > If this function is called from as a callback, I think
> > |this| needn't be a remoting.ClientSession.
> 
> Good point about the callback behaviour. I'd forgotten what an unholy hack
> |this| is in Javascript.
> 
> Regarding the new-line, I've tended to put type annotations on the same line,
as
> they won't be in a typed languages. Where I'm annotating function parameters,
> I've tended to put them above the function as there are often more than one.
> That said, I suspect I haven't been very consistent about it.

OK.

http://codereview.chromium.org/8336004/diff/1/remoting/webapp/me2mom/client_s...
remoting/webapp/me2mom/client_session.js:187: if (this.plugin) {
On 2011/10/18 01:42:01, Jamie wrote:
> On 2011/10/18 00:55:36, simonmorris wrote:
> > On 2011/10/18 00:19:27, Jamie wrote:
> > > We've stored the plugin already--not sure why we were looking it up again.
> > 
> > Are you certain |this| is always what you think it is here?
> > I think it depends how removePlugin() is invoked. That might
> > be the reason |plugin| used to be looked up - or it might
> > just be inadvertent, as you say.
> 
> Previously the function referred to both this.plugin and the version it had
> looked up, so I'm pretty sure it's a case of different parts of the function
> being written at different times.

Right - there's a |this.plugin| in the previous version, so
the function must be invoked in the way that makes |this| do
the right thing. grep confirms.