Adding WebVR temporary presentation handling on mobile

Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose "timestamp" attribute to match spec.

BUG=389343
Committed: https://crrev.com/52355939d8d703281cf450d853dda33e6c3a089f
Cr-Commit-Position: refs/heads/master@{#414165}

[bajones, 2016-08-22 19:42:15]

bajones@chromium.org changed reviewers:
+ haraken@chromium.org, mpearson@chromium.org

[bajones, 2016-08-22 19:42:15]

haraken@: Could you please review changes to core/ and modules/?

mpearson@: Please take a look at histograms.xml?

[bajones, 2016-08-22 19:44:18]

Description was changed from

==========
Adding WebVR presentation handling on mobile

Temporary code path that invokes fullscreen in leiu of proper VR presentation.
Also includes VRDisplayEvent and related code.

BUG=389343
==========

to

==========
Adding WebVR presentation handling on mobile

Temporary code path that invokes fullscreen in leiu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

[bajones, 2016-08-22 20:17:06]

Description was changed from

==========
Adding WebVR presentation handling on mobile

Temporary code path that invokes fullscreen in leiu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

to

==========
Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

[Mark P, 2016-08-22 22:46:40]

histograms.xml lgtm

[haraken, 2016-08-23 00:51:28]

haraken@chromium.org changed reviewers:
+ yhirano@chromium.org

[haraken, 2016-08-23 00:51:29]

+yhirano for ScriptPromise handling.

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:106:
m_frame->domWindow()->dispatchEvent(

Just to confirm:

- You're intending to dispatch the event against the window object instead of
other EventTarget objects, right?

- Do you need to dispatch the event synchronously? If no, you should use
domWindow()->enqueueEvent().

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:170:
m_fullscreenCheckTimer.startRepeating(1.0, BLINK_FROM_HERE);

It's unfortunate that we need to invoke timers every 1 second just to check the
fullscreen state...

Is there any way to avoid invoking many timers by adding some observer?

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRDisplayEvent.h (right):

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRDisplayEvent.h:8: #include
"device/vr/vr_service.mojom-blink.h"

It's unused.

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRPose.idl (right):

https://codereview.chromium.org/2265053004/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRPose.idl:9: readonly attribute double
timestamp;

Shall we make this change in a separate CL?

[bajones, 2016-08-23 21:03:10]

Thanks for the feedback!

On 2016/08/23 00:51:29, haraken wrote:
> Just to confirm:
> 
> - You're intending to dispatch the event against the window object instead of
> other EventTarget objects, right?

Yes. They're specced to fire against the window object.
 
> - Do you need to dispatch the event synchronously? If no, you should use
> domWindow()->enqueueEvent().

No need to be synchronous, so thanks for this suggestion! Updated.
 
> It's unfortunate that we need to invoke timers every 1 second just to check
the
> fullscreen state...
> 
> Is there any way to avoid invoking many timers by adding some observer?

I could, but it would be removed again pretty quickly. The use of the Fullscreen
API here is an incremental, stopgap solution to approximate the effect that
we're trying to achieve. Once the feature is more fleshed out, a few CLs down
the line, it will no longer use Fullscreen here at all. I could still modify
Fullscreen to implement an observer pattern if that's preferred, with the
understanding that it will be a very short lived change.

(I should mention that this feature is only accessible behind a flag. It's
likely that nobody but a few stray developers will ever hit this code path.)
 
> third_party/WebKit/Source/modules/vr/VRDisplayEvent.h:8: #include
> "device/vr/vr_service.mojom-blink.h"
> 
> It's unused.

Removed.

> Shall we make this change in a separate CL?

Done: https://codereview.chromium.org/2268893003/

[haraken, 2016-08-24 01:02:34]

LGTM

https://codereview.chromium.org/2265053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2265053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:105: if (m_frame) {

Add '&& m_frame->localDOMWindow()' just in case.

[yhirano, 2016-08-24 01:39:26]

https://codereview.chromium.org/2265053004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2265053004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:155: if (m_layer.source()) {
What happens otherwise? Will the promise remain pending forever?

[bajones, 2016-08-24 04:06:38]

On 2016/08/24 01:02:34, haraken wrote:
> Add '&& m_frame->localDOMWindow()' just in case.

Done.

On 2016/08/24 01:39:26, yhirano wrote:
> third_party/WebKit/Source/modules/vr/VRDisplay.cpp:155: if (m_layer.source())
{
> What happens otherwise? Will the promise remain pending forever?

Good catch. Now rejecting the promise in that scenario.

[bajones, 2016-08-24 04:06:56]

The CQ bit was checked by bajones@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 04:07:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 04:32:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 04:32:09]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[yhirano, 2016-08-24 04:35:00]

lgtm

[bajones, 2016-08-24 05:23:36]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-24 05:23:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2265053004/#ps60001
(title: "Fixed compile error")

[commit-bot: I haz the power, 2016-08-24 05:23:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 06:19:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 06:19:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bajones, 2016-08-24 07:53:42]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-08-24 07:54:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 07:59:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 07:59:08]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bajones, 2016-08-24 15:49:14]

The CQ bit was checked by bajones@chromium.org

[bajones, 2016-08-24 15:49:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, mpearson@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2265053004/#ps80001
(title: "Rebased to fix UseCounter conflict")

[commit-bot: I haz the power, 2016-08-24 15:49:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 18:10:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:10:34]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bajones, 2016-08-24 18:12:08]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:12:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 22:05:22]

Description was changed from

==========
Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

to

==========
Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

[commit-bot: I haz the power, 2016-08-24 22:05:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-24 22:10:20]

Description was changed from

==========
Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec. 

BUG=389343
==========

to

==========
Adding WebVR temporary presentation handling on mobile

Temporary code path that invokes fullscreen in lieu of proper VR presentation.
Also includes VRDisplayEvent and related code + casing change to VRPose
"timestamp" attribute to match spec.

BUG=389343

Committed: https://crrev.com/52355939d8d703281cf450d853dda33e6c3a089f
Cr-Commit-Position: refs/heads/master@{#414165}
==========

[commit-bot: I haz the power, 2016-08-24 22:10:21]

Patchset 5 (id:??) landed as
https://crrev.com/52355939d8d703281cf450d853dda33e6c3a089f
Cr-Commit-Position: refs/heads/master@{#414165}