[DevTools] Support device orientation override on device with sensors.

[DevTools] Support device orientation override on device with sensors.

Previously, override value was quickly replaced by real data from sensors.
Now, we put sensor data on hold until override is disabled.

This patch also fixes the cleanup/navigation issues with override enabled.

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170163

[dgozman, 2014-03-24 15:43:01]

Hi,

Could you please take a look?
Tim: owner review for modules/device_orientation.
Vlad: agent <--> controller interaction.

Thanks,
Dmitry

[timvolodine, 2014-03-25 11:39:52]

On 2014/03/24 15:43:01, dgozman wrote:
> Hi,
> 
> Could you please take a look?
> Tim: owner review for modules/device_orientation.
> Vlad: agent <--> controller interaction.
> 
> Thanks,
> Dmitry

dgozman@: just to clarify, is the intention to have an override on
document-level or globally? if it's global supplying override value to
DeviceOrientationDispatcher would actually be easier. But I suspect that the
override is per document i.e. we can have override on one page and no override
on the other, is that correct?

[dgozman, 2014-03-25 11:44:22]

On 2014/03/25 11:39:52, timvolodine wrote:
> On 2014/03/24 15:43:01, dgozman wrote:
> > Hi,
> > 
> > Could you please take a look?
> > Tim: owner review for modules/device_orientation.
> > Vlad: agent <--> controller interaction.
> > 
> > Thanks,
> > Dmitry
> 
> dgozman@: just to clarify, is the intention to have an override on
> document-level or globally? if it's global supplying override value to
> DeviceOrientationDispatcher would actually be easier. But I suspect that the
> override is per document i.e. we can have override on one page and no override
> on the other, is that correct?

Yes, that's correct. We carefully try to avoid spilling any changes to other
pages/documents.

[timvolodine, 2014-03-25 15:06:33]

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceOrientationController.cpp (right):

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.cpp:57: return;
hmm so I guess we don't fire overrides at regular intervals because they are the
same right? why not simply return here
if(m_override)
  return;

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.cpp:143: if
(m_lastOrientation)
could you use lastData() here instead of m_lastOrientation?

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceOrientationController.h (right):

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.h:66:
RefPtrWillBeMember<WebCore::DeviceOrientationData> m_lastOrientation;
I don't really see why m_lastOrientation is needed. How about having only
m_orientationOverride which would store override data or null if there is no
override. setOverride() would fire didChangeDeviceOrientation() once and set
m_orientationOverride. Inside didChangeDeviceOrientation either fire the
override or do nothing if the override is set. Probably the latter is sufficient
as we need to propagate the data only if it is changing.. wdyt?

[dgozman, 2014-03-25 17:50:30]

PTAL

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceOrientationController.cpp (right):

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.cpp:57: return;
On 2014/03/25 15:06:33, timvolodine wrote:
> hmm so I guess we don't fire overrides at regular intervals because they are
the
> same right? why not simply return here
> if(m_override)
>   return;

Done.

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.cpp:143: if
(m_lastOrientation)
On 2014/03/25 15:06:33, timvolodine wrote:
> could you use lastData() here instead of m_lastOrientation?

Done.

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceOrientationController.h (right):

https://codereview.chromium.org/209333008/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationController.h:66:
RefPtrWillBeMember<WebCore::DeviceOrientationData> m_lastOrientation;
On 2014/03/25 15:06:33, timvolodine wrote:
> I don't really see why m_lastOrientation is needed. How about having only
> m_orientationOverride which would store override data or null if there is no
> override. setOverride() would fire didChangeDeviceOrientation() once and set
> m_orientationOverride. Inside didChangeDeviceOrientation either fire the
> override or do nothing if the override is set. Probably the latter is
sufficient
> as we need to propagate the data only if it is changing.. wdyt?

You are right. I thought we should keep the last data to restore back after
override, but then I saw dispatcher::latestDeviceOrientationData and forgot to
kill this one.

[aandrey, 2014-03-25 19:59:43]

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceOrientationController.cpp (right):

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:130:
m_override = nullptr;
FYI, maybe we should add:
if (m_override == deviceOrientationData)
    return;

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:137:
m_override = nullptr;
FYI, maybe we should add:
if (!m_override)
    return;

[timvolodine, 2014-03-26 13:20:55]

