Offline easter egg - Fix incorrect score when restarting from a paused game

Offline easter egg - Fix incorrect score when restarting from a paused game

When paused (window resize, orientation change), the canvas frame was being
updated with an invalid time causing the score to be incremented by the duration
that the game was paused for. Added a check to ensure the game was not paused
before handling the key press / touches.

BUG=687887
TBR=mmenke@chromium.org

Review-Url: https://codereview.chromium.org/2747773002
Cr-Commit-Position: refs/heads/master@{#459138}
Committed: https://chromium.googlesource.com/chromium/src/+/cc185fb06a698256572d717750923a2588928519

[edwardjung, 2017-03-13 14:51:07]

Description was changed from

==========
Fix incorrect score when restarting from game pause

BUG=687887
==========

to

==========
Offline easter egg - Fix incorrect score when restarting from a paused game
(resize, orientation change)

BUG=687887
==========

[edwardjung, 2017-03-13 14:57:33]

Description was changed from

==========
Offline easter egg - Fix incorrect score when restarting from a paused game
(resize, orientation change)

BUG=687887
==========

to

==========
Offline easter egg - Fix incorrect score when restarting from a paused game
(resize, orientation change)

The canvas frame was being updated with an invalid time causing the score to be
incremented by the duration that the game was paused for. Added a check to
ensure the game was not paused before handling the key press / touches.

BUG=687887
==========

[edwardjung, 2017-03-13 15:03:23]

Description was changed from

==========
Offline easter egg - Fix incorrect score when restarting from a paused game
(resize, orientation change)

The canvas frame was being updated with an invalid time causing the score to be
incremented by the duration that the game was paused for. Added a check to
ensure the game was not paused before handling the key press / touches.

BUG=687887
==========

to

==========
Offline easter egg - Fix incorrect score when restarting from a paused game

When paused (window resize, orientation change), the canvas frame was being
updated with an invalid time causing the score to be incremented by the duration
that the game was paused for. Added a check to ensure the game was not paused
before handling the key press / touches.

BUG=687887
TBR=mmenke@chromium.org
==========

[edwardjung, 2017-03-13 15:03:40]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 15:04:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 15:29:02]

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

[commit-bot: I haz the power, 2017-03-13 15:29:03]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2017-03-13 16:03:22]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2017-03-13 16:03:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 16:03:53]

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

[commit-bot: I haz the power, 2017-03-13 16:03:54]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[edwardjung, 2017-03-13 16:09:04]

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

[edwardjung, 2017-03-13 16:09:04]

Hi, Julia PTAL. 

I'm in the owners file but need a LGTM from a committer. I usually ask mmenke
who is more familiar with the JS code, but I realise he's still catching up from
being OOO.

Thanks, 

Edward

[edwardjung, 2017-03-20 10:48:12]

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

[edwardjung, 2017-03-20 10:48:13]

+mmenke whoever has bandwidth to look at this small fix.

