Adding retry logic for applying desktop resolution changes to RDP session.

Adding retry logic for applying desktop resolution changes to RDP session.

This change adds retry logic for applying resolution changes when the session
is first started.  The RDP API we rely on can return E_UNEXPECTED for a few
seconds after the user logs in.  This is not documented on MSDN but has been
experienced by other users of the API.  This change adds a timer which is
used to retry the resolution update until it succeeds or reaches the max
number of attempts.  Once the user is logged on, we do not use the retry
logic as calls after that point are expected to succeed.

BUG=392220
Committed: https://crrev.com/b901d5b1a44d24ffc592dd8293fd86ef984f87f6
Cr-Commit-Position: refs/heads/master@{#441382}

[joedow, 2016-12-22 22:48:17]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 22:48:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-12-22 23:11:14]

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

[joedow, 2016-12-22 23:11:15]

PTAL!

Note: I chose to use a timer here instead of posting a delayed task as I wanted
to avoid using a WeakPtr if I could.  With this mechanism, I can stop the timer
if the session is disconnected.  LMK if this seems reasonable.

Thanks!
Joe

[commit-bot: I haz the power, 2016-12-22 23:13:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 23:13:41]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-12-23 00:32:13]

Patchset #3 (id:40001) has been deleted

[joedow, 2016-12-23 00:32:22]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 00:32:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 00:53:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 00:53:11]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-12-27 21:16:13]

lgtm

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
File remoting/host/win/rdp_client_window.cc (right):

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:232: << result << std::dec;
I don't think you need std::dec here. Each LOG() creates a separate stream, so
std::hex won't be applied to other messages.

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:547: if (attempt_count >
kMaxResolutionReapplyAttempts) {
Maybe just don't post the task after kMaxResolutionReapplyAttempts attempts?
That way attempt_count would have to be checked only in one place

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:561:
apply_resolution_timer_.Start(FROM_HERE,
It may be easier to start a repeating timer and then stop it as soon as the
operation succeeds or 20 failed attempts.

[joedow, 2017-01-04 15:47:44]

Thanks!

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
File remoting/host/win/rdp_client_window.cc (right):

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:232: << result << std::dec;
On 2016/12/27 21:16:13, Sergey Ulanov wrote:
> I don't think you need std::dec here. Each LOG() creates a separate stream, so
> std::hex won't be applied to other messages.

Done.

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:547: if (attempt_count >
kMaxResolutionReapplyAttempts) {
On 2016/12/27 21:16:13, Sergey Ulanov wrote:
> Maybe just don't post the task after kMaxResolutionReapplyAttempts attempts?
> That way attempt_count would have to be checked only in one place

Done.

https://codereview.chromium.org/2600623002/diff/60001/remoting/host/win/rdp_c...
remoting/host/win/rdp_client_window.cc:561:
apply_resolution_timer_.Start(FROM_HERE,
On 2016/12/27 21:16:13, Sergey Ulanov wrote:
> It may be easier to start a repeating timer and then stop it as soon as the
> operation succeeds or 20 failed attempts.

I think that's a good call.  I've updated it to use a repeating timer.

[joedow, 2017-01-04 15:48:59]

The CQ bit was checked by joedow@chromium.org

[joedow, 2017-01-04 15:48:59]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2600623002/#ps80001
(title: "Addressing CR feedback")

[commit-bot: I haz the power, 2017-01-04 15:49:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 16:08:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483544939327950,
"parent_rev": "d1235b67eb4784eb90c96fda07c55f58709355be", "commit_rev":
"6fde7c0f67cfcb3d50d7134eafc3da0904634a44"}

[commit-bot: I haz the power, 2017-01-04 16:09:18]

Description was changed from

==========
Adding retry logic for applying desktop resolution changes to RDP session.

This change adds retry logic for applying resolution changes when the session
is first started.  The RDP API we rely on can return E_UNEXPECTED for a few
seconds after the user logs in.  This is not documented on MSDN but has been
experienced by other users of the API.  This change adds a timer which is
used to retry the resolution update until it succeeds or reaches the max
number of attempts.  Once the user is logged on, we do not use the retry
logic as calls after that point are expected to succeed.

BUG=392220
==========

to

==========
Adding retry logic for applying desktop resolution changes to RDP session.

This change adds retry logic for applying resolution changes when the session
is first started.  The RDP API we rely on can return E_UNEXPECTED for a few
seconds after the user logs in.  This is not documented on MSDN but has been
experienced by other users of the API.  This change adds a timer which is
used to retry the resolution update until it succeeds or reaches the max
number of attempts.  Once the user is logged on, we do not use the retry
logic as calls after that point are expected to succeed.

BUG=392220

Review-Url: https://codereview.chromium.org/2600623002
==========

[commit-bot: I haz the power, 2017-01-04 16:09:19]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2017-01-04 16:11:09]

Description was changed from

==========
Adding retry logic for applying desktop resolution changes to RDP session.

This change adds retry logic for applying resolution changes when the session
is first started.  The RDP API we rely on can return E_UNEXPECTED for a few
seconds after the user logs in.  This is not documented on MSDN but has been
experienced by other users of the API.  This change adds a timer which is
used to retry the resolution update until it succeeds or reaches the max
number of attempts.  Once the user is logged on, we do not use the retry
logic as calls after that point are expected to succeed.

BUG=392220

Review-Url: https://codereview.chromium.org/2600623002
==========

to

==========
Adding retry logic for applying desktop resolution changes to RDP session.

This change adds retry logic for applying resolution changes when the session
is first started.  The RDP API we rely on can return E_UNEXPECTED for a few
seconds after the user logs in.  This is not documented on MSDN but has been
experienced by other users of the API.  This change adds a timer which is
used to retry the resolution update until it succeeds or reaches the max
number of attempts.  Once the user is logged on, we do not use the retry
logic as calls after that point are expected to succeed.

BUG=392220

Committed: https://crrev.com/b901d5b1a44d24ffc592dd8293fd86ef984f87f6
Cr-Commit-Position: refs/heads/master@{#441382}
==========

[commit-bot: I haz the power, 2017-01-04 16:11:11]

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