Prevents exiting presentation if triggering DON while presenting.

Prevent exiting presentation if triggering DON while presenting.

BUG=718133
Review-Url: https://codereview.chromium.org/2864363005
Cr-Commit-Position: refs/heads/master@{#474409}
Committed: https://chromium.googlesource.com/chromium/src/+/96ac883d24d719a63645a9374c4a555748eab3f8

[tiborg, 2017-05-09 19:20:15]

tiborg@chromium.org changed reviewers:
+ bajones@chromium.org, mthiesse@chromium.org

[tiborg, 2017-05-09 19:20:16]

Hey guys,
please take a look at this bugfix CL.
Thanks,
Tibor

[tiborg, 2017-05-10 15:38:31]

Description was changed from

==========
Prevents exiting presentation if triggering DON while presenting.

BUG=718133
==========

to

==========
Prevent exiting presentation if triggering DON while presenting.

BUG=718133
==========

[mthiesse, 2017-05-10 20:12:38]

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

https://codereview.chromium.org/2864363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: pending_present_request_
= true;
You should still set the flag here, or you'll do the wrong thing if presentation
fails above.

[tiborg, 2017-05-10 20:48:57]

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

https://codereview.chromium.org/2864363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:303: pending_present_request_
= true;
On 2017/05/10 20:12:38, mthiesse wrote:
> You should still set the flag here, or you'll do the wrong thing if
presentation
> fails above.

I'm just reporting if the page requested present in the vrdisplayactivate
handler. If the presentation fails we should be in non-presenting mode anyways.
So, we don't have to stop presenting after the vrdisplayactivate handler is
done. Or am I missing something here?

[mthiesse, 2017-05-10 21:16:06]

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

https://codereview.chromium.org/2864363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:688:
on_handled.Run(pending_present_request_);
I think we can simply change this to:
on_handled.Run(pending_present_request_ || is_presenting_);

And revert the rest of the changes.

[tiborg, 2017-05-16 15:27:01]

tiborg@chromium.org changed reviewers:
+ dcheng@chromium.org

[tiborg, 2017-05-16 15:27:01]

Hi Daniel,
Please take a look at device/vr/vr_service.mojom. I just flipped a switch and
renamed it accordingly.
Thanks,
Tibor

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

https://codereview.chromium.org/2864363005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:688:
on_handled.Run(pending_present_request_);
On 2017/05/10 21:16:06, mthiesse wrote:
> I think we can simply change this to:
> on_handled.Run(pending_present_request_ || is_presenting_);
> 
> And revert the rest of the changes.

Done. And flipped switch as discussed offline.

[mthiesse, 2017-05-16 17:57:54]

lgtm

[dcheng, 2017-05-22 21:09:44]

LGTM with a nit

(Sorry I'm not sure how I missed this CL, but please feel free to ping more
aggressively in the future)

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:118: OnActivate(VRDisplayEventReason reason) => (bool
willNotPresent);
nit: will_not_present

(Or consider using an enum, which will make it more robust)

[tiborg, 2017-05-23 16:00:12]

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:118: OnActivate(VRDisplayEventReason reason) => (bool
willNotPresent);
On 2017/05/22 21:09:44, dcheng wrote:
> nit: will_not_present
> 
> (Or consider using an enum, which will make it more robust)

All other parameter names in this file are camel-cased. Should I still rename to
will_not_present?

[tiborg, 2017-05-24 17:53:32]

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2864363005/diff/40001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:118: OnActivate(VRDisplayEventReason reason) => (bool
willNotPresent);
On 2017/05/23 16:00:12, tiborg wrote:
> On 2017/05/22 21:09:44, dcheng wrote:
> > nit: will_not_present
> > 
> > (Or consider using an enum, which will make it more robust)
> 
> All other parameter names in this file are camel-cased. Should I still rename
to
> will_not_present?

As discussed offline, new parameters should be underscore case. Changed it.

[tiborg, 2017-05-24 17:53:37]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-05-24 17:53:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2864363005/#ps60001
(title: "Changed parameter to underscore case")

[commit-bot: I haz the power, 2017-05-24 17:54:37]

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, 2017-05-24 18:04:29]

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

[commit-bot: I haz the power, 2017-05-24 18:04:30]

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

[bajones, 2017-05-24 18:17:10]

modules/vr/ LGTM

[tiborg, 2017-05-24 18:18:23]

The CQ bit was checked by tiborg@chromium.org

[tiborg, 2017-05-24 18:18:33]

The CQ bit was unchecked by tiborg@chromium.org

[tiborg, 2017-05-24 20:38:52]

The CQ bit was checked by tiborg@chromium.org

[commit-bot: I haz the power, 2017-05-24 20:39:28]

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, 2017-05-24 20:45:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495658332583390,
"parent_rev": "e452a38ed13a4b9c472196221494d228a8be7f30", "commit_rev":
"96ac883d24d719a63645a9374c4a555748eab3f8"}

[commit-bot: I haz the power, 2017-05-24 20:45:29]

Description was changed from

==========
Prevent exiting presentation if triggering DON while presenting.

BUG=718133
==========

to

==========
Prevent exiting presentation if triggering DON while presenting.

BUG=718133

Review-Url: https://codereview.chromium.org/2864363005
Cr-Commit-Position: refs/heads/master@{#474409}
Committed:
https://chromium.googlesource.com/chromium/src/+/96ac883d24d719a63645a9374c4a...
==========

[commit-bot: I haz the power, 2017-05-24 20:45:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/96ac883d24d719a63645a9374c4a...