Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2144933002:
Fix the T-Rex easter egg on iOS. (Closed)
|
Created:
4 years, 5 months ago by kkhorimoto Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the T-Rex easter egg on iOS.
In this CL:
- Always supply a user agent to WKWebViews in web//'s creation methods.
This allows the IS_IOS variable in offline.js to be correctly set to
true.
- Delay initializing the game runner until after images have been
loaded.
- Check for existence of errorPageController when activating the game.
BUG=none
Committed: https://crrev.com/068ace2593da95f2f47b20432e02e8d1e3706807
Cr-Commit-Position: refs/heads/master@{#406949}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Eugene's comments #Patch Set 3 : rebase #
Total comments: 7
Patch Set 4 : Edward's comments #
Total comments: 2
Patch Set 5 : fixed indent #
Messages
Total messages: 23 (10 generated)
kkhorimoto@chromium.org changed reviewers: + eugenebut@google.com, mmenke@chromium.org
The CQ bit was checked by kkhorimoto@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...
mmenke@chromium.org changed reviewers: + edwardjung@chromium.org
[+edwardjung] Going to defer to edwardjung on this, as I'm unfamiliar with the code for the game. Once he signs off, I'll rubberstamp.
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
Thanks a lot for fixing this. Have no idea how could we ship the product with broken dinosaur game :( ios/ lgtm https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... components/neterror/resources/offline.js:295: if(!Runner.imageSprite.complete) { Space after |if| https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... components/neterror/resources/offline.js:672: if (window.errorPageController) { Optional NIT: Maybe s/window.errorPageController/errorPageController for consistency with the next line. https://codereview.chromium.org/2144933002/diff/1/ios/web/web_state/web_view_... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2144933002/diff/1/ios/web/web_state/web_view_... ios/web/web_state/web_view_internal_creation_util.mm:56: return CreateWKWebView(frame, configuration, browser_state, NO); Optional NIT: /* use_desktop_user_agent */ or create local variable for NO. Otherwise it's not obvious from the call place what does this BOOL mean.
https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... components/neterror/resources/offline.js:295: if(!Runner.imageSprite.complete) { On 2016/07/13 05:28:57, Eugene But wrote: > Space after |if| Done. https://codereview.chromium.org/2144933002/diff/1/components/neterror/resourc... components/neterror/resources/offline.js:672: if (window.errorPageController) { On 2016/07/13 05:28:57, Eugene But wrote: > Optional NIT: Maybe s/window.errorPageController/errorPageController for > consistency with the next line. Done. https://codereview.chromium.org/2144933002/diff/1/ios/web/web_state/web_view_... File ios/web/web_state/web_view_internal_creation_util.mm (right): https://codereview.chromium.org/2144933002/diff/1/ios/web/web_state/web_view_... ios/web/web_state/web_view_internal_creation_util.mm:56: return CreateWKWebView(frame, configuration, browser_state, NO); On 2016/07/13 05:28:57, Eugene But wrote: > Optional NIT: /* use_desktop_user_agent */ or create local variable for NO. > Otherwise it's not obvious from the call place what does this BOOL mean. Done.
Sorry for the delay, was OOO. Thanks for putting together this fix! https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:295: if (!Runner.imageSprite.complete) { Do a positive check for the 'complete' property. if (Runner.imageSprite.complete) { … } else { … } https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:301: } I would use the addEventListener style of attaching a listener that is used everywhere else. Cleaner and you can bind 'this' to the function at the same time: Runner.imageSprite.addEventListener(Runner.events.LOAD, this.init.bind(this)); https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:674: } Will there ever be an instance where errorPageController won't exist at this point? We always want to track the easter egg activations. Regardless, the check should be: if (window.errorPageController) { … } As otherwise this would throw a reference error if errorPageController doesn't exist.
https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:295: if (!Runner.imageSprite.complete) { On 2016/07/18 10:37:11, edwardjung wrote: > Do a positive check for the 'complete' property. > > if (Runner.imageSprite.complete) { > … > } else { > … > } Done. https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:301: } On 2016/07/18 10:37:11, edwardjung wrote: > I would use the addEventListener style of attaching a listener that is used > everywhere else. Cleaner and you can bind 'this' to the function at the same > time: > > Runner.imageSprite.addEventListener(Runner.events.LOAD, this.init.bind(this)); Done. https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:674: } On 2016/07/18 10:37:11, edwardjung wrote: > Will there ever be an instance where errorPageController won't exist at this > point? We always want to track the easter egg activations. > > Regardless, the check should be: > > if (window.errorPageController) { > … > } > > As otherwise this would throw a reference error if errorPageController doesn't > exist. errorPageController is consistently nil on iOS. Somehow my previous check of "if (errorPageController)" wasn't throwing an exception when I uploaded this first patch, but now it does, so the "window" portion is added. I can't seem to find where this variable is set though; from your comment it seems like it is probably a bug that it is nil here.
LGTM errorPageController being null on iOS is probably a bug. I won't let it block this CL but wonder if you could follow up on this? Thanks. https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/40001/components/neterror/res... components/neterror/resources/offline.js:674: } > errorPageController is consistently nil on iOS. Somehow my previous check of > "if (errorPageController)" wasn't throwing an exception when I uploaded this > first patch, but now it does, so the "window" portion is added. I can't seem to > find where this variable is set though; from your comment it seems like it is > probably a bug that it is nil here. errorPageController is exposed from NetErrorPageController::Install() as a global so it should exist. https://cs.chromium.org/chromium/src/chrome/renderer/net/net_error_page_contr... This used to work without this check, so I'm wondering if something changed in the transition to WKWebView?
Sorry one nit https://codereview.chromium.org/2144933002/diff/60001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/60001/components/neterror/res... components/neterror/resources/offline.js:300: this.init.bind(this)); Nit: JS style is to indent by four spaces and not to the bracket in C++.
LGTM, rubberstamp, deferring to edwardjung.
https://codereview.chromium.org/2144933002/diff/60001/components/neterror/res... File components/neterror/resources/offline.js (right): https://codereview.chromium.org/2144933002/diff/60001/components/neterror/res... components/neterror/resources/offline.js:300: this.init.bind(this)); On 2016/07/20 21:39:17, edwardjung wrote: > Nit: JS style is to indent by four spaces and not to the bracket in C++. Done.
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, edwardjung@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2144933002/#ps80001 (title: "fixed indent")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix the T-Rex easter egg on iOS. In this CL: - Always supply a user agent to WKWebViews in web//'s creation methods. This allows the IS_IOS variable in offline.js to be correctly set to true. - Delay initializing the game runner until after images have been loaded. - Check for existence of errorPageController when activating the game. BUG=none ========== to ========== Fix the T-Rex easter egg on iOS. In this CL: - Always supply a user agent to WKWebViews in web//'s creation methods. This allows the IS_IOS variable in offline.js to be correctly set to true. - Delay initializing the game runner until after images have been loaded. - Check for existence of errorPageController when activating the game. BUG=none Committed: https://crrev.com/068ace2593da95f2f47b20432e02e8d1e3706807 Cr-Commit-Position: refs/heads/master@{#406949} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/068ace2593da95f2f47b20432e02e8d1e3706807 Cr-Commit-Position: refs/heads/master@{#406949} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
