Update network error template to new design

Update network error template to new design

BUG=414233
Committed: https://crrev.com/f8cb5584fe0d38a3d3f19d4bb8cb92c753f8d1f3
Cr-Commit-Position: refs/heads/master@{#296209}

[edwardjung, 2014-09-15 14:52:10]

edwardjung@chromium.org changed reviewers:
+ sky@chromium.org

[edwardjung, 2014-09-15 14:52:11]

Please see the bug for specific extra details which explains the reasoning
behind files added and why the details aren't added here.

[sky, 2014-09-15 16:15:30]

sky@chromium.org changed reviewers:
+ arv@chromium.org

[sky, 2014-09-15 16:15:31]

LGTM - but I'm adding arv to make sure the html/css changes are ok. Wait for him
to sign off before landing.

[arv (Not doing code reviews), 2014-09-15 17:35:44]

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.css (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.css:50: width: 41px;
Keep these in alphabetical order

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.html:19: <span i18n-content="heading"></span>
Skip this span and move the i18n-content to the h1.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.html:83: <div id="offline-resources">
Can these be moved to js so you don't have to map them or duplicate the
name/lookup?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:132:
document.getElementById('details-button').classList.add('hidden');
How about just using .hidden instead of class hidden?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:182: { name: 'CACTUS_LARGE', id:
'1x-obstacle-large' },
wrong indentation

no ws after {

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:229: ANIM_END: 'webkitAnimationEnd',
skip webkit prefix

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:249: new AudioContext() : new
webkitAudioContext();
wrong indentation

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:249: new AudioContext() : new
webkitAudioContext();
Remove webkit prefix

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:263: 
remove empty line

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:274: var soundSrc =
String(document.getElementById(Runner.sounds[sound]).src);
useless String cast?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:279:
Runner.audioContext['decodeAudioData'](buffer, function(index, audioData) {
Why not Runner.audioContext.decodeAudioData? Are you missing some externs?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:280: this.soundFx[index] = audioData;
wrong indentation

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:295: if (Runner.test.mobile() &&
this.dimensions.WIDTH < Runner.DEFAULT_WIDTH) {
Why bother testing if mobile. The screen width should be sufficient

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:486: var now = new Date().getTime();
Date.now but you are better of using performance.now for better granularity

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:499: this.tRex.updateJump(deltaTime,
this.config);
wrong indent

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:617: case Runner.events.KEYDOWN:
I would just put all of this code in an IIFE and have local constants for these
instead of putting them on Runner.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:694: if
(Runner.keycodes.DUCK[String(e.keyCode)] && this.tRex.jumping) {
useless stringify

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:867: return this.raqId ? true : false;
return !!this.raqId;

[edwardjung, 2014-09-16 13:14:33]

Thanks for the quick review.

I've addressed all the comments.

Wonder if you had any suggestions on:

+ Base64 encoding the assets into the JS itself. Is there a way to get grit to
do this. The method I found didn't seem to work so resorted to having assets on
the page.

+ Checking in a closure compile JS file. It doesn't pass presubmit checks since
it doesn't pass the JS style guide. Can JS compiling be achieved as part of the
build process? I don't see any other JS being compiled so presume not.

Edward

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.css (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.css:50: width: 41px;
On 2014/09/15 17:35:43, arv wrote:
> Keep these in alphabetical order

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.html:19: <span i18n-content="heading"></span>
On 2014/09/15 17:35:43, arv wrote:
> Skip this span and move the i18n-content to the h1.

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.html:83: <div id="offline-resources">
On 2014/09/15 17:35:43, arv wrote:
> Can these be moved to js so you don't have to map them or duplicate the
> name/lookup?

As the page will be offline I can't access the resources via XHR or chrome:// so
they have to be base 64 encoded. I wanted to manually base 64 encode all these
assets and have them in the JS as you suggest but it causes a failure in the
presubmit checks, 80 char width. 

I was also wondering in terms of maintenance whether this was a better solution
since you just leave all the encoding to grit. Ideally the I would just have
grit be able to parse the JS and base 64 encode urls but I couldn't get it
working.

Do you have any other suggestions on this front?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:132:
document.getElementById('details-button').classList.add('hidden');
On 2014/09/15 17:35:43, arv wrote:
> How about just using .hidden instead of class hidden?

Done. Thanks for the tip.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:182: { name: 'CACTUS_LARGE', id:
'1x-obstacle-large' },
On 2014/09/15 17:35:44, arv wrote:
> wrong indentation
> 
> no ws after {

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:229: ANIM_END: 'webkitAnimationEnd',
On 2014/09/15 17:35:43, arv wrote:
> skip webkit prefix

CSS animations are still prefixed in Chrome. The unprefixed event does not work.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:249: new AudioContext() : new
webkitAudioContext();
On 2014/09/15 17:35:43, arv wrote:
> Remove webkit prefix

Done. Just assigned a AudioContext instead.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:249: new AudioContext() : new
webkitAudioContext();
On 2014/09/15 17:35:43, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:263: 
On 2014/09/15 17:35:44, arv wrote:
> remove empty line

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:274: var soundSrc =
String(document.getElementById(Runner.sounds[sound]).src);
On 2014/09/15 17:35:44, arv wrote:
> useless String cast?

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:279:
Runner.audioContext['decodeAudioData'](buffer, function(index, audioData) {
On 2014/09/15 17:35:43, arv wrote:
> Why not Runner.audioContext.decodeAudioData? Are you missing some externs?

Removed. Actually I'm not compiling the file anymore. The closure compiled file
doesn't pass the presubmit checks and didn't feel the extra manual overhead in
maintaining the compiled version was worth it. 

Do you have any suggestions regarding submitting a compiled file.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:280: this.soundFx[index] = audioData;
On 2014/09/15 17:35:43, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:295: if (Runner.test.mobile() &&
this.dimensions.WIDTH < Runner.DEFAULT_WIDTH) {
On 2014/09/15 17:35:43, arv wrote:
> Why bother testing if mobile. The screen width should be sufficient

Done.

We were trying to restrict this to mobile only. However seems like the same
logic should apply to desktop if you happen to resize to a narrow viewport.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:486: var now = new Date().getTime();
On 2014/09/15 17:35:44, arv wrote:
> Date.now but you are better of using performance.now for better granularity

Thanks for the tip. Switched to performance.now.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:499: this.tRex.updateJump(deltaTime,
this.config);
On 2014/09/15 17:35:44, arv wrote:
> wrong indent

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:617: case Runner.events.KEYDOWN:
On 2014/09/15 17:35:44, arv wrote:
> I would just put all of this code in an IIFE and have local constants for
these
> instead of putting them on Runner.

Something like this?

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:694: if
(Runner.keycodes.DUCK[String(e.keyCode)] && this.tRex.jumping) {
On 2014/09/15 17:35:43, arv wrote:
> useless stringify

Done.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:867: return this.raqId ? true : false;
On 2014/09/15 17:35:43, arv wrote:
> return !!this.raqId;

Done.

[arv (Not doing code reviews), 2014-09-16 14:20:21]

Sorry for all the comments. This code is pretty good. I'm just trying to help
you make it perfect.

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/545973003/diff/60001/chrome/renderer/resource...
chrome/renderer/resources/neterror.html:83: <div id="offline-resources">
On 2014/09/16 13:14:30, edwardjung wrote:
> On 2014/09/15 17:35:43, arv wrote:
> > Can these be moved to js so you don't have to map them or duplicate the
> > name/lookup?
> 
> As the page will be offline I can't access the resources via XHR or chrome://
so
> they have to be base 64 encoded. I wanted to manually base 64 encode all these
> assets and have them in the JS as you suggest but it causes a failure in the
> presubmit checks, 80 char width. 
> 
> I was also wondering in terms of maintenance whether this was a better
solution
> since you just leave all the encoding to grit. Ideally the I would just have
> grit be able to parse the JS and base 64 encode urls but I couldn't get it
> working.
> 
> Do you have any other suggestions on this front?

I was thinking doing new Audio(src) and new Image(src)

But now that I understand that you are relying on grit to inline the resources I
can't think of any better solution that what you are already doing.

We could extend grit to do this kind of inlining in js files but I'm not sure it
is worth it.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:138: setButtonLayout();
Wrong indentation. When you line break after function() { you only indent 2,
like this:

document.addEventListener('DOMContentLoaded', function() {
  setButtonLayout();
  if (document.querySelector('.icon-offline')) {
    document.body.classList.add('offline');
    new Runner('.interstitial-wrapper');
  }
});

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:5: /**
Maybe 'use strict' to catch errors?

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:18: var Runner = function(outerContainerId,
opt_config) {
Prefer function declarations where allowed.

function Runner(outerContainerId, opt_config) {

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:62: // Singleton
Do we need this singleton pattern enforced in the code?

If you really want to enforce it you can use the following pattern:

function Runner(...) {
  if (Runner.instance_) {
    return Runner.instance_;
  }
  Runner.instance_ = this;
  ...
}

Since if the function returns an object when you do new Runner, that object is
used instead of the one that was allocated for new.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:70: window['Runner'] = Runner;
I think you should be able to just do @export on Runner

/** @export */
function Runner() {

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:91: Runner.prototype.updateConfigSetting =
function(setting, value) {
How about using an object literal so you do not have to repeat yourself all
over?

Runner.prototype = {
  updateConfigSetting: function(setting, value) {
    ...
  },
  loadImages: function() {
    ...
  }
};

// Statics are not as pretty :'(
Runner.config = { ... };
Runner.DEFAULT_WIDTH = ...;

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:137: Runner.DEFAULT_WIDTH = 600;
Now that you have an IIFE, there is really no reason to make these properties of
Runner. Just do:

var DEFAULT_WIDTH = 600;

or

/** @const */
var DEFAULT_WIDTH = 600;

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:238: Runner.audioContext = new
AudioContext();
Since you are using a singleton, maybe just create this in the constructor?

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:264: var buffer =
Runner.decodeBase64ToArrayBuffer(soundSrc);
Since you are doing the decoding here, you can put your resources inside a
<template> element so that the browser does not have to process them twice (once
as the page loads and once here).

To get the elements out of the template you need an extra step here.

var templateContent = document.getElementById(templateId).content;
for (...) {
  var soundSrc = templateContent.getElementById(soundId).src;
}

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2095: Runner.HorizonLine = function(canvas,
bgImg) {
Why not just HorizontalLine? The IIFE creates the scoping already.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2247: 'CACTUS_SMALL':
images['CACTUS_SMALL'],
Wrong indentation.

this.obstacleImgs = {
  'CACTUS_SMALL': images['CACTUS_SMALL'],
  'CACTUS_LARGE': images['CACTUS_LARGE']
};

Why are these quoted?

this.obstacleImgs = {
  CACTUS_SMALL: images.CACTUS_SMALL,
  CACTUS_LARGE: images.CACTUS_LARGE
};

The only reason to quote them is that they are used by some external code that
is not part of your JSCompiler set of files.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2310: var cloudSpeed = ((this.cloudSpeed /
1000) * deltaTime) * speed;
Too many parens

var cloudSpeed = this.cloudSpeed / 1000 * deltaTime * speed;

[felt, 2014-09-16 15:08:33]

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

[felt, 2014-09-16 15:08:34]

Is the end result of this that the security warnings and neterrors have the same
visual appearance but different code? (Is there a refactoring step after this
one?)

[edwardjung, 2014-09-17 09:13:21]

On 2014/09/16 15:08:34, felt wrote:
> Is the end result of this that the security warnings and neterrors have the
same
> visual appearance but different code? (Is there a refactoring step after this
> one?)

This is correct. Same appearance but different template, using the same
interstitial_v2 styles. There will be some refactoring as mentioned in the
email.

[edwardjung, 2014-09-17 12:12:35]

Gone through and updated. Thanks.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/neterror.js (right):

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/neterror.js:138: setButtonLayout();
On 2014/09/16 14:20:20, arv wrote:
> Wrong indentation. When you line break after function() { you only indent 2,
> like this:
> 
> document.addEventListener('DOMContentLoaded', function() {
>   setButtonLayout();
>   if (document.querySelector('.icon-offline')) {
>     document.body.classList.add('offline');
>     new Runner('.interstitial-wrapper');
>   }
> });

Done.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:5: /**
On 2014/09/16 14:20:20, arv wrote:
> Maybe 'use strict' to catch errors?

Done.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:18: var Runner = function(outerContainerId,
opt_config) {
On 2014/09/16 14:20:21, arv wrote:
> Prefer function declarations where allowed.
> 
> function Runner(outerContainerId, opt_config) {

Done.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:62: // Singleton
On 2014/09/16 14:20:21, arv wrote:
> Do we need this singleton pattern enforced in the code?
> 
> If you really want to enforce it you can use the following pattern:
> 
> function Runner(...) {
>   if (Runner.instance_) {
>     return Runner.instance_;
>   }
>   Runner.instance_ = this;
>   ...
> }
> 
> Since if the function returns an object when you do new Runner, that object is
> used instead of the one that was allocated for new.

Done.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:70: window['Runner'] = Runner;
On 2014/09/16 14:20:20, arv wrote:
> I think you should be able to just do @export on Runner
> 
> /** @export */
> function Runner() {

As I'm no longer using the compiler, I'm removing these exports.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:91: Runner.prototype.updateConfigSetting =
function(setting, value) {
On 2014/09/16 14:20:21, arv wrote:
> How about using an object literal so you do not have to repeat yourself all
> over?
> 
> Runner.prototype = {
>   updateConfigSetting: function(setting, value) {
>     ...
>   },
>   loadImages: function() {
>     ...
>   }
> };
> 
> // Statics are not as pretty :'(
> Runner.config = { ... };
> Runner.DEFAULT_WIDTH = ...;

Done, here and elsewhere

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:137: Runner.DEFAULT_WIDTH = 600;
On 2014/09/16 14:20:21, arv wrote:
> Now that you have an IIFE, there is really no reason to make these properties
of
> Runner. Just do:
> 
> var DEFAULT_WIDTH = 600;
> 
> or
> 
> /** @const */
> var DEFAULT_WIDTH = 600;

Done.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:238: Runner.audioContext = new
AudioContext();
On 2014/09/16 14:20:21, arv wrote:
> Since you are using a singleton, maybe just create this in the constructor?

Refactored.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:264: var buffer =
Runner.decodeBase64ToArrayBuffer(soundSrc);
On 2014/09/16 14:20:21, arv wrote:
> Since you are doing the decoding here, you can put your resources inside a
> <template> element so that the browser does not have to process them twice
(once
> as the page loads and once here).
> 
> To get the elements out of the template you need an extra step here.
> 
> var templateContent = document.getElementById(templateId).content;
> for (...) {
>   var soundSrc = templateContent.getElementById(soundId).src;
> }

Done. Nice.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2095: Runner.HorizonLine = function(canvas,
bgImg) {
On 2014/09/16 14:20:21, arv wrote:
> Why not just HorizontalLine? The IIFE creates the scoping already.

I guess this is from the habit of writing Closure JS keeping everything tightly
name spaced. 

Renamed all the assets classes.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2247: 'CACTUS_SMALL':
images['CACTUS_SMALL'],
On 2014/09/16 14:20:21, arv wrote:
> Wrong indentation.
> 
> this.obstacleImgs = {
>   'CACTUS_SMALL': images['CACTUS_SMALL'],
>   'CACTUS_LARGE': images['CACTUS_LARGE']
> };
> 
> Why are these quoted?
> 
> this.obstacleImgs = {
>   CACTUS_SMALL: images.CACTUS_SMALL,
>   CACTUS_LARGE: images.CACTUS_LARGE
> };
> 
> The only reason to quote them is that they are used by some external code that
> is not part of your JSCompiler set of files.

Fixed.

https://codereview.chromium.org/545973003/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/offline.js:2310: var cloudSpeed = ((this.cloudSpeed /
1000) * deltaTime) * speed;
On 2014/09/16 14:20:20, arv wrote:
> Too many parens
> 
> var cloudSpeed = this.cloudSpeed / 1000 * deltaTime * speed;

Done.

[edwardjung, 2014-09-19 09:02:30]

@arv did you have any more comments?

[arv (Not doing code reviews), 2014-09-19 15:50:42]

Sorry, this fell off my radar.

A few more style nits.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1846: var sourceX =
(DistanceMeter.dimensions.WIDTH) * value;
too many parens (in a lot of places)

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1848: var targetX = (digitPos *
(DistanceMeter.dimensions.DEST_WIDTH));
 var targetX = digitPos * DistanceMeter.dimensions.DEST_WIDTH;

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1945: for (var i = this.digits.length - 1;
i > -1; i--) {
>= 0

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1963: this.draw(i,
parseInt(this.highScore[i]), true);
parseInt(v, 10)

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2093: return (this.xPos +
Cloud.config.WIDTH) > 0;
too many parens

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2125: * @enum {string}
@enum {number}

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2162: return (Math.random() >
this.bumpThreshold) ? this.dimensions.WIDTH : 0;
too many parens

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2162: return (Math.random() >
this.bumpThreshold) ? this.dimensions.WIDTH : 0;
too many parens

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2208: var increment = Math.floor((speed *
(FPS / 1000)) * deltaTime);
too many parens

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2240: 
No need for all of these empty lines.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2298: 
change to one empty line between methods?

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2301: * Update the horizon.
Useless description.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2302: * @param {number} deltaTime Time
elapsed.
remove description here as well... and other useless ones.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2305: *    the obstacles from being updated
/ added. This happens in the
indent +1 space. It should be 4 spaces from @

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2309: function(deltaTime, currentSpeed,
updateObstacles) {
fits on previous line?

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2347: return !obj.remove;
wrong indentation

this.clouds = this.clouds.filter(function(obj) {
  return !obj.remove;
});

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2362: for (var i = 0, j =
this.obstacles.length; i < j; i++) {
for (var i = 0; i < this.obstacles.length; i++) {

The extra trickery has no real perf benefit and makes the code harder to read.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2378: (lastObstacle.xPos +
(lastObstacle.width + lastObstacle.gap)) <
too many parens

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2432: 
remove empty line

[edwardjung, 2014-09-19 17:56:46]

No problem, please have another look. 

I removed the top author comment since this now won't be compiled.

Thanks.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1846: var sourceX =
(DistanceMeter.dimensions.WIDTH) * value;
On 2014/09/19 15:50:41, arv wrote:
> too many parens (in a lot of places)

Not sure what I was thinking here or in other places! 

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1848: var targetX = (digitPos *
(DistanceMeter.dimensions.DEST_WIDTH));
On 2014/09/19 15:50:42, arv wrote:
>  var targetX = digitPos * DistanceMeter.dimensions.DEST_WIDTH;

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1945: for (var i = this.digits.length - 1;
i > -1; i--) {
On 2014/09/19 15:50:42, arv wrote:
> >= 0

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1963: this.draw(i,
parseInt(this.highScore[i]), true);
On 2014/09/19 15:50:42, arv wrote:
> parseInt(v, 10)

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2093: return (this.xPos +
Cloud.config.WIDTH) > 0;
On 2014/09/19 15:50:42, arv wrote:
> too many parens

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2125: * @enum {string}
On 2014/09/19 15:50:42, arv wrote:
> @enum {number}

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2162: return (Math.random() >
this.bumpThreshold) ? this.dimensions.WIDTH : 0;
On 2014/09/19 15:50:42, arv wrote:
> too many parens

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2162: return (Math.random() >
this.bumpThreshold) ? this.dimensions.WIDTH : 0;
On 2014/09/19 15:50:42, arv wrote:
> too many parens

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2208: var increment = Math.floor((speed *
(FPS / 1000)) * deltaTime);
On 2014/09/19 15:50:41, arv wrote:
> too many parens

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2240: 
On 2014/09/19 15:50:42, arv wrote:
> No need for all of these empty lines.

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2298: 
On 2014/09/19 15:50:41, arv wrote:
> change to one empty line between methods?

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2301: * Update the horizon.
On 2014/09/19 15:50:42, arv wrote:
> Useless description.

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2302: * @param {number} deltaTime Time
elapsed.
On 2014/09/19 15:50:41, arv wrote:
> remove description here as well... and other useless ones.

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2305: *    the obstacles from being updated
/ added. This happens in the
On 2014/09/19 15:50:41, arv wrote:
> indent +1 space. It should be 4 spaces from @

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2309: function(deltaTime, currentSpeed,
updateObstacles) {
On 2014/09/19 15:50:42, arv wrote:
> fits on previous line?

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2347: return !obj.remove;
On 2014/09/19 15:50:41, arv wrote:
> wrong indentation
> 
> this.clouds = this.clouds.filter(function(obj) {
>   return !obj.remove;
> });

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2362: for (var i = 0, j =
this.obstacles.length; i < j; i++) {
On 2014/09/19 15:50:42, arv wrote:
> for (var i = 0; i < this.obstacles.length; i++) {
> 
> The extra trickery has no real perf benefit and makes the code harder to read.

Done. Good to know, I'd always thought there was performance gains but with so
few iterations I guess there isn't.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2378: (lastObstacle.xPos +
(lastObstacle.width + lastObstacle.gap)) <
On 2014/09/19 15:50:42, arv wrote:
> too many parens

Done.

https://codereview.chromium.org/545973003/diff/120001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:2432: 
On 2014/09/19 15:50:42, arv wrote:
> remove empty line

Done.

[arv (Not doing code reviews), 2014-09-19 18:24:28]

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:142: ],
-2 spaces

Runner.imageSources = {
  LDPI: [
    {name: 'CACTUS_LARGE', id: '1x-obstacle-large'},
    {name: 'CACTUS_SMALL', id: '1x-obstacle-small'},
    {name: 'CLOUD', id: '1x-cloud'},
    {name: 'HORIZON', id: '1x-horizon'},
    {name: 'RESTART', id: '1x-restart'},
    {name: 'TEXT_SPRITE', id: '1x-text'},
    {name: 'TREX', id: '1x-trex'}
  ],
  HDPI: [
    {name: 'CACTUS_LARGE', id: '2x-obstacle-large'},
    {name: 'CACTUS_SMALL', id: '2x-obstacle-small'},
    {name: 'CLOUD', id: '2x-cloud'},
    {name: 'HORIZON', id: '2x-horizon'},
    {name: 'RESTART', id: '2x-restart'},
    {name: 'TEXT_SPRITE', id: '2x-text'},
    {name: 'TREX', id: '2x-trex'}
  ]
};

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:170: Runner.keycodes = {
No space after { nor before }

Two spaces before //

Runner.keycodes = {
  JUMP: {'38': 1, '32': 1},  // Up, spacebar
  DUCK: {'40': 1},  // Down
  RESTART: {'13': 1}  // Enter
};

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:247: 
remove one empty line

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:269: var mobileSpeed = speed *
((this.dimensions.WIDTH / DEFAULT_WIDTH) *
No need for the parens

      var mobileSpeed = speed * this.dimensions.WIDTH / DEFAULT_WIDTH *
          this.config.MOBILE_SPEED_COEFFICIENT;

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:351: var padding =
parseInt(boxStyles.paddingLeft.substr(0,
Maybe use Number instead? I assume you don't expect non numbers here.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:354: this.dimensions.WIDTH =
this.outerContainerEl.offsetWidth - (padding * 2);
this.dimensions.WIDTH = this.outerContainerEl.offsetWidth - padding * 2;

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:479: var collision = hasObstacles ?
var collision = hasObstacles &&
    Runner.Collision.check(this.horizon.obstacles[0], this.tRex);

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:483: this.distanceRan += this.currentSpeed
* (deltaTime / this.msPerFrame);
this.distanceRan += this.currentSpeed * deltaTime / this.msPerFrame;

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:563:
document.addEventListener(Runner.events.MOUSEDOWN, this, false);
or

document.addEventListener(Runner.events.MOUSEDOWN, this);

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:590: 
remove empty line

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:862: Runner.test = {};
No need for the extra functions.

Runner.test = {
  isMobile: window.
  ...
};

I would just make them constant vars instead.

/** @const */ var IS_MOBILE = navigator.userAgent.indexOf('Mobi') > -1;
...

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:908: Runner.GameOverPanel =
function(canvas, textSprite, restartImg, dimensions) {
Runner.GameOverPanel = function(canvas, textSprite, restartImg, dimensions) {
  this.canvas = canvas;
  this.canvasCtx = canvas.getContext('2d');
  this.canvasDimensions = dimensions;
  this.textSprite = textSprite;
  this.restartImg = restartImg;
  this.draw();
};

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:978: textSourceWidth = textSourceWidth * 2;
Why not *= here too?

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1020: var tRexBox = new
Runner.CollisionBox(

  var tRexBox = new Runner.CollisionBox(
      tRex.xPos + 1,
      tRex.yPos + 1,
      tRex.config.WIDTH - 2,
      tRex.config.HEIGHT - 2);

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1281: var minGap = Math.round((this.width *
speed) +
useless parens

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1282: (this.typeConfig.minGap *
gapCoefficient));
useless parens

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1292: return (this.xPos + this.width) > 0;
useless parens

[edwardjung, 2014-09-21 20:26:53]

Did another pass to correct the comments. 

Thanks.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:142: ],
On 2014/09/19 18:24:28, arv wrote:
> -2 spaces
> 
> Runner.imageSources = {
>   LDPI: [
>     {name: 'CACTUS_LARGE', id: '1x-obstacle-large'},
>     {name: 'CACTUS_SMALL', id: '1x-obstacle-small'},
>     {name: 'CLOUD', id: '1x-cloud'},
>     {name: 'HORIZON', id: '1x-horizon'},
>     {name: 'RESTART', id: '1x-restart'},
>     {name: 'TEXT_SPRITE', id: '1x-text'},
>     {name: 'TREX', id: '1x-trex'}
>   ],
>   HDPI: [
>     {name: 'CACTUS_LARGE', id: '2x-obstacle-large'},
>     {name: 'CACTUS_SMALL', id: '2x-obstacle-small'},
>     {name: 'CLOUD', id: '2x-cloud'},
>     {name: 'HORIZON', id: '2x-horizon'},
>     {name: 'RESTART', id: '2x-restart'},
>     {name: 'TEXT_SPRITE', id: '2x-text'},
>     {name: 'TREX', id: '2x-trex'}
>   ]
> };

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:170: Runner.keycodes = {
On 2014/09/19 18:24:27, arv wrote:
> No space after { nor before }
> 
> Two spaces before //
> 
> Runner.keycodes = {
>   JUMP: {'38': 1, '32': 1},  // Up, spacebar
>   DUCK: {'40': 1},  // Down
>   RESTART: {'13': 1}  // Enter
> };

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:247: 
On 2014/09/19 18:24:27, arv wrote:
> remove one empty line

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:269: var mobileSpeed = speed *
((this.dimensions.WIDTH / DEFAULT_WIDTH) *
On 2014/09/19 18:24:28, arv wrote:
> No need for the parens
> 
>       var mobileSpeed = speed * this.dimensions.WIDTH / DEFAULT_WIDTH *
>           this.config.MOBILE_SPEED_COEFFICIENT;

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:351: var padding =
parseInt(boxStyles.paddingLeft.substr(0,
On 2014/09/19 18:24:28, arv wrote:
> Maybe use Number instead? I assume you don't expect non numbers here.

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:354: this.dimensions.WIDTH =
this.outerContainerEl.offsetWidth - (padding * 2);
On 2014/09/19 18:24:27, arv wrote:
> this.dimensions.WIDTH = this.outerContainerEl.offsetWidth - padding * 2;

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:479: var collision = hasObstacles ?
On 2014/09/19 18:24:27, arv wrote:
> var collision = hasObstacles &&
>     Runner.Collision.check(this.horizon.obstacles[0], this.tRex);

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:483: this.distanceRan += this.currentSpeed
* (deltaTime / this.msPerFrame);
On 2014/09/19 18:24:27, arv wrote:
> this.distanceRan += this.currentSpeed * deltaTime / this.msPerFrame;

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:563:
document.addEventListener(Runner.events.MOUSEDOWN, this, false);
On 2014/09/19 18:24:27, arv wrote:
> or
> 
> document.addEventListener(Runner.events.MOUSEDOWN, this);

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:590: 
On 2014/09/19 18:24:28, arv wrote:
> remove empty line

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:862: Runner.test = {};
On 2014/09/19 18:24:27, arv wrote:
> No need for the extra functions.
> 
> Runner.test = {
>   isMobile: window.
>   ...
> };
> 
> I would just make them constant vars instead.
> 
> /** @const */ var IS_MOBILE = navigator.userAgent.indexOf('Mobi') > -1;
> ...

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:908: Runner.GameOverPanel =
function(canvas, textSprite, restartImg, dimensions) {
On 2014/09/19 18:24:28, arv wrote:
> Runner.GameOverPanel = function(canvas, textSprite, restartImg, dimensions) {
>   this.canvas = canvas;
>   this.canvasCtx = canvas.getContext('2d');
>   this.canvasDimensions = dimensions;
>   this.textSprite = textSprite;
>   this.restartImg = restartImg;
>   this.draw();
> };

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:978: textSourceWidth = textSourceWidth * 2;
On 2014/09/19 18:24:27, arv wrote:
> Why not *= here too?

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1020: var tRexBox = new
Runner.CollisionBox(
On 2014/09/19 18:24:27, arv wrote:
> 
>   var tRexBox = new Runner.CollisionBox(
>       tRex.xPos + 1,
>       tRex.yPos + 1,
>       tRex.config.WIDTH - 2,
>       tRex.config.HEIGHT - 2);

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1281: var minGap = Math.round((this.width *
speed) +
On 2014/09/19 18:24:28, arv wrote:
> useless parens

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1282: (this.typeConfig.minGap *
gapCoefficient));
On 2014/09/19 18:24:28, arv wrote:
> useless parens

Done.

https://codereview.chromium.org/545973003/diff/140001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1292: return (this.xPos + this.width) > 0;
On 2014/09/19 18:24:28, arv wrote:
> useless parens

Done.

[arv (Not doing code reviews), 2014-09-22 15:03:15]

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:878: Runner.GameOverPanel =
function(canvas, textSprite, restartImg, dimensions) {
I still do not understand why these all are properties of Runner? Why not just
use local functions. Less typing, less reading, less execution.

function GameOverPanel(canvas, textSprite, restartImg, dimensions) {

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:967: Runner.Collision = function() {};
No one seems to be calling this.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:978: Runner.Collision.check =
function(obstacle, tRex, opt_canvasCtx) {
Why is this a property of Collision? Maybe change it to a local function and
call it checkForCollision?

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1202: sourceX, 0,
wrong indentation

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1277: new Runner.CollisionBox(0, 7, 5, 27),
wrong indentation

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1290: collisionBoxes: [
wrong indentation

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1392: frames: [44, 0],
wrong indentation

[edwardjung, 2014-09-22 16:54:26]

Made the changes. Apologies that you've been spotting the same style issues that
I've overlooked.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:878: Runner.GameOverPanel =
function(canvas, textSprite, restartImg, dimensions) {
On 2014/09/22 15:03:14, arv wrote:
> I still do not understand why these all are properties of Runner? Why not just
> use local functions. Less typing, less reading, less execution.
> 
> function GameOverPanel(canvas, textSprite, restartImg, dimensions) {

Sorry, my laziness.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:967: Runner.Collision = function() {};
On 2014/09/22 15:03:14, arv wrote:
> No one seems to be calling this.

Removed

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:978: Runner.Collision.check =
function(obstacle, tRex, opt_canvasCtx) {
On 2014/09/22 15:03:14, arv wrote:
> Why is this a property of Collision? Maybe change it to a local function and
> call it checkForCollision?

Done.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1202: sourceX, 0,
On 2014/09/22 15:03:14, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1277: new Runner.CollisionBox(0, 7, 5, 27),
On 2014/09/22 15:03:15, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1290: collisionBoxes: [
On 2014/09/22 15:03:14, arv wrote:
> wrong indentation

Done.

https://codereview.chromium.org/545973003/diff/160001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:1392: frames: [44, 0],
On 2014/09/22 15:03:14, arv wrote:
> wrong indentation

Done.

[arv (Not doing code reviews), 2014-09-22 17:02:44]

LGTM with nits/

I think that you could do a few less "statics" but at this point you've taken
care of most of them.

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:81: var ISHDPI = window.devicePixelRatio >
1;
IS_HDPI would be cleaner

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:84: var ISMOBILE =
window.navigator.userAgent.indexOf('Mobi') > -1;
same here and next one

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:797: Runner.getRandomNum = function(min,
max) {
function getRandomNum(min, max) {

This function is not related to the Runner.

In general "static" methods have very little value in JS. Consider using local
methods more often.

[edwardjung, 2014-09-22 17:50:26]

Thanks for the all the tips, learnt a lot.

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:81: var ISHDPI = window.devicePixelRatio >
1;
On 2014/09/22 17:02:44, arv wrote:
> IS_HDPI would be cleaner

Done.

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:84: var ISMOBILE =
window.navigator.userAgent.indexOf('Mobi') > -1;
On 2014/09/22 17:02:44, arv wrote:
> same here and next one

Done.

https://codereview.chromium.org/545973003/diff/200001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:797: Runner.getRandomNum = function(min,
max) {
On 2014/09/22 17:02:44, arv wrote:
> function getRandomNum(min, max) {
> 
> This function is not related to the Runner.
> 
> In general "static" methods have very little value in JS. Consider using local
> methods more often.

Done.

[arv (Not doing code reviews), 2014-09-22 18:05:33]

Still LGTM

Thanks for the extra cleanup.

https://codereview.chromium.org/545973003/diff/220001/chrome/renderer/resourc...
File chrome/renderer/resources/offline.js (right):

https://codereview.chromium.org/545973003/diff/220001/chrome/renderer/resourc...
chrome/renderer/resources/offline.js:87: var IS_TOUCHENABLED = 'ontouchstart' in
window;
IS_TOUCH_ENABLED maybe?

[edwardjung, 2014-09-23 08:26:59]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2014-09-23 08:27:37]

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

[commit-bot: I haz the power, 2014-09-23 09:44:47]

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

[commit-bot: I haz the power, 2014-09-23 09:44:48]

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

[edwardjung, 2014-09-23 16:37:25]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2014-09-23 16:38:35]

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

[commit-bot: I haz the power, 2014-09-23 18:01:44]

Committed patchset #14 (id:260001) as cadb3af4494f2a14930436d13c5b69d525acb364

[commit-bot: I haz the power, 2014-09-23 18:02:25]

Patchset 14 (id:??) landed as
https://crrev.com/f8cb5584fe0d38a3d3f19d4bb8cb92c753f8d1f3
Cr-Commit-Position: refs/heads/master@{#296209}

[dmazzoni, 2014-09-25 17:41:17]

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

[dmazzoni, 2014-09-25 17:41:19]

Nicely done! Didn't take long for the game to be discovered:

http://thenextweb.com/google/2014/09/25/googles-latest-chrome-build-hidden-ga...