thanks Dmitry, looks much better, some more comments below.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceOrientationController.cpp (right):

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:43: ,
m_override(nullptr)
no need for this, it's initialized to 0 anyway by RefPtr

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:74:
WebCore::DeviceOrientationData* DeviceOrientationController::lastData()
no need for WebCore:: prefix
also everywhere in the code below

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:130:
m_override = nullptr;
On 2014/03/25 19:59:44, aandrey wrote:
> FYI, maybe we should add:
> if (m_override == deviceOrientationData)
>     return;

hmm not sure about this. you would be comparing pointers here anyway. it's
probably fine to just fire an event every time setOverride is called, makes it
also easier for testing..

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:130:
m_override = nullptr;
a more direct way would be: m_override.clear(), also below

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:137:
m_override = nullptr;
On 2014/03/25 19:59:44, aandrey wrote:
> FYI, maybe we should add:
> if (!m_override)
>     return;

+1
that way clearOverride would not interfere with normal dispatch if override was
not set.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceOrientationController.h (right):

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.h:48: void
setOverride(WebCore::DeviceOrientationData*);
could you remove WebCore:: prefixes in this class please?

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.h:66:
RefPtrWillBeMember<WebCore::DeviceOrientationData> m_override;
m_override sounds like a boolean, probably better is m_overrideData or
m_overrideOrientationData

[dgozman, 2014-03-26 13:51:57]

Please take another look. I've also rebaselined a test which I missed before.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceOrientationController.cpp (right):

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:43: ,
m_override(nullptr)
On 2014/03/26 13:20:55, timvolodine wrote:
> no need for this, it's initialized to 0 anyway by RefPtr

Done.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:74:
WebCore::DeviceOrientationData* DeviceOrientationController::lastData()
On 2014/03/26 13:20:55, timvolodine wrote:
> no need for WebCore:: prefix
> also everywhere in the code below

Done.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:130:
m_override = nullptr;
On 2014/03/26 13:20:55, timvolodine wrote:
> a more direct way would be: m_override.clear(), also below

Done.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.cpp:137:
m_override = nullptr;
On 2014/03/26 13:20:55, timvolodine wrote:
> On 2014/03/25 19:59:44, aandrey wrote:
> > FYI, maybe we should add:
> > if (!m_override)
> >     return;
> 
> +1
> that way clearOverride would not interfere with normal dispatch if override
was
> not set.

I agree. Done.

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceOrientationController.h (right):

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.h:48: void
setOverride(WebCore::DeviceOrientationData*);
On 2014/03/26 13:20:55, timvolodine wrote:
> could you remove WebCore:: prefixes in this class please?

Oh, yes!

https://codereview.chromium.org/209333008/diff/50001/Source/modules/device_or...
Source/modules/device_orientation/DeviceOrientationController.h:66:
RefPtrWillBeMember<WebCore::DeviceOrientationData> m_override;
On 2014/03/26 13:20:55, timvolodine wrote:
> m_override sounds like a boolean, probably better is m_overrideData or
> m_overrideOrientationData

Renamed to m_overrideOrientationData.

[timvolodine, 2014-03-27 00:04:29]

lgtm

nice, thanks!
would be good for somebody else to look at *Agent code as well.

https://codereview.chromium.org/209333008/diff/240001/Source/modules/device_o...
File Source/modules/device_orientation/DeviceOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/209333008/diff/240001/Source/modules/device_o...
Source/modules/device_orientation/DeviceOrientationInspectorAgent.cpp:20: static
const char alpha[] = "alpha";
I think you need to indent this, because the scope of this nested namespace is
smaller than the main namespace

[Vladislav Kaznacheev, 2014-03-27 08:27:36]

*Agent code lgtm

[dgozman, 2014-03-27 09:30:32]

Thank you for review!

https://codereview.chromium.org/209333008/diff/240001/Source/modules/device_o...
File Source/modules/device_orientation/DeviceOrientationInspectorAgent.cpp
(right):

https://codereview.chromium.org/209333008/diff/240001/Source/modules/device_o...
Source/modules/device_orientation/DeviceOrientationInspectorAgent.cpp:20: static
const char alpha[] = "alpha";
On 2014/03/27 00:04:30, timvolodine wrote:
> I think you need to indent this, because the scope of this nested namespace is
> smaller than the main namespace

There are similar namespaces in the code without any indentation. I'd rather
follow that for consistency.

[dgozman, 2014-03-27 09:30:41]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-03-27 09:30:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgozman@chromium.org/209333008/240001

[commit-bot: I haz the power, 2014-03-27 10:31:16]

Change committed as 170163