Provide WebVR pose data only to the focused frame.

Provide WebVR pose data only to the focused frame.

Testing is tracked in issue 679827. For now this is manually tested.

BUG=683322, 686890
Review-Url: https://codereview.chromium.org/2668003003
Cr-Commit-Position: refs/heads/master@{#448315}
Committed: https://chromium.googlesource.com/chromium/src/+/527310a8c856c31a016c8f9c9ddb90a5290aef5b

[mthiesse, 2017-01-31 18:21:03]

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

[mthiesse, 2017-01-31 18:21:04]

jbroman@chromium.org: Please review changes in Source/core/
bajones@chromium.org: Please review changes in Source/modules/

[jbroman, 2017-02-01 06:14:55]

Missing core/page/FocusChangedObserver.h?

[mthiesse, 2017-02-01 15:17:09]

On 2017/02/01 06:14:55, jbroman wrote:
> Missing core/page/FocusChangedObserver.h?

Whoops, uploaded.

[mthiesse, 2017-02-01 15:17:10]

On 2017/02/01 06:14:55, jbroman wrote:
> Missing core/page/FocusChangedObserver.h?

Whoops, uploaded.

[mthiesse, 2017-02-01 15:18:22]

Patchset #6 (id:100001) has been deleted

[bajones, 2017-02-02 00:28:47]

This LGTM as a quick fix, but similar to
https://codereview.chromium.org/2665293002/ we ideally want to track the focused
state of the page on the browser side and prevent the poses from being sent over
IPC at all. This is not only a matter of efficiency (don't send data that won't
be used) but also of security. Chrome generally assumes the renderer process
isn't trusted, which in this case manifests as "could be compromised to force
m_focused to true and spy on poses meant for other pages." That's probably not
the worst thing in the world, but if we feel it's worth suppressing the poses at
all it's worth doing in a secure way.

That being said, I don't know what focus-tracking capabilities exist in the
browser process, and I'm not sure how to relate the ones that are available to
the right VRDisplayImpls, so some research is required here.

[mthiesse, 2017-02-02 15:30:45]

On 2017/02/02 00:28:47, bajones wrote:
> This LGTM as a quick fix, but similar to
> https://codereview.chromium.org/2665293002/ we ideally want to track the
focused
> state of the page on the browser side and prevent the poses from being sent
over
> IPC at all. This is not only a matter of efficiency (don't send data that
won't
> be used) but also of security. Chrome generally assumes the renderer process
> isn't trusted, which in this case manifests as "could be compromised to force
> m_focused to true and spy on poses meant for other pages." That's probably not
> the worst thing in the world, but if we feel it's worth suppressing the poses
at
> all it's worth doing in a secure way.
> 
> That being said, I don't know what focus-tracking capabilities exist in the
> browser process, and I'm not sure how to relate the ones that are available to
> the right VRDisplayImpls, so some research is required here.

jbroman would probably know. Does each frame have its own RenderWidget? If so,
then all we'd need is a way to know from the browser which RenderWidget each
VRDisplay lives in.

[jbroman, 2017-02-02 16:07:20]

On 2017/02/02 at 15:30:45, mthiesse wrote:
> On 2017/02/02 00:28:47, bajones wrote:
> > This LGTM as a quick fix, but similar to
> > https://codereview.chromium.org/2665293002/ we ideally want to track the
focused
> > state of the page on the browser side and prevent the poses from being sent
over
> > IPC at all. This is not only a matter of efficiency (don't send data that
won't
> > be used) but also of security. Chrome generally assumes the renderer process
> > isn't trusted, which in this case manifests as "could be compromised to
force
> > m_focused to true and spy on poses meant for other pages." That's probably
not
> > the worst thing in the world, but if we feel it's worth suppressing the
poses at
> > all it's worth doing in a secure way.
> > 
> > That being said, I don't know what focus-tracking capabilities exist in the
> > browser process, and I'm not sure how to relate the ones that are available
to
> > the right VRDisplayImpls, so some research is required here.
> 
> jbroman would probably know. Does each frame have its own RenderWidget? If so,
then all we'd need is a way to know from the browser which RenderWidget each
VRDisplay lives in.

cc dcheng, who would know more detail about focus in an OOPIF world

My understanding is that each frame has a RenderFrame (+RenderFrameHost). Each
of those is associated with a RenderWidgetHostView (+RenderWidgetHost) for the
local frame root (i.e., the last frame in an ancestor walk before it's hosted in
another process, due to OOPIF).

I think (but am not very confident) that the browser process only manages focus
up to the RenderWidget level, and that amongst same-process frames the renderer
is responsible for managing it on the Blink side.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusChangedObserver.h (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusChangedObserver.h:1: /*
Wrong copyright header. The correct header for new files is here:
https://www.chromium.org/blink/coding-style#TOC-License (with the current year,
of course)

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:875:
notifyFocusChangedObservers();
Won't this possibly notify twice, since setFocusedFrame also does so?

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1412: for (auto& it :
m_focusChangedObservers) {
nit: const auto&

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1414: }
super-nit: don't need the braces

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:128: void
NavigatorVR::focusedFrameChanged() {
nit: This condition might be clearer factored out to a static function?

static bool isFrameDocumentFocused(LocalFrame* frame) {
  if (!frame) return false;
  Page* page = frame->page();
  Document* document = frame->document();
  return page && document &&
page->focusController().isDocumentFocused(document);
}

void NavigatorVR::focusedFrameChanged() {
  const bool focused = supplementable() &&
isFrameDocumentFocused(supplementable()->frame());
  if (m_focused != focused && m_controller) {
    m_controller->setListeningForActivate(focused && m_listeningForActivate);
    m_controller->focusChanged(focused);
  }
  m_focused = focused;
}

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:132:
supplementable()->frame()->page()->focusController().isDocumentFocused(
Do you really want isDocumentFocused? This will return true if any descendant
frame has focus as well. This may or may not be what you want, but you probably
want to match whatever you'll implement on the browser side later on.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:78: for (size_t i = 0; i <
m_displays.size(); ++i)
super-nit: you could use a range-based loop here too:

for (const auto& display : m_displays)
  display->focusChanged(focused);

[mthiesse, 2017-02-02 17:52:18]

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusChangedObserver.h (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusChangedObserver.h:1: /*
On 2017/02/02 16:07:20, jbroman wrote:
> Wrong copyright header. The correct header for new files is here:
> https://www.chromium.org/blink/coding-style#TOC-License (with the current
year,
> of course)

Done.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:875:
notifyFocusChangedObservers();
On 2017/02/02 16:07:20, jbroman wrote:
> Won't this possibly notify twice, since setFocusedFrame also does so?

Yes, intentionally. We need to call it in setFocusedFrame, because that's called
from a bunch of places, but setFocused also changes whether or not the "Focused
Frame" is actually focused, even if it doesn't change the "Focused Frame". I
didn't want to check from setFocused whether or not setFocusedFrame would be
called, whether it would return early, etc. to avoid notifying twice.

I could separate this into two callbacks, observer->focusedFrameChanged and
observer->focusChanged if that helps?

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1412: for (auto& it :
m_focusChangedObservers) {
On 2017/02/02 16:07:20, jbroman wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusController.cpp:1414: }
On 2017/02/02 16:07:20, jbroman wrote:
> super-nit: don't need the braces

Done.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:128: void
NavigatorVR::focusedFrameChanged() {
On 2017/02/02 16:07:20, jbroman wrote:
> nit: This condition might be clearer factored out to a static function?
> 
> static bool isFrameDocumentFocused(LocalFrame* frame) {
>   if (!frame) return false;
>   Page* page = frame->page();
>   Document* document = frame->document();
>   return page && document &&
> page->focusController().isDocumentFocused(document);
> }
> 
> void NavigatorVR::focusedFrameChanged() {
>   const bool focused = supplementable() &&
> isFrameDocumentFocused(supplementable()->frame());
>   if (m_focused != focused && m_controller) {
>     m_controller->setListeningForActivate(focused && m_listeningForActivate);
>     m_controller->focusChanged(focused);
>   }
>   m_focused = focused;
> }

Done.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:132:
supplementable()->frame()->page()->focusController().isDocumentFocused(
On 2017/02/02 16:07:20, jbroman wrote:
> Do you really want isDocumentFocused? This will return true if any descendant
> frame has focus as well. This may or may not be what you want, but you
probably
> want to match whatever you'll implement on the browser side later on.

Hmm I think you're right, and isDocumentFocused was probably only working by
fluke since we only allow one VSync provider to be connected at a time.

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.cpp (right):

https://codereview.chromium.org/2668003003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.cpp:78: for (size_t i = 0; i <
m_displays.size(); ++i)
On 2017/02/02 16:07:20, jbroman wrote:
> super-nit: you could use a range-based loop here too:
> 
> for (const auto& display : m_displays)
>   display->focusChanged(focused);

Done.

[haraken, 2017-02-03 04:21:47]

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

[haraken, 2017-02-03 04:21:47]

https://codereview.chromium.org/2668003003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2668003003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:172: bool m_focused = false;

It looks a bit weird that we have to introduce the boolean value on multiple
header files. Can we avoid it somehow?

[mthiesse, 2017-02-03 15:54:21]

https://codereview.chromium.org/2668003003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2668003003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:172: bool m_focused = false;
On 2017/02/03 04:21:47, haraken wrote:
> 
> It looks a bit weird that we have to introduce the boolean value on multiple
> header files. Can we avoid it somehow?

Yes, done.

[haraken, 2017-02-03 18:46:18]

Mostly looks good.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:30: bool
isFrameFocused(LocalFrame* frame) {

I'd move this to FocusChangedObserver.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.h:76: bool m_focused = false;

Maybe would it make more sense to move this boolean to FocusChangedObserver?
i.e., "Focus"ChangedObserver should maintain focus-related things.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.h:41: void focusChanged(bool
focused);

Blink prefers using an enum over boolean for function parameters. Would you
switch the boolean in this CL to an enum (Blured/Focused)?

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:153: int16_t m_frameId = -1;

Not related to this CL, we should not expose a frame ID to Blink...

+dcheng@

[mthiesse, 2017-02-03 20:47:29]

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.cpp (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.cpp:30: bool
isFrameFocused(LocalFrame* frame) {
On 2017/02/03 18:46:18, haraken wrote:
> 
> I'd move this to FocusChangedObserver.

Done.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/NavigatorVR.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/NavigatorVR.h:76: bool m_focused = false;
On 2017/02/03 18:46:18, haraken wrote:
> 
> Maybe would it make more sense to move this boolean to FocusChangedObserver?
> i.e., "Focus"ChangedObserver should maintain focus-related things.

That doesn't really make sense to me in the general case (imagining there may be
other FocusChangedObservers in the future). I'm not sure what it would mean for
the observer to be focused.

Here it's clear that m_focused means the navigator is focused.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRController.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRController.h:41: void focusChanged(bool
focused);
On 2017/02/03 18:46:18, haraken wrote:
> 
> Blink prefers using an enum over boolean for function parameters. Would you
> switch the boolean in this CL to an enum (Blured/Focused)?
> 

Got rid of the parameter rather than adding an enum, as the parameter wasn't
useful anyways.

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.h (right):

https://codereview.chromium.org/2668003003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.h:153: int16_t m_frameId = -1;
On 2017/02/03 18:46:18, haraken wrote:
> 
> Not related to this CL, we should not expose a frame ID to Blink...
> 
> +dcheng@

I don't think this is the kind of frame ID you're thinking of.

This frame ID is a hacky mechanism for the browser to know which VR Pose each
frame it received was rendered with. The frame ID is literally drawn into the
webVR canvas (as a pixel in the corner), and read back inside the browser.

Even with a compromised renderer, knowing or manipulating this frameID would do
nothing but cause webVR to render incorrectly, which either a correctly
functioning page, or a compromised renderer, could already do anyways.

[bajones, 2017-02-03 21:11:09]

On 2017/02/03 20:47:29, mthiesse wrote:
> I don't think this is the kind of frame ID you're thinking of.
> 
> This frame ID is a hacky mechanism for the browser to know which VR Pose each
> frame it received was rendered with. The frame ID is literally drawn into the
> webVR canvas (as a pixel in the corner), and read back inside the browser.
> 
> Even with a compromised renderer, knowing or manipulating this frameID would
do
> nothing but cause webVR to render incorrectly, which either a correctly
> functioning page, or a compromised renderer, could already do anyways.

I was just about to write and say the same thing. There's an unfortunate
namespace conflict in the context of this CL. Maybe this could be "poseID"
instead, since that's what it's actually tracking at this point?

[mthiesse, 2017-02-03 21:17:07]

On 2017/02/03 21:11:09, bajones wrote:
> On 2017/02/03 20:47:29, mthiesse wrote:
> > I don't think this is the kind of frame ID you're thinking of.
> > 
> > This frame ID is a hacky mechanism for the browser to know which VR Pose
each
> > frame it received was rendered with. The frame ID is literally drawn into
the
> > webVR canvas (as a pixel in the corner), and read back inside the browser.
> > 
> > Even with a compromised renderer, knowing or manipulating this frameID would
> do
> > nothing but cause webVR to render incorrectly, which either a correctly
> > functioning page, or a compromised renderer, could already do anyways.
> 
> I was just about to write and say the same thing. There's an unfortunate
> namespace conflict in the context of this CL. Maybe this could be "poseID"
> instead, since that's what it's actually tracking at this point?

It's not tracking poses, it's tracking frames, and it's not only used for poses,
it's also used to apply bounds changes on the right frame.

[bajones, 2017-02-03 21:19:57]

On 2017/02/03 21:17:07, mthiesse wrote:
> It's not tracking poses, it's tracking frames, and it's not only used for
poses,
> it's also used to apply bounds changes on the right frame.

Ah, OK. In that case if it's going to be problematic I'd suggest vrFrameID.
Otherwise probably just warrants a comment to disambiguate this specific case.

[haraken, 2017-02-06 01:33:17]

Thanks, makes sense. LGTM.

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusChangedObserver.h (right):

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusChangedObserver.h:22: void
observe(Page*);

Maybe can we remove the observe() method by making FocusChangedObserver's
constructor call registerFocusChangedObserver()?

i.e., add 'explicit FocusChangedObserver(Page*)'

[jbroman, 2017-02-06 15:34:18]

lgtm 2, fwiw

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:755: if
(m_navigatorVR->isFocused()) {
nit: This check seems redundant with the one in ConnectVSyncProvider. Is it
useful to check twice?

[mthiesse, 2017-02-06 16:37:47]

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/FocusChangedObserver.h (right):

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/FocusChangedObserver.h:22: void
observe(Page*);
On 2017/02/06 01:33:17, haraken wrote:
> 
> Maybe can we remove the observe() method by making FocusChangedObserver's
> constructor call registerFocusChangedObserver()?
> 
> i.e., add 'explicit FocusChangedObserver(Page*)'

Done.

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/vr/VRDisplay.cpp (right):

https://codereview.chromium.org/2668003003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/vr/VRDisplay.cpp:755: if
(m_navigatorVR->isFocused()) {
On 2017/02/06 15:34:18, jbroman wrote:
> nit: This check seems redundant with the one in ConnectVSyncProvider. Is it
> useful to check twice?

Removed redundant check.

[mthiesse, 2017-02-06 16:37:52]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-02-06 16:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2668003003/#ps220001
(title: "Address comments")

[commit-bot: I haz the power, 2017-02-06 16:38:08]

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-02-06 18:31:11]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1486399072302860,
"parent_rev": "807f8f0f2e030129bebe6efd0bb26ddaed5a7366", "commit_rev":
"527310a8c856c31a016c8f9c9ddb90a5290aef5b"}

[commit-bot: I haz the power, 2017-02-06 18:31:42]

Description was changed from

==========
Provide WebVR pose data only to the focused frame.

Testing is tracked in issue 679827. For now this is manually tested.

BUG=683322, 686890
==========

to

==========
Provide WebVR pose data only to the focused frame.

Testing is tracked in issue 679827. For now this is manually tested.

BUG=683322, 686890

Review-Url: https://codereview.chromium.org/2668003003
Cr-Commit-Position: refs/heads/master@{#448315}
Committed:
https://chromium.googlesource.com/chromium/src/+/527310a8c856c31a016c8f9c9ddb...
==========

[commit-bot: I haz the power, 2017-02-06 18:31:49]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/527310a8c856c31a016c8f9c9ddb...