Propagate DPI information to web-app.

Propagate DPI information to web-app.

BUG=135089
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147605

[Jamie, 2012-07-18 01:09:29]

ptal

[Wez, 2012-07-19 01:00:42]

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/plu...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/plu...
remoting/client/plugin/chromoting_instance.cc:360: data->SetInteger("x_dpi",
dpi.x());
nit: I was going to suggest only passing x_dpi and y_xpi if they are non-zero,
i.e. if the host sent them, but I notice that you're having the plugin fake them
if they're not already present; is that worth it? There's not really any
difference between a client that doesn't understand the DPI fields (and so won't
include them here) and one that does, but is talking to an older host.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
File remoting/client/rectangle_update_decoder.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
remoting/client/rectangle_update_decoder.cc:84: source_dpi.setX(96);
See comment elsewhere re faking the DPI here.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
File remoting/client/rectangle_update_decoder.h (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
remoting/client/rectangle_update_decoder.h:78: // Remote screen resolution in
DPI.
nit: "Vertical & horizontal DPI of the remote screen." or "Dots-per-inch (DPI)
of the remote screen."

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
File remoting/webapp/client_plugin.js (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
remoting/webapp/client_plugin.js:20:
remoting.ClientPlugin.prototype.desktopDpiX;
nit: desktopXDpi, desktopYDpi read more naturally.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:168: /** @type {number} */
(message.data['x_dpi']) : 0;
nit: Treat missing DPI as 96 DPI for consistency with the behaviour when
connecting to a pre-DPI host.

[Jamie, 2012-07-19 23:00:06]

ptal

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/plu...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/plu...
remoting/client/plugin/chromoting_instance.cc:360: data->SetInteger("x_dpi",
dpi.x());
On 2012/07/19 01:00:42, Wez wrote:
> nit: I was going to suggest only passing x_dpi and y_xpi if they are non-zero,
> i.e. if the host sent them, but I notice that you're having the plugin fake
them
> if they're not already present; is that worth it? There's not really any
> difference between a client that doesn't understand the DPI fields (and so
won't
> include them here) and one that does, but is talking to an older host.

Done.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
File remoting/client/rectangle_update_decoder.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
remoting/client/rectangle_update_decoder.cc:84: source_dpi.setX(96);
On 2012/07/19 01:00:42, Wez wrote:
> See comment elsewhere re faking the DPI here.

Done.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
File remoting/client/rectangle_update_decoder.h (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/client/rec...
remoting/client/rectangle_update_decoder.h:78: // Remote screen resolution in
DPI.
On 2012/07/19 01:00:42, Wez wrote:
> nit: "Vertical & horizontal DPI of the remote screen." or "Dots-per-inch (DPI)
> of the remote screen."

Done.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
File remoting/webapp/client_plugin.js (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
remoting/webapp/client_plugin.js:20:
remoting.ClientPlugin.prototype.desktopDpiX;
On 2012/07/19 01:00:42, Wez wrote:
> nit: desktopXDpi, desktopYDpi read more naturally.

Done.

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
File remoting/webapp/client_plugin_async.js (right):

https://chromiumcodereview.appspot.com/10801003/diff/1014/remoting/webapp/cli...
remoting/webapp/client_plugin_async.js:168: /** @type {number} */
(message.data['x_dpi']) : 0;
On 2012/07/19 01:00:42, Wez wrote:
> nit: Treat missing DPI as 96 DPI for consistency with the behaviour when
> connecting to a pre-DPI host.

That was left-over debugging code. Fixed here and in the ctor.

[Wez, 2012-07-19 23:15:28]

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/plu...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/plu...
remoting/client/plugin/chromoting_instance.cc:360: if (dpi.x()) {
nit: In the host code you've used single-line if without {}; since these are the
only single line ifs in this method, you could do the same here.

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/rec...
File remoting/client/rectangle_update_decoder.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/rec...
remoting/client/rectangle_update_decoder.cc:83: decoder_needs_reset = true;
nit: The Decoder doesn't actually need resetting, though; you just need to pass
the new DPI to the FrameConsumer. Restting the Decoder without a resize may
actually confuse it because it'll be in the middle of a stream.

[Jamie, 2012-07-20 00:42:14]

ptal

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/plu...
File remoting/client/plugin/chromoting_instance.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/plu...
remoting/client/plugin/chromoting_instance.cc:360: if (dpi.x()) {
On 2012/07/19 23:15:29, Wez wrote:
> nit: In the host code you've used single-line if without {}; since these are
the
> only single line ifs in this method, you could do the same here.

I only did that for consistency with the rest of that file. I prefer this style
given a choice. However, no-braces seems marginally more prevalent in this file,
so I'll change it.

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/rec...
File remoting/client/rectangle_update_decoder.cc (right):

https://chromiumcodereview.appspot.com/10801003/diff/2016/remoting/client/rec...
remoting/client/rectangle_update_decoder.cc:83: decoder_needs_reset = true;
On 2012/07/19 23:15:29, Wez wrote:
> nit: The Decoder doesn't actually need resetting, though; you just need to
pass
> the new DPI to the FrameConsumer. Restting the Decoder without a resize may
> actually confuse it because it'll be in the middle of a stream.

Done.

[Wez, 2012-07-20 00:43:53]

lgtm

[commit-bot: I haz the power, 2012-07-20 00:45:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10801003/1016

[commit-bot: I haz the power, 2012-07-20 03:03:01]

Change committed as 147605