Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534543002:
VRDisplay inherits from EventTarget (Closed)
DescriptionVRDisplay inherits from EventTarget
Spec is here:
https://w3c.github.io/webvr/#interface-vrdisplay
BUG=
Committed: https://crrev.com/b4d018d41f2bc8d8528d4ca2408116585a6a22f0
Cr-Commit-Position: refs/heads/master@{#436957}
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase, and replace ActiveDOMObject with ContextLifecycleObserver #
Total comments: 2
Patch Set 3 : Fix destroy #
Total comments: 1
Messages
Total messages: 35 (23 generated)
Description was changed from ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= ========== to ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= ==========
xing.xu@intel.com changed reviewers: + bajones@chromium.org, bshe@chromium.org, mlamouri@chromium.org
The CQ bit was checked by xing.xu@intel.com
The CQ bit was unchecked by xing.xu@intel.com
The CQ bit was checked by xing.xu@intel.com 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.
xing.xu@intel.com changed reviewers: + shaobo.yan@intel.com
lgtm Sorry for late reply.
https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:673: return ContextLifecycleObserver::getExecutionContext(); Seems more common in objects that derive from ActiveDOMObject to use ActiveDOMObject::getExecutionContext(); See: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation... https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:689: // TODO(xing.xu): Implement destory. Such as forceExitPresent(). Minor typo here ("destory" -> "destroy") but rather than fix that I think we should simply call forceExitPresent(). here as suggested in the comment.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:689: // TODO(xing.xu): Implement destory. Such as forceExitPresent(). If you're not sure what to do in contextDestroyed, I'd prefer deferring adding ActiveDOMObject to a later CL. https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.h (right): https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.h:91: void resume() override; Do you really want to implement suspend/resume? Otherwise, you can use ContextLifecycleObserver.
https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2534543002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:689: // TODO(xing.xu): Implement destory. Such as forceExitPresent(). On 2016/11/29 at 01:43:28, haraken wrote: > If you're not sure what to do in contextDestroyed, I'd prefer deferring adding ActiveDOMObject to a later CL. +1, it sounds that you dan't have a need for the object to be an ActiveDOMOObject yet.
The CQ bit was checked by xing.xu@intel.com 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...
PTAL, Updated, thanks.
LGTM https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:757: ContextLifecycleObserver::clearContext(); This won't be needed. ContextLifecycleObserver clears the context after calling all contextDestroyed(). https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:763: return hasEventListeners(); Would you change this to: return getExecutionContext() && hasEventListeners(); ? Currently the wrapper tracing doesn't have logic to ignore pending activities on a detached context (we're fixing it now). So the caller side needs to take care of it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/07 09:01:23, haraken wrote: > LGTM > > https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): > > https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:757: > ContextLifecycleObserver::clearContext(); > > This won't be needed. ContextLifecycleObserver clears the context after calling > all contextDestroyed(). > > https://codereview.chromium.org/2534543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/vr/VRDisplay.cpp:763: return > hasEventListeners(); > > Would you change this to: > > return getExecutionContext() && hasEventListeners(); > > ? > > Currently the wrapper tracing doesn't have logic to ignore pending activities on > a detached context (we're fixing it now). So the caller side needs to take care > of it. Thanks haraken@, it is do destroyed by LifecycleNotifier. Updated, ptal.
The CQ bit was checked by xing.xu@intel.com 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...
LGTM
The CQ bit was unchecked by xing.xu@intel.com
The CQ bit was unchecked by xing.xu@intel.com
The CQ bit was checked by xing.xu@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from shaobo.yan@intel.com Link to the patchset: https://codereview.chromium.org/2534543002/#ps40001 (title: "Fix destroy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2534543002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right): https://codereview.chromium.org/2534543002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRDisplay.cpp:212: Document* doc = this->document(); Here and below, it looks like you could write `document()` instead of `this->document()`.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481121530564800,
"parent_rev": "0de06b646eae831d142553a91db1cafeaa821b41", "commit_rev":
"d94058ed297756e7a68dce58e5dc43c7308b482a"}
Message was sent while issue was closed.
Description was changed from ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= ========== to ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= ========== to ========== VRDisplay inherits from EventTarget Spec is here: https://w3c.github.io/webvr/#interface-vrdisplay BUG= Committed: https://crrev.com/b4d018d41f2bc8d8528d4ca2408116585a6a22f0 Cr-Commit-Position: refs/heads/master@{#436957} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b4d018d41f2bc8d8528d4ca2408116585a6a22f0 Cr-Commit-Position: refs/heads/master@{#436957} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
