Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583503003:
Code cleanup (Closed)
DescriptionCode cleanup
1. Introduce VrSupportLevel to replace multiple booleans
2. remove unused function
BUG=None
Committed: https://crrev.com/4109af2731f8f96abc83eec3007e3b46f4df05a9
Cr-Commit-Position: refs/heads/master@{#440127}
Patch Set 1 #Patch Set 2 : more cleanup #Patch Set 3 : remove unused functions #
Total comments: 4
Patch Set 4 : Nits #
Total comments: 17
Patch Set 5 : reviews #Patch Set 6 : reviews #
Total comments: 2
Patch Set 7 : review #
Messages
Total messages: 37 (22 generated)
The CQ bit was checked by bshe@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 bshe@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...
Description was changed from ========== (Clean up) Better seperation of Cardboard mode and Daydream mode Also includes some minor cleanup BUG=None ========== to ========== Code cleanup 1. Introduce VrSupportLevel to replace multiple booleans 2. remove unused function BUG=None ==========
bshe@chromium.org changed reviewers: + dtrainor@chromium.org, mthiesse@chromium.org
Hi guys. This is a code cleanup. Hopefully it makes the code more readable.
lgtm https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:99: if (mVrClassesBuilder != null) { nit: return early. https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:575: // Only enable ChromeVR(VrShell) on Daydream devices as it currently needs a Daydream nit: space in "ChromeVR(VrShell)"
The CQ bit was checked by bshe@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...
https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:99: if (mVrClassesBuilder != null) { On 2016/12/15 18:18:34, mthiesse wrote: > nit: return early. Done. https://codereview.chromium.org/2583503003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:575: // Only enable ChromeVR(VrShell) on Daydream devices as it currently needs a Daydream On 2016/12/15 18:18:34, mthiesse wrote: > nit: space in "ChromeVR(VrShell)" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:102: mEnterVRIntent = null; I think these default to null right? I would just not set them to null here. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:115: // VrCore compatibility might change at rumtime so initialize the following two Feels like we're half-supporting VrCore compatibility changes (the support level only ever gets set in this constructor). Can we defer that to a later point? Actually I'd be fine pulling out all the logic to a function like updateVrCoreCompatibility(). Then we can just call that once here and call it from these other places when we eventually support it. If you do that then you can add full support for setting/clearing all of the state up front. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:132: // TODO(mthiesse): Update the native WebContents pointer and compositor. This This can happen with things like DOM distiller IIUC. It was also built for pre-render but that feature is gone. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:329: if ((mVrSupportLevel == VR_DAYDREAM) Remove () around this? https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:492: assert mVrClassesBuilder != null; It seems like this class not having a mVrClassesBuilder is a valid state. Should we just return false if this isn't set? That way the constructor can remove that first if block checking mVrClassesBuilder. Should simplify the flow? https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:493: if (mVrCoreVersionChecker != null) { Can this be: if (mVrCoreVersionChecker == null) { mVrCoreVersionChecker = mVrClassesBuilder.createVrCoreVersionChecker(); } return mVrCoreVersionChecker.isVrCoreCompatible();
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:132: // TODO(mthiesse): Update the native WebContents pointer and compositor. This On 2016/12/15 19:23:27, David Trainor wrote: > This can happen with things like DOM distiller IIUC. It was also built for > pre-render but that feature is gone. I'm actually working on implementing this as part of crbug.com/670452
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:102: mEnterVRIntent = null; On 2016/12/15 19:23:27, David Trainor wrote: > I think these default to null right? I would just not set them to null here. Right. But they are final variables and the compiler is unhappy if I don't explicitly set their value. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:115: // VrCore compatibility might change at rumtime so initialize the following two On 2016/12/15 19:23:27, David Trainor wrote: > Feels like we're half-supporting VrCore compatibility changes (the support level > only ever gets set in this constructor). Can we defer that to a later point? > > Actually I'd be fine pulling out all the logic to a function like > updateVrCoreCompatibility(). Then we can just call that once here and call it > from these other places when we eventually support it. If you do that then you > can add full support for setting/clearing all of the state up front. Added a function to update the mVrSupportLevel. We acutally don't know where to call the function yet. Ideally, we want to call it when user updated VrCore while Chrome is running. But we don't get any notification on the event. The best I can think of is onResume(since update VrCore would put Chrome into background) or on a page refresh. But anyway, it is a different issue and the TODO that I left should remind me to fix it (it is a low priority since user can always restart Chrome to get it working). https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:132: // TODO(mthiesse): Update the native WebContents pointer and compositor. This On 2016/12/15 19:27:35, mthiesse wrote: > On 2016/12/15 19:23:27, David Trainor wrote: > > This can happen with things like DOM distiller IIUC. It was also built for > > pre-render but that feature is gone. > > I'm actually working on implementing this as part of crbug.com/670452 I will defer this to Micheal then. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:329: if ((mVrSupportLevel == VR_DAYDREAM) On 2016/12/15 19:23:27, David Trainor wrote: > Remove () around this? Done. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:492: assert mVrClassesBuilder != null; On 2016/12/15 19:23:27, David Trainor wrote: > It seems like this class not having a mVrClassesBuilder is a valid state. > Should we just return false if this isn't set? Not having a mVrClassesBuilder is a valid state. But I want this function to be only called when it exist. So it only reflect the state that if VrCore that installed on the phone is compatible with Chrome or not. I view mVrClassesBuild exist or not as a different state. > > That way the constructor can remove that first if block checking > mVrClassesBuilder. Should simplify the flow? We probably can't since we still need to set the final variables to null. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:493: if (mVrCoreVersionChecker != null) { On 2016/12/15 19:23:27, David Trainor wrote: > Can this be: > > if (mVrCoreVersionChecker == null) { > mVrCoreVersionChecker = mVrClassesBuilder.createVrCoreVersionChecker(); > } > > return mVrCoreVersionChecker.isVrCoreCompatible(); Done.
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:132: // TODO(mthiesse): Update the native WebContents pointer and compositor. This On 2016/12/15 22:01:51, bshe wrote: > On 2016/12/15 19:27:35, mthiesse wrote: > > On 2016/12/15 19:23:27, David Trainor wrote: > > > This can happen with things like DOM distiller IIUC. It was also built for > > > pre-render but that feature is gone. > > > > I'm actually working on implementing this as part of crbug.com/670452 > > I will defer this to Micheal then. For now, can you add a shutdownVR call here?
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:492: assert mVrClassesBuilder != null; On 2016/12/15 22:01:51, bshe wrote: > On 2016/12/15 19:23:27, David Trainor wrote: > > It seems like this class not having a mVrClassesBuilder is a valid state. > > Should we just return false if this isn't set? > Not having a mVrClassesBuilder is a valid state. But I want this function to be > only called > when it exist. So it only reflect the state that if VrCore that installed on the > phone > is compatible with Chrome or not. I view mVrClassesBuild exist or not as a > different state. > > > > That way the constructor can remove that first if block checking > > mVrClassesBuilder. Should simplify the flow? > We probably can't since we still need to set the final variables to null. > > > True, but they won't really be final once the support can chance dynamically (once you figure out the right hook). I'd be fine with making them not final now and fixing it up to look correct since that's what it's going to be in the end anyway.
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:132: // TODO(mthiesse): Update the native WebContents pointer and compositor. This On 2016/12/16 19:28:13, mthiesse wrote: > On 2016/12/15 22:01:51, bshe wrote: > > On 2016/12/15 19:27:35, mthiesse wrote: > > > On 2016/12/15 19:23:27, David Trainor wrote: > > > > This can happen with things like DOM distiller IIUC. It was also built > for > > > > pre-render but that feature is gone. > > > > > > I'm actually working on implementing this as part of crbug.com/670452 > > > > I will defer this to Micheal then. > > For now, can you add a shutdownVR call here? Done. https://codereview.chromium.org/2583503003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:492: assert mVrClassesBuilder != null; On 2016/12/16 19:47:41, David Trainor wrote: > On 2016/12/15 22:01:51, bshe wrote: > > On 2016/12/15 19:23:27, David Trainor wrote: > > > It seems like this class not having a mVrClassesBuilder is a valid state. > > > Should we just return false if this isn't set? > > Not having a mVrClassesBuilder is a valid state. But I want this function to > be > > only called > > when it exist. So it only reflect the state that if VrCore that installed on > the > > phone > > is compatible with Chrome or not. I view mVrClassesBuild exist or not as a > > different state. > > > > > > That way the constructor can remove that first if block checking > > > mVrClassesBuilder. Should simplify the flow? > > We probably can't since we still need to set the final variables to null. > > > > > > > > True, but they won't really be final once the support can chance dynamically > (once you figure out the right hook). I'd be fine with making them not final > now and fixing it up to look correct since that's what it's going to be in the > end anyway. Done.
The CQ bit was checked by bshe@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.
lgtm! https://codereview.chromium.org/2583503003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:109: mVrSupportLevel = VR_NOT_AVAILABLE; Do you want to clear out mTabObserver and mEnableVRIntent here?
Thanks! https://codereview.chromium.org/2583503003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2583503003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:109: mVrSupportLevel = VR_NOT_AVAILABLE; On 2016/12/21 07:57:46, David Trainor (OOO till Jan 3) wrote: > Do you want to clear out mTabObserver and mEnableVRIntent here? Done.
The CQ bit was checked by bshe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2583503003/#ps140001 (title: "review")
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": 140001, "attempt_start_ts": 1482336154051910,
"parent_rev": "0e17e20e3f7b996eac0f76bf80e769c64b0afbe1", "commit_rev":
"aa69c908c9969674314e57fbc75efa69d2e0f89a"}
Message was sent while issue was closed.
Description was changed from ========== Code cleanup 1. Introduce VrSupportLevel to replace multiple booleans 2. remove unused function BUG=None ========== to ========== Code cleanup 1. Introduce VrSupportLevel to replace multiple booleans 2. remove unused function BUG=None Review-Url: https://codereview.chromium.org/2583503003 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Code cleanup 1. Introduce VrSupportLevel to replace multiple booleans 2. remove unused function BUG=None Review-Url: https://codereview.chromium.org/2583503003 ========== to ========== Code cleanup 1. Introduce VrSupportLevel to replace multiple booleans 2. remove unused function BUG=None Committed: https://crrev.com/4109af2731f8f96abc83eec3007e3b46f4df05a9 Cr-Commit-Position: refs/heads/master@{#440127} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4109af2731f8f96abc83eec3007e3b46f4df05a9 Cr-Commit-Position: refs/heads/master@{#440127} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
