When resizing-to-fit, prefer to match desktop rotation if possible.

When resizing-to-fit, prefer to match desktop rotation if possible.

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

[Jamie, 2013-02-13 18:21:01]

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
File remoting/host/desktop_resizer_win.cc (right):

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:98: DEVMODE existing_mode =
modes_[candidate_size];
Would best_mode_for_size be a better name for this variable? I spent a few
minutes trying to work out what it was for.

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:104: current_mode.dmDisplayOrientation)) {
This will never pick a portrait mode if the physical display is landscape,
correct? I'm not sure that's the right choice for Me2Me, especially if the
display is curtained. I think it deserves a comment at least, explaining that
this is a design choice.

[Wez, 2013-02-13 22:44:55]

PTAL

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
File remoting/host/desktop_resizer_win.cc (right):

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:98: DEVMODE existing_mode =
modes_[candidate_size];
On 2013/02/13 18:21:01, Jamie wrote:
> Would best_mode_for_size be a better name for this variable? I spent a few
> minutes trying to work out what it was for.

You mean modes_ -> best_mode_for_size_?

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:104: current_mode.dmDisplayOrientation)) {
On 2013/02/13 18:21:01, Jamie wrote:
> This will never pick a portrait mode if the physical display is landscape,
> correct? I'm not sure that's the right choice for Me2Me, especially if the
> display is curtained. I think it deserves a comment at least, explaining that
> this is a design choice.

No.  This logic applies iff the mode we're enumerating, and the previous
best-match mode have the same dimensions.  If the new mode's orientation doesn't
match the current mode, but the best-match mode's does, then we prefer the
best-match mode still - i.e. we prefer matching the orientation over matching
the refresh rate.

I'll rephrase this as a single comment explaining that prioritisation across
these blocks.

[Jamie, 2013-02-14 00:10:34]

lgtm

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
File remoting/host/desktop_resizer_win.cc (right):

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:98: DEVMODE existing_mode =
modes_[candidate_size];
On 2013/02/13 22:44:55, Wez wrote:
> On 2013/02/13 18:21:01, Jamie wrote:
> > Would best_mode_for_size be a better name for this variable? I spent a few
> > minutes trying to work out what it was for.
> 
> You mean modes_ -> best_mode_for_size_?

I meant best_mode_for_size instead of existing_mode, but actually I like your
suggestion better.

https://chromiumcodereview.appspot.com/12223124/diff/1/remoting/host/desktop_...
remoting/host/desktop_resizer_win.cc:104: current_mode.dmDisplayOrientation)) {
On 2013/02/13 22:44:55, Wez wrote:
> On 2013/02/13 18:21:01, Jamie wrote:
> > This will never pick a portrait mode if the physical display is landscape,
> > correct? I'm not sure that's the right choice for Me2Me, especially if the
> > display is curtained. I think it deserves a comment at least, explaining
that
> > this is a design choice.
> 
> No.  This logic applies iff the mode we're enumerating, and the previous
> best-match mode have the same dimensions.  If the new mode's orientation
doesn't
> match the current mode, but the best-match mode's does, then we prefer the
> best-match mode still - i.e. we prefer matching the orientation over matching
> the refresh rate.
> 
> I'll rephrase this as a single comment explaining that prioritisation across
> these blocks.

Oh, I see. I had it in mind that the API returned the unrotated width and
height, but that wouldn't be very useful... In that case, this is fine.

[commit-bot: I haz the power, 2013-02-14 00:47:29]

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

[commit-bot: I haz the power, 2013-02-14 05:10:20]

Change committed as 182390