Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926953002:
Fix WebVR so we don't animation when stopped at a JS breakpoint (Closed)
DescriptionFix WebVR so we don't animate while stopped at a JS breakpoint
While stopped at a JavaScript breakpoint, we shouldn't call into Javascript.
BUG=719101
Review-Url: https://codereview.chromium.org/2926953002
Cr-Commit-Position: refs/heads/master@{#478068}
Committed: https://chromium.googlesource.com/chromium/src/+/dfb76f24c05c31ce433a30fc9b774b25c01c89da
Patch Set 1 #
Total comments: 4
Patch Set 2 : cr feedback - remove flags #
Total comments: 1
Patch Set 3 : update includes #
Total comments: 2
Patch Set 4 : cr feedback #
Messages
Total messages: 26 (13 generated)
billorr@chromium.org changed reviewers: + bajones@chromium.org
PTAL
Description was changed from ========== Fix WebVR so we don't animation when stopped at a JS breakpoint While stopped at a JavaScript breakpoint, we shouldn't call into Javascript. BUG=719101 ========== to ========== Fix WebVR so we don't animate while stopped at a JS breakpoint While stopped at a JavaScript breakpoint, we shouldn't call into Javascript. BUG=719101 ==========
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:219: bool suspended_ = false; You can drop this flag and instead use executionContext->IsSuspended().
mthiesse@chromium.org changed reviewers: + mthiesse@chromium.org
https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:77: if (request_vsync_on_resume_) { No need for this boolean. RequestVSync() is always safe to call, and returns early if you shouldn't schedule the vsync.
The CQ bit was checked by billorr@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/2926953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:77: if (request_vsync_on_resume_) { On 2017/06/08 03:26:30, mthiesse wrote: > No need for this boolean. RequestVSync() is always safe to call, and returns > early if you shouldn't schedule the vsync. Done. https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2926953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:219: bool suspended_ = false; On 2017/06/08 00:55:12, haraken wrote: > > You can drop this flag and instead use executionContext->IsSuspended(). Done. https://codereview.chromium.org/2926953002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2926953002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.h:21: #include "core/dom/SuspendableObject.h" moving this up above...
LGTM on my side. Let's wait for an approval of mthiesse or bajones.
LGTM
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
Thanks, Bill! haraken: This seems like something that should be enforced by Blink somehow. Is there a way we could catch problems like this, at least in debug builds? For example, that some function (task runner? window animations?) is not called unless there is a SuspendableObject in the stack.
On 2017/06/08 05:20:38, ddorwin wrote: > Thanks, Bill! > > haraken: This seems like something that should be enforced by Blink somehow. Is > there a way we could catch problems like this, at least in debug builds? For > example, that some function (task runner? window animations?) is not called > unless there is a SuspendableObject in the stack. Unfortunately no. There is no mechanical way to distinguish tasks / objects that should work during the context suspension from tasks / objects that should not work. We need to annotate the tasks / objects one by one :/ (Actually this is already broken in many ways in the current code base...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2926953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2926953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:141: DVLOG(2) << __FUNCTION__ Oh, sorry, my mistake. The patch that makes this return early hasn't landed yet. Can you please add if (!pending_vrdisplay_raf_) return;
The CQ bit was checked by billorr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, mthiesse@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2926953002/#ps60001 (title: "cr feedback")
https://codereview.chromium.org/2926953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2926953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:141: DVLOG(2) << __FUNCTION__ On 2017/06/08 14:08:09, mthiesse wrote: > Oh, sorry, my mistake. The patch that makes this return early hasn't landed yet. > > Can you please add if (!pending_vrdisplay_raf_) return; Done.
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": 60001, "attempt_start_ts": 1496946149783020,
"parent_rev": "618fd4f3564ecc43f4f7bff660c22a9dd5fc2309", "commit_rev":
"dfb76f24c05c31ce433a30fc9b774b25c01c89da"}
Message was sent while issue was closed.
Description was changed from ========== Fix WebVR so we don't animate while stopped at a JS breakpoint While stopped at a JavaScript breakpoint, we shouldn't call into Javascript. BUG=719101 ========== to ========== Fix WebVR so we don't animate while stopped at a JS breakpoint While stopped at a JavaScript breakpoint, we shouldn't call into Javascript. BUG=719101 Review-Url: https://codereview.chromium.org/2926953002 Cr-Commit-Position: refs/heads/master@{#478068} Committed: https://chromium.googlesource.com/chromium/src/+/dfb76f24c05c31ce433a30fc9b77... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/dfb76f24c05c31ce433a30fc9b77... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
