Hide the cursor on the client when it is hidden on the host

Hide the cursor on the client when it is hidden on the host

This CL
1. Relaxes the CursorShapeIsValid() check on ClientControlDispatcher
   to allow cursors with empty dimensions.
2. Modifies MouseCursorMonitorAura to sends an empty cursor when the
   cursor is ui::kCursorNone or when the cursor bitmap cannot be loaded.
3. Initializes the CursorShapeInfo with empty data on the VideoScheduler
   as it is a required field.

Committed: https://crrev.com/efa63e0e545f7984e63bda0fb94a5f02449eca20
Cr-Commit-Position: refs/heads/master@{#310175}

[kelvinp, 2014-12-30 21:30:00]

kelvinp@chromium.org changed reviewers:
+ garykac@chromium.org

[kelvinp, 2014-12-30 21:34:31]

kelvinp@chromium.org changed reviewers:
- garykac@chromium.org

[kelvinp, 2014-12-30 21:34:32]

PTAL

[kelvinp, 2015-01-05 17:50:52]

kelvinp@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[kelvinp, 2015-01-05 17:51:03]

kelvinp@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-01-05 18:00:26]

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:58: return;
Should we notify the client that the cursor is hidden in this case (i.e. send an
empty cursor message)?

[Wez, 2015-01-05 20:02:46]

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

[Wez, 2015-01-05 20:02:46]

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:58: return;
On 2015/01/05 18:00:25, Sergey Ulanov wrote:
> Should we notify the client that the cursor is hidden in this case (i.e. send
an
> empty cursor message)?

Yes, please do. There is even an EmptyCursorShape() helper function in the
EmptyCursorFilter header. :)

[kelvinp, 2015-01-06 02:22:18]

PTAL

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/1/remoting/host/chromeos/mouse...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:58: return;
On 2015/01/05 20:02:46, Wez wrote:
> On 2015/01/05 18:00:25, Sergey Ulanov wrote:
> > Should we notify the client that the cursor is hidden in this case (i.e.
send
> an
> > empty cursor message)?
> 
> Yes, please do. There is even an EmptyCursorShape() helper function in the
> EmptyCursorFilter header. :)

Whoa, excellent point.
I have taken the step a little further to make sure that the client
always hide the cursor when the host send an empty cursor.
Please take another look.

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
remoting/host/video_scheduler.cc:124: cursor_proto->set_data(std::string());
Always initializes with empty data.  Or else
ClientControlDispatcher::CursorShapeIsValid on the host will complains that the
cursor info is missing a required field.

[Sergey Ulanov, 2015-01-06 18:52:10]

lgtm when my comments are addressed.

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:54: namespace {
nit: move this before MouseCursorMonitorAura constructor.

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:56: webrtc::MouseCursor*
CreateEmptyMouseCursor() {
Use scoped_ptr<> for the result or add a comment that this function passes
ownership to the caller (not using scoped_ptr<> is acceptable here only because
OnMouseCursor() doesn't use it).

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
remoting/host/video_scheduler.cc:124: cursor_proto->set_data(std::string());
nit: add an empty line above this one and remove it below - that would make it
clear that the block below sets the data field.

[kelvinp, 2015-01-06 22:04:14]

FYI

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:54: namespace {
On 2015/01/06 18:52:10, Sergey Ulanov wrote:
> nit: move this before MouseCursorMonitorAura constructor.

Done.

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/video_sche...
remoting/host/video_scheduler.cc:124: cursor_proto->set_data(std::string());
On 2015/01/06 18:52:10, Sergey Ulanov wrote:
> nit: add an empty line above this one and remove it below - that would make it
> clear that the block below sets the data field.

Done.

[kelvinp, 2015-01-06 22:04:17]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-06 22:04:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/833693002/40001

[Wez, 2015-01-06 23:24:26]

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/mouse_cursor_monitor_aura.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/mouse_cursor_monitor_aura.cc:56: webrtc::MouseCursor*
CreateEmptyMouseCursor() {
Not for this CL: Let's add a CreateEmpty() creator to webrtc::MouseCursor?

https://codereview.chromium.org/833693002/diff/20001/remoting/protocol/client...
File remoting/protocol/client_control_dispatcher.cc (right):

https://codereview.chromium.org/833693002/diff/20001/remoting/protocol/client...
remoting/protocol/client_control_dispatcher.cc:42: height < 0 || height >
(SHRT_MAX / 2)) {
Since you're making this change in the client-side control channel dispatcher,
it won't "go live" until either we switch over entirely to the v2 app, or this
change makes it into Chrome.

Are we already hitting this when other hosts see empty cursors?

[commit-bot: I haz the power, 2015-01-06 23:50:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-06 23:51:23]

Patchset 3 (id:??) landed as
https://crrev.com/efa63e0e545f7984e63bda0fb94a5f02449eca20
Cr-Commit-Position: refs/heads/master@{#310175}

[kelvinp, 2015-01-07 00:07:33]

> Are we already hitting this when other hosts see empty cursors?
Yes, currently the mac and linux hosts doesn't hide the cursor properly.
I haven't checked Windows host yet.
I think this change will fix the problem for all hosts.

[Wez, 2015-01-07 20:47:38]

On 2015/01/07 00:07:33, kelvinp wrote:
> > Are we already hitting this when other hosts see empty cursors?
> Yes, currently the mac and linux hosts doesn't hide the cursor properly.

Can you clarify what you mean by that?

> I haven't checked Windows host yet.
> I think this change will fix the problem for all hosts.

Yes, but hosts and clients are distributed on different schedules, so since this
CL has to modify the client to make it compatible with the host changes, that
suggests we need to do something different to avoid creating issues with older
client plugins.