Rename ClientDimensions to ClientResolution and add pixel-size and DPI fields.

Rename ClientDimensions to ClientResolution and add pixel-size and DPI fields.

This CL also updates the client and host protocol handlers to the new name.

BUG=172404
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182115

[Wez, 2013-02-10 23:11:48]

PTAL; this is mostly a straight re-name, aside from the proto changes and client
& host side dispatcher/stubs adding DPI fields.

[Jamie, 2013-02-11 20:35:37]

https://chromiumcodereview.appspot.com/12220092/diff/1/remoting/client/plugin...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/12220092/diff/1/remoting/client/plugin...
remoting/client/plugin/chromoting_instance.cc:596: void
ChromotingInstance::NotifyClientDimensions(int width, int height)
This won't compile, surely? There's no opening brace, and you've removed
NotifyClientDimensions from the class definition.

[Wez, 2013-02-11 22:26:39]

On 2013/02/11 20:35:37, Jamie wrote:
>
https://chromiumcodereview.appspot.com/12220092/diff/1/remoting/client/plugin...
> File remoting/client/plugin/chromoting_instance.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12220092/diff/1/remoting/client/plugin...
> remoting/client/plugin/chromoting_instance.cc:596: void
> ChromotingInstance::NotifyClientDimensions(int width, int height)
> This won't compile, surely? There's no opening brace, and you've removed
> NotifyClientDimensions from the class definition.

Yup; broke client bits while splitting stuff out into separate CLs - fixed in
the latest patch set.

[Jamie, 2013-02-11 23:25:57]

lgtm, assuming simple answers to my comments. If not then I'll take another
look.

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/host/clien...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/host/clien...
remoting/host/client_session.cc:92: SkIPoint::Make(kDefaultDPI, kDefaultDPI));
Why not use the new fields, if present? Is that coming in a follow-up?

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/host/resiz...
File remoting/host/resizing_host_observer_unittest.cc (right):

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/host/resiz...
remoting/host/resizing_host_observer_unittest.cc:78: "", client_size,
SkIPoint());
Can you add a unit test to make sure that the DPI information is propagated
correctly?

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/proto/cont...
File remoting/proto/control.proto (right):

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/proto/cont...
remoting/proto/control.proto:16: optional int32 dips_height = 2;
I assume the names are only important for the accessor functions? In other
words, this doesn't break protocol compatibility?

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/proto/inte...
File remoting/proto/internal.proto (right):

https://chromiumcodereview.appspot.com/12220092/diff/1020/remoting/proto/inte...
remoting/proto/internal.proto:20: optional ClientResolution client_resolution =
2;
I assume that the size difference between ClientDimensions and ClientResolution
isn't going to cause protocol incompatibilities?

[Wez, 2013-02-12 01:55:28]

https://codereview.chromium.org/12220092/diff/1020/remoting/host/client_sessi...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/12220092/diff/1020/remoting/host/client_sessi...
remoting/host/client_session.cc:92: SkIPoint::Make(kDefaultDPI, kDefaultDPI));
On 2013/02/11 23:25:58, Jamie wrote:
> Why not use the new fields, if present? Is that coming in a follow-up?

Adding that separately, in crrev.com/12225113.

https://codereview.chromium.org/12220092/diff/1020/remoting/host/resizing_hos...
File remoting/host/resizing_host_observer_unittest.cc (right):

https://codereview.chromium.org/12220092/diff/1020/remoting/host/resizing_hos...
remoting/host/resizing_host_observer_unittest.cc:78: "", client_size,
SkIPoint());
On 2013/02/11 23:25:58, Jamie wrote:
> Can you add a unit test to make sure that the DPI information is propagated
> correctly?

You mean when hooking up the host-side DPI-handling?  SGTM.

https://codereview.chromium.org/12220092/diff/1020/remoting/proto/control.proto
File remoting/proto/control.proto (right):

https://codereview.chromium.org/12220092/diff/1020/remoting/proto/control.pro...
remoting/proto/control.proto:16: optional int32 dips_height = 2;
On 2013/02/11 23:25:58, Jamie wrote:
> I assume the names are only important for the accessor functions? In other
> words, this doesn't break protocol compatibility?

The numbers identify the fields at the protocol level - you can rename stuff
e.g. field to obsolete_field so long as the numbers are unmodified.

https://codereview.chromium.org/12220092/diff/1020/remoting/proto/internal.proto
File remoting/proto/internal.proto (right):

https://codereview.chromium.org/12220092/diff/1020/remoting/proto/internal.pr...
remoting/proto/internal.proto:20: optional ClientResolution client_resolution =
2;
On 2013/02/11 23:25:58, Jamie wrote:
> I assume that the size difference between ClientDimensions and
ClientResolution
> isn't going to cause protocol incompatibilities?

These are protobufs, so you can add (and remove[*]) fields as much as you like.

[*] So long as you don't try to re-use the removed members' indices...

[commit-bot: I haz the power, 2013-02-12 02:01:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12220092/1020

[commit-bot: I haz the power, 2013-02-13 02:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/12220092/1020

[commit-bot: I haz the power, 2013-02-13 04:31:03]

Change committed as 182115