Initial prototype minus drop-shadow.

Initial prototype minus drop-shadow.


BUG=None
TEST=Manual

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

[Jamie, 2011-10-20 00:50:28]

This is a first stab at implementing the new UI mocks for the host-side
disconnect window. There's no drop-shadow, and it's not anti-aliased, but it's
otherwise complete, and resizes to fit the text.

[Wez, 2011-10-20 20:28:58]

LGTM, although there are a load of GDI calls in there that could fail (usually
in low-resource situations), so you should consider how you'd want things to
fail if they returned errors.

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:70: RGB(0.13 * 255, 0.69 * 255, 0.11 *
255))) {
nit: Is this the preferred way to express RGB values?

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:122: // that the entire window is the
title bar.
Does this affect focus or clickability of widgets in the window?

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:175: GetWindowText(control, text,
arraysize(text));
Check the return value of GetWindowText(), otherwise |text| may not be
NULL-terminated.

[Jamie, 2011-10-20 23:56:54]

Ignore the title of the most recent patch--it also adds bidi support to the
dialog. PTAL at that code as well.

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:70: RGB(0.13 * 255, 0.69 * 255, 0.11 *
255))) {
On 2011/10/20 20:28:58, Wez wrote:
> nit: Is this the preferred way to express RGB values?

The fractions are copied from Dave's Mac implementation, that's why I expressed
them this way.

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:122: // that the entire window is the
title bar.
On 2011/10/20 20:28:58, Wez wrote:
> Does this affect focus or clickability of widgets in the window?

No.

http://codereview.chromium.org/8356032/diff/1/remoting/host/disconnect_window...
remoting/host/disconnect_window_win.cc:175: GetWindowText(control, text,
arraysize(text));
On 2011/10/20 20:28:58, Wez wrote:
> Check the return value of GetWindowText(), otherwise |text| may not be
> NULL-terminated.

Done.

[Wez, 2011-10-21 00:46:29]

LGTM, but I think it would be helpful to add comments to the blocks of code
handling placement of the individual controls to indicate what the intended
placement behaviour is.

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:151: // Load the dialog resource so that
we can modify the RTL flags if necessary.
nit: Rephrase so it's clear that we create a modified *copy* of the resource,
rather than trying to modify it in-place.

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:160: CHECK(dialog_pointer);
nit: This changes the failure mode in the case that the resources are missing
from logging an error and doing nothing, to crashing the whole process.  Do you
want that?

[Jamie, 2011-10-21 01:14:50]

I've commented the layout semantics.

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
File remoting/host/disconnect_window_win.cc (right):

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:151: // Load the dialog resource so that
we can modify the RTL flags if necessary.
On 2011/10/21 00:46:29, Wez wrote:
> nit: Rephrase so it's clear that we create a modified *copy* of the resource,
> rather than trying to modify it in-place.

Without an explanation of why this is important, I don't think it adds anything.

http://codereview.chromium.org/8356032/diff/5001/remoting/host/disconnect_win...
remoting/host/disconnect_window_win.cc:160: CHECK(dialog_pointer);
On 2011/10/21 00:46:29, Wez wrote:
> nit: This changes the failure mode in the case that the resources are missing
> from logging an error and doing nothing, to crashing the whole process.  Do
you
> want that?

Yes, I think we're asserting that the disconnect window is important for
privacy, so failing to load it should be a critical failure. I've also added a
CHECK that the dialog was created.

[commit-bot: I haz the power, 2011-10-21 22:35:45]

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

[commit-bot: I haz the power, 2011-10-21 22:35:46]

Can't apply patch for file remoting/host/plugin/host_plugin.rc.
While running patch -p1 --forward --force;
patching file remoting/host/plugin/host_plugin.rc
Hunk #1 FAILED at 91.
Hunk #2 FAILED at 123.
2 out of 2 hunks FAILED -- saving rejects to file
remoting/host/plugin/host_plugin.rc.rej