[mmenke, 2017-03-20 15:11:28]

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
components/neterror/resources/offline.js:661: e.preventDefault();
Is this the right thing to do if e.target == this.detailsButton?  There's an
exception to do nothing if that's the case below, seems weird that there's no
similar exception here.  Does this only prevent scrolling?  Seems fairly broad
for that.

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
components/neterror/resources/offline.js:664: if (!this.paused) {
Exactly what line are you trying to skip here?  I don't see how the score would
be updated by any of this code.

[edwardjung, 2017-03-21 18:51:22]

Did some refactoring which hopefully makes it clearer. I still need to test this
on an iOS device when I'm back in the office but there shouldn't be any issues
there.

Thanks.

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
components/neterror/resources/offline.js:661: e.preventDefault();
On 2017/03/20 15:11:28, mmenke wrote:
> Is this the right thing to do if e.target == this.detailsButton?  There's an
> exception to do nothing if that's the case below, seems weird that there's no
> similar exception here.  Does this only prevent scrolling?  Seems fairly broad
> for that.

Yes, as during game play you don't want to be accidentally hit the details
button.

However the recent layout change means there is no details button shown on the
offline page any more. So it doesn't apply anymore.

https://codereview.chromium.org/2747773002/diff/1/components/neterror/resourc...
components/neterror/resources/offline.js:664: if (!this.paused) {
On 2017/03/20 15:11:28, mmenke wrote:
> Exactly what line are you trying to skip here?  I don't see how the score
would
> be updated by any of this code.

this.update();

As the delta time is calculated when the the game is paused, the score is
updated incorrectly. It's only when the key as actually pressed (key up) the
game time is reset.

Line 727:
  } else if (this.paused && isjumpKey) {
  …
  this.play();
  }

I've refactored this method so it's clearer.

[mmenke, 2017-03-21 18:59:59]

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:663: if (!this.crashed && !this.paused)
{
Removing the e.target != this.detailsButton doesn't hurt accessibility, right? 
i.e., a user selects the details button (Or reload button) with tab and presses
space to expand it?  (Disclaimer:  I know nothing about accessibility)

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:666: // Starting the game for the first
time.
One more question:  Should this also call e.preventDefault()?  Down does, to
avoid scrolling.  Should up do it, too?

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:691: e.currentTarget ==
this.containerEl) {
nit:  Fix indentation (Align after (, or use 4-space indent)

[edwardjung, 2017-03-23 10:31:47]

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
File components/neterror/resources/offline.js (right):

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:663: if (!this.crashed && !this.paused)
{
On 2017/03/21 18:59:58, mmenke wrote:
> Removing the e.target != this.detailsButton doesn't hurt accessibility, right?

> i.e., a user selects the details button (Or reload button) with tab and
presses
> space to expand it?  (Disclaimer:  I know nothing about accessibility)

In the past yes, but we no longer have the details button on the internet
disconnected error. Keeping it in doesn't hurt in case the details button is
reintroduced in future, but I think it's more likely we have new switches for
turning wifi / data settings, which I'll handle down the line.

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:666: // Starting the game for the first
time.
On 2017/03/21 18:59:58, mmenke wrote:
> One more question:  Should this also call e.preventDefault()?  Down does, to
> avoid scrolling.  Should up do it, too?

Good point. The vast majority of the time the page isn't scrollable, but it is
possible to scroll down on small screens sizes. Added.

https://codereview.chromium.org/2747773002/diff/60001/components/neterror/res...
components/neterror/resources/offline.js:691: e.currentTarget ==
this.containerEl) {
On 2017/03/21 18:59:59, mmenke wrote:
> nit:  Fix indentation (Align after (, or use 4-space indent)

Done.

[mmenke, 2017-03-23 15:20:28]

LGTM!

[edwardjung, 2017-03-23 17:12:25]

The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 17:13:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 17:30:45]

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

[commit-bot: I haz the power, 2017-03-23 17:30:46]

Dry run: This issue passed the CQ dry run.

[edwardjung, 2017-03-23 17:43:12]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2017-03-23 17:43:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 17:58:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490290992360280,
"parent_rev": "a4e99e1885a8107a8afc13978ba785775cb11b61", "commit_rev":
"cc185fb06a698256572d717750923a2588928519"}

[commit-bot: I haz the power, 2017-03-23 17:59:27]

Description was changed from

==========
Offline easter egg - Fix incorrect score when restarting from a paused game

When paused (window resize, orientation change), the canvas frame was being
updated with an invalid time causing the score to be incremented by the duration
that the game was paused for. Added a check to ensure the game was not paused
before handling the key press / touches.

BUG=687887
TBR=mmenke@chromium.org
==========

to

==========
Offline easter egg - Fix incorrect score when restarting from a paused game

When paused (window resize, orientation change), the canvas frame was being
updated with an invalid time causing the score to be incremented by the duration
that the game was paused for. Added a check to ensure the game was not paused
before handling the key press / touches.

BUG=687887
TBR=mmenke@chromium.org

Review-Url: https://codereview.chromium.org/2747773002
Cr-Commit-Position: refs/heads/master@{#459138}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc185fb06a698256572d71775092...
==========

[commit-bot: I haz the power, 2017-03-23 17:59:29]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cc185fb06a698256572d71775092...