Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2452223002:
It2Me Host changes to better support Confirmation Dialog (Closed)
DescriptionIt2Me Host changes to support Confirmation Dialog state
This change adds a new state to the It2Me connection process. This state
will be sent to the Webapp/client to indicate that the remote user has
started the connection process but has not yet completed it. For our
purposes, this state will signal that the local user must take action
(i.e. click accept on the confirmation dialog) and we can use it to
updated the text / UI state on the local machine.
BUG=658459
Committed: https://crrev.com/a41e7dd53d4bc9f268939d10a02c24302cef8020
Cr-Commit-Position: refs/heads/master@{#428776}
Patch Set 1 #Patch Set 2 : Pre-review cleanup #Patch Set 3 : Fixing unit tests #
Total comments: 10
Patch Set 4 : Addressing feedback #Patch Set 5 : Reordering enum #
Dependent Patchsets: Messages
Total messages: 34 (24 generated)
Description was changed from ========== It2Me Host changes to better support Confirmation Dialog BUG=658459 ========== to ========== It2Me Host changes to support Confirmation Dialog state This change adds a new state to the It2Me connection process. This state will be sent to the Webapp/client to indicate that the remote user has started the connection process but has not yet completed it. For our purposes, this state will signal that the local user must take action (i.e. click accept on the confirmation dialog) and we can use it to updated the text / UI state on the local machine. BUG=658459 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
joedow@chromium.org changed reviewers: + jamiewalch@chromium.org
PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:396: DCHECK(state == kRequestedAccessCode || maybe update formatting for all DCHECKS here to make it consistent https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:414: case kConnecting: Put kConnecting before kConnected https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:434: bool It2MeHost::IsConnected() const { This doesn't look like the right name for this method. Maybe rename it to IsOnline()? https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:50: kConnecting, I don't think there is any reason to keep the current order, so maybe move this above to keep states in their logical order.
LGTM once Sergey's comments are addressed.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed feedback, PTAL! https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.cc (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:396: DCHECK(state == kRequestedAccessCode || On 2016/10/27 22:44:12, Sergey Ulanov wrote: > maybe update formatting for all DCHECKS here to make it consistent I'm going to update the changed conditions to match the old format as I prefer scanning the conditions vertically. https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:414: case kConnecting: On 2016/10/27 22:44:12, Sergey Ulanov wrote: > Put kConnecting before kConnected Done. https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.cc:434: bool It2MeHost::IsConnected() const { On 2016/10/27 22:44:12, Sergey Ulanov wrote: > This doesn't look like the right name for this method. Maybe rename it to > IsOnline()? Changed. Agreed that IsConnected is ambiguous since it means either connected to the service or the client. IsOnline is almost correct but the check contains the kRequestedAccessCode which means it has sent a request but isn't technically online yet. I'm going to call it IsRunning() since it encompasses the non-error states that occur after the kStarting state. Let me know if that sounds good. https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:50: kConnecting, On 2016/10/27 22:44:12, Sergey Ulanov wrote: > I don't think there is any reason to keep the current order, so maybe move this > above to keep states in their logical order. > I wanted to make sure that mismatched host/clients (i.e. new host w/ old client or vice versa) would use the same mapping. Leaving this as-is means that an older client will just log 'unknown' event for the kConnecting state instead of interpreting it as something else.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:50: kConnecting, On 2016/10/31 17:16:07, joedow wrote: > On 2016/10/27 22:44:12, Sergey Ulanov wrote: > > I don't think there is any reason to keep the current order, so maybe move > this > > above to keep states in their logical order. > > > > I wanted to make sure that mismatched host/clients (i.e. new host w/ old client > or vice versa) would use the same mapping. Leaving this as-is means that an > older client will just log 'unknown' event for the kConnecting state instead of > interpreting it as something else. The state is sent from host to client as a string, i.e. as "CONNECTING", so order of the states and their int values shouldn't matter - the webapp should still handle it correctly. (BTW it logs int value of the state here: https://codesearch.chromium.org/chromium/src/remoting/webapp/crd/js/host_scre..., but this doesn't seem useful because it's always converted to -1 at that point. Correct value is logged here: https://codesearch.chromium.org/chromium/src/remoting/webapp/crd/js/host_sess...)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL! https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... File remoting/host/it2me/it2me_host.h (right): https://codereview.chromium.org/2452223002/diff/80001/remoting/host/it2me/it2... remoting/host/it2me/it2me_host.h:50: kConnecting, On 2016/10/31 18:21:17, Sergey Ulanov wrote: > On 2016/10/31 17:16:07, joedow wrote: > > On 2016/10/27 22:44:12, Sergey Ulanov wrote: > > > I don't think there is any reason to keep the current order, so maybe move > > this > > > above to keep states in their logical order. > > > > > > > I wanted to make sure that mismatched host/clients (i.e. new host w/ old > client > > or vice versa) would use the same mapping. Leaving this as-is means that an > > older client will just log 'unknown' event for the kConnecting state instead > of > > interpreting it as something else. > > The state is sent from host to client as a string, i.e. as "CONNECTING", so > order of the states and their int values shouldn't matter - the webapp should > still handle it correctly. > > (BTW it logs int value of the state here: > https://codesearch.chromium.org/chromium/src/remoting/webapp/crd/js/host_scre..., > but this doesn't seem useful because it's always converted to -1 at that point. > Correct value is logged here: > https://codesearch.chromium.org/chromium/src/remoting/webapp/crd/js/host_sess...) Ah, I didn't expect that we were using a stringified version of the values. I've reordered them to match the connection sequence.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org Link to the patchset: https://codereview.chromium.org/2452223002/#ps120001 (title: "Reordering enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== It2Me Host changes to support Confirmation Dialog state This change adds a new state to the It2Me connection process. This state will be sent to the Webapp/client to indicate that the remote user has started the connection process but has not yet completed it. For our purposes, this state will signal that the local user must take action (i.e. click accept on the confirmation dialog) and we can use it to updated the text / UI state on the local machine. BUG=658459 ========== to ========== It2Me Host changes to support Confirmation Dialog state This change adds a new state to the It2Me connection process. This state will be sent to the Webapp/client to indicate that the remote user has started the connection process but has not yet completed it. For our purposes, this state will signal that the local user must take action (i.e. click accept on the confirmation dialog) and we can use it to updated the text / UI state on the local machine. BUG=658459 Committed: https://crrev.com/a41e7dd53d4bc9f268939d10a02c24302cef8020 Cr-Commit-Position: refs/heads/master@{#428776} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a41e7dd53d4bc9f268939d10a02c24302cef8020 Cr-Commit-Position: refs/heads/master@{#428776} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
