VRDisplay should not prevent entering the back/forward cache.
Would someone be able to work on this?
Chris could you add a little bit more context?
(In reply to Sergio Villar Senin from comment #2) > Chris could you add a little bit more context? We are working on making the back/forward cache (formerly known as the page cache) 100% successful at caching pages. As a result of this, we no longer allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including VRDisplay) need to suspend correctly. Currently, any page which uses a VRDisplay will not be able to enter the back/forward cache because of this code: // FIXME: This should never prevent entering the back/forward cache. bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const { return true; } This method needs to be removed to allow the page to enter the back/forward cache. Then you'll likely need to override ActiveDOMObject's suspend() and resume() to implement correct suspension. Looking at VRDisplay, it is likely the following needs to be done: 1. Instead of dispatching events synchronously, append tasks to the WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends while in the cache. 2. Call suspend() / resume() on m_scriptedAnimationController to suspend animations while in the cache 3. Maybe call stopPresenting() on suspension? I don't know much about VRDisplay.
(In reply to Chris Dumez from comment #3) > (In reply to Sergio Villar Senin from comment #2) > > Chris could you add a little bit more context? > > We are working on making the back/forward cache (formerly known as the page > cache) 100% successful at caching pages. As a result of this, we no longer > allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including > VRDisplay) need to suspend correctly. > > Currently, any page which uses a VRDisplay will not be able to enter the > back/forward cache because of this code: > // FIXME: This should never prevent entering the back/forward cache. > bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const > { > return true; > } Ah I was not aware of such a thing, it was not there when I added the code. > This method needs to be removed to allow the page to enter the back/forward > cache. Then you'll likely need to override ActiveDOMObject's suspend() and > resume() to implement correct suspension. Looking at VRDisplay, it is likely > the following needs to be done: > 1. Instead of dispatching events synchronously, append tasks to the > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends > while in the cache. > 2. Call suspend() / resume() on m_scriptedAnimationController to suspend > animations while in the cache > 3. Maybe call stopPresenting() on suspension? I don't know much about > VRDisplay. I understand the issue now, I'll come up with something.
(In reply to Sergio Villar Senin from comment #4) > (In reply to Chris Dumez from comment #3) > > (In reply to Sergio Villar Senin from comment #2) > > > Chris could you add a little bit more context? > > > > We are working on making the back/forward cache (formerly known as the page > > cache) 100% successful at caching pages. As a result of this, we no longer > > allow ActiveDOMObjects to prevent caching. ALL ActiveDOMObject (including > > VRDisplay) need to suspend correctly. > > > > Currently, any page which uses a VRDisplay will not be able to enter the > > back/forward cache because of this code: > > // FIXME: This should never prevent entering the back/forward cache. > > bool VRDisplay::shouldPreventEnteringBackForwardCache_DEPRECATED() const > > { > > return true; > > } > > Ah I was not aware of such a thing, it was not there when I added the code. It was there, it was just named differently. It used to be: bool VRDisplay::canSuspendForPageCache() const { return false; } I am pretty sure you added that code and the name is pretty clear about the implications.
> > This method needs to be removed to allow the page to enter the back/forward > > cache. Then you'll likely need to override ActiveDOMObject's suspend() and > > resume() to implement correct suspension. Looking at VRDisplay, it is likely > > the following needs to be done: > > 1. Instead of dispatching events synchronously, append tasks to the > > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends > > while in the cache. > > 2. Call suspend() / resume() on m_scriptedAnimationController to suspend > > animations while in the cache > > 3. Maybe call stopPresenting() on suspension? I don't know much about > > VRDisplay. > > I understand the issue now, I'll come up with something. Thanks for you help.
(In reply to Chris Dumez from comment #3) > 1. Instead of dispatching events synchronously, append tasks to the > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends > while in the cache. Right now the events are only dispatched against the DOMWindow object. Should the dispatch be asynchronous regardless of the target?
(In reply to Zan Dobersek from comment #7) > (In reply to Chris Dumez from comment #3) > > 1. Instead of dispatching events synchronously, append tasks to the > > WindowEventLoop to dispatch them. The WindowEventLoop correctly suspends > > while in the cache. > > Right now the events are only dispatched against the DOMWindow object. > Should the dispatch be asynchronous regardless of the target? Yes, I believe so.
Created attachment 382822 [details] WIP Test pending.
Created attachment 382824 [details] Patch
Comment on attachment 382824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382824&action=review > Source/WebCore/Modules/webvr/VRDisplay.cpp:216 > +void VRDisplay::dispatchVRDisplayEventInWindowEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&& reason) Ryosuke already added ActiveDOMObject::queueTaskToDispatchEvent() for this purpose. > Source/WebCore/Modules/webvr/VRDisplay.cpp:258 > + stopPresenting(); Note that you may want to do this only if the reason for suspension is BackForwardCache.
Created attachment 382827 [details] Dispatch events in DOMWindow
Comment on attachment 382827 [details] Dispatch events in DOMWindow Please see previous review comments.
We can use ActiveDOMObject::queueTaskKeepingObjectAlive(). The event-dispatch version doesn't properly handle the target for this case.
(In reply to Zan Dobersek from comment #14) > We can use ActiveDOMObject::queueTaskKeepingObjectAlive(). The > event-dispatch version doesn't properly handle the target for this case. I did not understand this comment.
Comment on attachment 382827 [details] Dispatch events in DOMWindow View in context: https://bugs.webkit.org/attachment.cgi?id=382827&action=review Inlining the comment into the patch ... > Source/WebCore/Modules/webvr/VRDisplay.cpp:224 > + auto event = VRDisplayEvent::create(eventName, makeRefPtr(this), WTFMove(reason)); > + document()->eventLoop().queueTask(TaskSource::UserInteraction, *document(), [this, event = WTFMove(event)]() mutable { > + if (!document()) > + return; > + if (auto* window = document()->domWindow()) > + window->dispatchEvent(event); > + }); This should use the queueTaskKeepingObjectAlive() function. queueTaskToDispatchEvent() would be an option if the VRDisplay object was the target of these events, but it's not.
Created attachment 382909 [details] Patch
(In reply to Zan Dobersek from comment #16) > Comment on attachment 382827 [details] > Dispatch events in DOMWindow > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382827&action=review > > Inlining the comment into the patch ... > > > Source/WebCore/Modules/webvr/VRDisplay.cpp:224 > > + auto event = VRDisplayEvent::create(eventName, makeRefPtr(this), WTFMove(reason)); > > + document()->eventLoop().queueTask(TaskSource::UserInteraction, *document(), [this, event = WTFMove(event)]() mutable { > > + if (!document()) > > + return; > > + if (auto* window = document()->domWindow()) > > + window->dispatchEvent(event); > > + }); > > This should use the queueTaskKeepingObjectAlive() function. > queueTaskToDispatchEvent() would be an option if the VRDisplay object was > the target of these events, but it's not. Right. I guess in the end it's a matter of taste as it does not change essentially anything wrt eventLoop()->queueTask(), but I replaced it in the new patch version.
Comment on attachment 382909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382909&action=review > Source/WebCore/ChangeLog:16 > + No new tests were added as there is no testing machinery for WebVR so far. It's unclear how :( > Source/WebCore/Modules/webvr/VRDisplay.cpp:216 > +void VRDisplay::dispatchVRDisplayEventInEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&& reason) We likely don't need the WTF:: > Source/WebCore/Modules/webvr/VRDisplay.cpp:261 > + stopPresenting(); As mentioned earlier, you likely want to stop presenting ONLY if ReasonForSuspension is ReasonForSuspension::BackForwardCache. > Source/WebCore/Modules/webvr/VRDisplay.h:119 > + void dispatchVRDisplayEventInEventLoop(const WTF::AtomString& eventName, Optional<VRDisplayEventReason>&&); WTF:: likely unnecessary.
Committed r252178: <https://trac.webkit.org/changeset/252178>
<rdar://problem/56974306>
Thanks for fixing!