Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955153002:
Add arcade mode to chrome://dino (Closed)
|
Created:
3 years, 5 months ago by edwardjung Modified:
3 years, 5 months ago Reviewers:
mmenke CC:
chromium-reviews, elawrence Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd arcade mode to chrome://dino
Specific mode for users who go to chrome://dino just want to play the game.
+ Hides the offline messaging for game focused mode.
+ Game area scales up to the window width, removing the current 600px max width restriction.
Credit to: elawrence@ for the idea
BUG=
Review-Url: https://codereview.chromium.org/2955153002
Cr-Commit-Position: refs/heads/master@{#485576}
Committed: https://chromium.googlesource.com/chromium/src/+/5d7f712b4d54e0127f78f8529046db80b2c1a975
Patch Set 1 #Patch Set 2 : Add arcade mode to chrome://dino #Patch Set 3 : CSS clean up #Patch Set 4 : Correct width calculation leading to incorrect scaling #Patch Set 5 : Fix incorrect scaling on wide screens and overflow #
Total comments: 22
Patch Set 6 : Address review comments #Patch Set 7 : Update positioning calculation #
Messages
Total messages: 20 (10 generated)
edwardjung@chromium.org changed reviewers: + mmenke@chromium.org
Matt PTAL, Video of the change: https://drive.google.com/file/d/0B4AXZ3lfv81gdEFZN2x4T2N4WTg/view
On 2017/06/30 14:12:47, edwardjung wrote: > Matt PTAL, > > Video of the change: > https://drive.google.com/file/d/0B4AXZ3lfv81gdEFZN2x4T2N4WTg/view Could you clarify the description to say what arcade mode is? Looking at the video, I have no idea what's changed. It's a small enough change that I expect to get to doing a review today, but I consider it pretty low priority.
Description was changed from ========== Add arcade mode to chrome://dino BUG= ========== to ========== Add arcade mode to chrome://dino Specific mode for users who go to chrome://dino just want to play the game. + Hides the offline messaging for game focused mode. + Game area scales up to the window width, removing the current 600px max width restriction. BUG= ==========
On 2017/06/30 15:02:09, mmenke wrote: > On 2017/06/30 14:12:47, edwardjung wrote: > > Matt PTAL, > > > > Video of the change: > > https://drive.google.com/file/d/0B4AXZ3lfv81gdEFZN2x4T2N4WTg/view > > Could you clarify the description to say what arcade mode is? Looking at the > video, I have no idea what's changed. It's a small enough change that I expect > to get to doing a review today, but I consider it pretty low priority. Sure, I've just updated it.
https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (left): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:465: @media (min-width: 600px) and (max-width: 736px) and (orientation: landscape) { Why are you removing this? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:511: .arcade-mode { Is this needed? It seems like the next block applies overflow:hidden to this anyways. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:525: overflow: hidden; I assume there's a reason we aren't just using display:none for these? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:529: height: 100vh; vh? Is that a typo, or does it mean something? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:26: this.dimensions = Runner.defaultDimensions; I guess these are now logical dimensions, as opposed to dimensions in screen space? Comment on that somewhere? Or better, rename this to logical_dimensions? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:129: TOP_POSITION_WEIGHT: 10 Should document these, and name them so it's clear they're only for arcade mode. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:852: if (this.isArcadeMode()) { I'd suggest moving this to the callsite, to make it clearer that this is actually conditioned on being in arcade mode. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:861: setContainerScale: function() { setContainerScaleForArcadeMode? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:865: var scale = Math.max(1, Math.min(scaleHeight, scaleWidth)); So does this mean you can see farther if you make the window wide but really short? https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:867: (this.dimensions.HEIGHT * scale)) / Runner.config.TOP_POSITION_WEIGHT); So I guess this is figuring out how much extra height there is to work with, and moving the container of the canvas to have 1/10th of that area above it, and the rest below it? Could you document that? Also, I think this would be clearer if this were: var scaled_canvas_height = this.dimensions.HEIGHT * scale; var translate_y = Math.max(-Runner.config.TOP_MARGIN, scaled_canvas_height * (1 - Runner.config.TOP_POSITION_WEIGHT)); (Note this different in meaning of the weight - it's now a percent). Also, why is the -Runner.config.TOP_MARGIN needed, instead of just 0? I don't suppose we could express that as an arcade-mode CSS property instead? (Would be nice if we could express this all as a CSS property based on arcade mode, but not sure if that's possible or would just be too complicated)
Here's another patch set. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (left): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:465: @media (min-width: 600px) and (max-width: 736px) and (orientation: landscape) { On 2017/07/05 18:51:28, mmenke wrote: > Why are you removing this? Centre aligning the container makes more sense for this subset of widths. It was off balance when the container width is 600px. It also fixes the snap that occurs when resizing the window into these sizes. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/neterror.css (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:511: .arcade-mode { On 2017/07/05 18:51:28, mmenke wrote: > Is this needed? It seems like the next block applies overflow:hidden to this > anyways. Done. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:525: overflow: hidden; On 2017/07/05 18:51:29, mmenke wrote: > I assume there's a reason we aren't just using display:none for these? With display none: you can't transition out the elements as it removes them from the DOM immediately. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/neterror.css:529: height: 100vh; On 2017/07/05 18:51:29, mmenke wrote: > vh? Is that a typo, or does it mean something? Not a typo, a new proportional unit referring to viewport height. Just using 100% doesn't cover the whole screen / window. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:26: this.dimensions = Runner.defaultDimensions; On 2017/07/05 18:51:29, mmenke wrote: > I guess these are now logical dimensions, as opposed to dimensions in screen > space? Comment on that somewhere? Or better, rename this to > logical_dimensions? Done. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:129: TOP_POSITION_WEIGHT: 10 On 2017/07/05 18:51:29, mmenke wrote: > Should document these, and name them so it's clear they're only for arcade mode. Done. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:852: if (this.isArcadeMode()) { On 2017/07/05 18:51:29, mmenke wrote: > I'd suggest moving this to the callsite, to make it clearer that this is > actually conditioned on being in arcade mode. Done. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:861: setContainerScale: function() { On 2017/07/05 18:51:29, mmenke wrote: > setContainerScaleForArcadeMode? Changed to: setArcadeModeContainerScale https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:865: var scale = Math.max(1, Math.min(scaleHeight, scaleWidth)); On 2017/07/05 18:51:29, mmenke wrote: > So does this mean you can see farther if you make the window wide but really > short? No, this just scales up the game container in the window, horizontally and vertically whilst keeping the aspect ratio of the container size. You don't actually get more running space. It never scales down, just stays the same size. https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:867: (this.dimensions.HEIGHT * scale)) / Runner.config.TOP_POSITION_WEIGHT); On 2017/07/05 18:51:29, mmenke wrote: > So I guess this is figuring out how much extra height there is to work with, and > moving the container of the canvas to have 1/10th of that area above it, and the > rest below it? Could you document that? > > Also, I think this would be clearer if this were: > > var scaled_canvas_height = this.dimensions.HEIGHT * scale; > var translate_y = Math.max(-Runner.config.TOP_MARGIN, > scaled_canvas_height * (1 - Runner.config.TOP_POSITION_WEIGHT)); (Note this > different in meaning of the weight - it's now a percent). Did you mean" Math.max(-Runner.config.TOP_MARGIN, scaled_canvas_height * (1 / Runner.config.TOP_POSITION_PERCENT)) // 0.1; This just gives me 10% of the scaled container height. My calculation takes into the account the available window height. So you get more padding at the top as the window height get's bigger. > > Also, why is the -Runner.config.TOP_MARGIN needed, instead of just 0? The container is absolutely positioned at 35px from the top. -translateY(35px) would in effect set it to 0; I've set the top to 0 when in arcade mode. So this top margin isn't required. > suppose we could express that as an arcade-mode CSS property instead? (Would be > nice if we could express this all as a CSS property based on arcade mode, but > not sure if that's possible or would just be too complicated) I agree, but one of the issue with transform, is that it will get overridden by the scale set in the JS here. This is also a much more accurate way to get the positioning right to the available height.
https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:867: (this.dimensions.HEIGHT * scale)) / Runner.config.TOP_POSITION_WEIGHT); On 2017/07/06 19:33:20, edwardjung wrote: > On 2017/07/05 18:51:29, mmenke wrote: > > So I guess this is figuring out how much extra height there is to work with, > and > > moving the container of the canvas to have 1/10th of that area above it, and > the > > rest below it? Could you document that? > > > > Also, I think this would be clearer if this were: > > > > var scaled_canvas_height = this.dimensions.HEIGHT * scale; > > var translate_y = Math.max(-Runner.config.TOP_MARGIN, > > scaled_canvas_height * (1 - Runner.config.TOP_POSITION_WEIGHT)); (Note > this > > different in meaning of the weight - it's now a percent). > > Did you mean" > Math.max(-Runner.config.TOP_MARGIN, > scaled_canvas_height * (1 / Runner.config.TOP_POSITION_PERCENT)) // 0.1; > > This just gives me 10% of the scaled container height. My calculation takes into > the account the available window height. So you get more padding at the top as > the window height get's bigger. No, I did not - dividing by percents generally means it's not a percent. 1 - 0.9 gives 0.1, so is the same thing as the old value of "10", but it's actually a percent, and has a clearer meaning (I was thinking 90% up the page, though if you get rid of the subtraction, could use 0.1 to indicate it's 10% from the top of the page). > > > > Also, why is the -Runner.config.TOP_MARGIN needed, instead of just 0? > > The container is absolutely positioned at 35px from the top. -translateY(35px) > would in effect set it to 0; I've set the top to 0 when in arcade mode. So this > top margin isn't required. > > > suppose we could express that as an arcade-mode CSS property instead? (Would > be > > nice if we could express this all as a CSS property based on arcade mode, but > > not sure if that's possible or would just be too complicated) > > I agree, but one of the issue with transform, is that it will get overridden by > the scale set in the JS here. > This is also a much more accurate way to get the positioning right to the > available height. > >
https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2955153002/diff/80001/components/neterror/res... components/neterror/resources/offline.js:867: (this.dimensions.HEIGHT * scale)) / Runner.config.TOP_POSITION_WEIGHT); > > No, I did not - dividing by percents generally means it's not a percent. 1 - 0.9 > gives 0.1, so is the same thing as the old value of "10", but it's actually a > percent, and has a clearer meaning (I was thinking 90% up the page, though if > you get rid of the subtraction, could use 0.1 to indicate it's 10% from the top > of the page). > Aaah, got it. Switched to to 0.1.
LGTM!
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499767564332800,
"parent_rev": "8909a54471f168be1300c13aaecd2a2985e960ad", "commit_rev":
"5d7f712b4d54e0127f78f8529046db80b2c1a975"}
Message was sent while issue was closed.
Description was changed from ========== Add arcade mode to chrome://dino Specific mode for users who go to chrome://dino just want to play the game. + Hides the offline messaging for game focused mode. + Game area scales up to the window width, removing the current 600px max width restriction. BUG= ========== to ========== Add arcade mode to chrome://dino Specific mode for users who go to chrome://dino just want to play the game. + Hides the offline messaging for game focused mode. + Game area scales up to the window width, removing the current 600px max width restriction. BUG= Review-Url: https://codereview.chromium.org/2955153002 Cr-Commit-Position: refs/heads/master@{#485576} Committed: https://chromium.googlesource.com/chromium/src/+/5d7f712b4d54e0127f78f8529046... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5d7f712b4d54e0127f78f8529046...
Message was sent while issue was closed.
Description was changed from ========== Add arcade mode to chrome://dino Specific mode for users who go to chrome://dino just want to play the game. + Hides the offline messaging for game focused mode. + Game area scales up to the window width, removing the current 600px max width restriction. BUG= Review-Url: https://codereview.chromium.org/2955153002 Cr-Commit-Position: refs/heads/master@{#485576} Committed: https://chromium.googlesource.com/chromium/src/+/5d7f712b4d54e0127f78f8529046... ========== to ========== Add arcade mode to chrome://dino Specific mode for users who go to chrome://dino just want to play the game. + Hides the offline messaging for game focused mode. + Game area scales up to the window width, removing the current 600px max width restriction. Credit to: elawrence@ for the idea BUG= Review-Url: https://codereview.chromium.org/2955153002 Cr-Commit-Position: refs/heads/master@{#485576} Committed: https://chromium.googlesource.com/chromium/src/+/5d7f712b4d54e0127f78f8529046... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
