Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2930113002:
Ensure that all CompositorView callbacks are complete when entering VR. (Closed)
DescriptionEnsure that all CompositorView callbacks are complete when entering VR.
BUG=729164
Review-Url: https://codereview.chromium.org/2930113002
Cr-Commit-Position: refs/heads/master@{#478698}
Committed: https://chromium.googlesource.com/chromium/src/+/7c6286a9712160545f38780bbcdb6537f1c9ce70
Patch Set 1 #
Total comments: 4
Patch Set 2 : extract method to clear callbacks #
Total comments: 2
Patch Set 3 : rename method #Messages
Total messages: 19 (10 generated)
amp@chromium.org changed reviewers: + aelias@chromium.org, liberato@chromium.org, mthiesse@chromium.org
liberato@ recommended adding a new 'detachForVR' call to the CompositorView, but I'm not sure we want CompositorView aware of VR and the only interface we have available to make the call (from CompositorViewHolder) is a 'View' interface. So I left it on the setVisibility call for now, but please recommend better solutions.
seems reasonable to me to have it in setVisibility. makes more sense than my idea, now that i think about it. FWIW, i think that getCompositorView() returns a View, but mCompositorView is an actual CompositorView. thanks -fl https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:238: public void surfaceRedrawNeededAsync(SurfaceHolder holder, Runnable drawingFinished) { maybe if !getVisibility() then run the callback? that will fix any issues in case one arrives after setVisibility. i wouldn't expect one to, but i don't know if they can be in-flight etc. or maybe wait until we see if it actually happens. https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:399: List<Runnable> runnables = mDrawingFinishedCallbacks; this should probably be pulled out into its own function.
https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:238: public void surfaceRedrawNeededAsync(SurfaceHolder holder, Runnable drawingFinished) { On 2017/06/09 20:33:42, liberato wrote: > maybe if !getVisibility() then run the callback? that will fix any issues in > case one arrives after setVisibility. i wouldn't expect one to, but i don't > know if they can be in-flight etc. > > or maybe wait until we see if it actually happens. VR hasn't set the visibility at this point yet, so we can't check it here. But yea I'm not sure what to do if a swap comes through after we have already ran the callback... https://codereview.chromium.org/2930113002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:399: List<Runnable> runnables = mDrawingFinishedCallbacks; On 2017/06/09 20:33:42, liberato wrote: > this should probably be pulled out into its own function. Done.
The CQ bit was checked by amp@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/2930113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2930113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398: private void clearDrawFinishedCallbacks() { I'd prefer the name "runDrawFinishedCallbacks()"
+mdjones please OWNERS review https://codereview.chromium.org/2930113002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java (right): https://codereview.chromium.org/2930113002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398: private void clearDrawFinishedCallbacks() { On 2017/06/09 23:05:23, aelias wrote: > I'd prefer the name "runDrawFinishedCallbacks()" Done.
amp@chromium.org changed reviewers: + mdjones@chromium.org
lgtm
The CQ bit was checked by amp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org Link to the patchset: https://codereview.chromium.org/2930113002/#ps40001 (title: "rename method")
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": 40001, "attempt_start_ts": 1497289671101150,
"parent_rev": "ffb2d6a885f945ea11c802dddec4ea66aaa0ea45", "commit_rev":
"7c6286a9712160545f38780bbcdb6537f1c9ce70"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that all CompositorView callbacks are complete when entering VR. BUG=729164 ========== to ========== Ensure that all CompositorView callbacks are complete when entering VR. BUG=729164 Review-Url: https://codereview.chromium.org/2930113002 Cr-Commit-Position: refs/heads/master@{#478698} Committed: https://chromium.googlesource.com/chromium/src/+/7c6286a9712160545f38780bbcdb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7c6286a9712160545f38780bbcdb... |
