Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794013005:
CRD iOS: Updating remoting service to use oauth and host list fetcher. (Closed)
DescriptionCRD iOS: Updating remoting service to use oauth and host list fetcher.
This intergrates some work that has landed in remoting base in the last month,
remoting/client/ios now as access to common oauth classes in remoting/base. This
is the integration with those classes and it's use. The app is still in
development so there are still mocked methods and functions but this is a working
first pass.
R=joedow@chromium.org
BUG=652781
Review-Url: https://codereview.chromium.org/2794013005
Cr-Commit-Position: refs/heads/master@{#463369}
Committed: https://chromium.googlesource.com/chromium/src/+/18f139664640da5a1b9fa73225ecbde332d5f39e
Patch Set 1 #Patch Set 2 : Update h guard. #
Total comments: 53
Patch Set 3 : Updated based on feedback. #
Total comments: 2
Patch Set 4 : Fixing up code based on feedback. #
Messages
Total messages: 25 (18 generated)
The CQ bit was checked by nicholss@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 checked by nicholss@chromium.org to run a CQ dry run
PTAL, thanks!
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: This issue passed the CQ dry run.
Looks pretty good, just a few typo/nit comments and questions since I'm not familiar with ObjC patterns and idioms. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/BUILD.gn (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/BUILD.gn:14: "host_list_fetcher.h", Discussed offline, but there are existing unittests for these classes. It would be good to consider porting them once you have settled on your implementation here. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_info.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:11: HostInfo::HostInfo() {} nit: newline https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:39: LOG(ERROR) << "Response Status is " << response_status; I'd change this to say "Unknown response status: " so it is more explicit as to why this is an error. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_list_fetcher.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.cc:34: DCHECK(hostlist_callback_.is_null()); nit: this was written with the old Callback syntax, you can just check the value now: DCHECK(callback); DCHECK(!hostlist_callback_); https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_list_fetcher.h (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.h:25: "chromoting/v1/@me/hosts"; I don't think this line needs to be broken, looks like a git cl format quirk https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.h:27: "https://www-googleapis-test.sandbox.google.com/chromoting/v1/@me/hosts"; Are you using this? I'd remove it if you don't intend to target the test environment. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.h (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:5: #ifndef REMOTING_CLIENT_IOS_FACADE_REMOTING_SERCICE_H_ Can you fix this header guard (SERCICE is a typo): REMOTING_CLIENT_IOS_FACADE_REMOTING_SERVICE_H_ https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:13: // |RemotingAuthenticationDelegate|'s are interesed in authentication related nit: remove apostrophe (non-possessive) and fix interesed (interested) https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:23: // |RemotingHostListDelegate|'s are interested in notifications related to host remove apostrophe https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:43: // Register to be a |RemotingAuthenticationDelegate|. Does the caller need to unregister at some point? If that is a common iOS pattern then feel free to ignore otherwise a comment would be good. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:46: // A cached answer if there is a currently authenticated user. Isn't this just the current authentication state (i.e. no authenticated user returns false otherwise true)? I think I'm getting thrown off by calling it the cached answer instead of the current state. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:50: // of the applicaiton or OAuth Flow. typo: applicaiton -> application https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:56: emai:(NSString*)email; typo: emai -> rename to email https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:63: // delegate is the application will attempt to fetch a fresh host list. Just curious, why is the host list fetched again? If the host list is already fresh (for some definition of fresh), you could just post a task which will call the delegate with the current host list. It's hard for me to tell if that would be better without seeing the usage of this class. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:12: #import "remoting/client/ios/facade/remoting_service.h" In the header the includes were listed before the imports. Does the order matter (by convention or functionally)? https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:24: "google.com/talkgadget/oauth/chrome-remote-desktop/dev"; Typically file scoped vars are put into an anonymous namespace (C++), I'm not sure if ObjC is different or not but wanted to call it out. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:28: std::string auth_code, pass string by const ref, unless ObjC is different https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:34: auth_code, false)); nit: it can be useful to add a comment to bare true/false/int values, i.e. /*is_service_account=*/false https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:45: std::string refresh_token, pass by const ref for these? https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:110: _authDelegate = delegate; Are you using nil as a way to unregister here? I think the typical pattern is add*Observer and remove*Observer, that would also make it easier to maintain a list of observers so multiple instances could register. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:153: if (host_status == remoting::kHostStatusOnline) { nit: you could make this a switch with a default so any changes to the original enum would cause a compiler error (to tell the engineer to fix this logic as well). Up to you for this one though as I'm not sure what new states we would add. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:158: status = "UNKNOWN"; Log UNREACHED() here? https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:246: return _runtime; Can you just return 'remoting::ChromotingClientRuntime::GetInstance()' here and remove the member variable?
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Thanks for the feedback!! I have updated the CL to address actionable comments and commented to on other items to clarify. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/BUILD.gn (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/BUILD.gn:14: "host_list_fetcher.h", On 2017/04/05 22:30:55, joedow wrote: > Discussed offline, but there are existing unittests for these classes. It would > be good to consider porting them once you have settled on your implementation > here. Unit test coverage needs improvement. It is on my list. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_info.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:11: HostInfo::HostInfo() {} On 2017/04/05 22:30:55, joedow wrote: > nit: newline Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:39: LOG(ERROR) << "Response Status is " << response_status; On 2017/04/05 22:30:55, joedow wrote: > I'd change this to say "Unknown response status: " so it is more explicit as to > why this is an error. Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_list_fetcher.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.cc:34: DCHECK(hostlist_callback_.is_null()); On 2017/04/05 22:30:55, joedow wrote: > nit: this was written with the old Callback syntax, you can just check the value > now: > DCHECK(callback); > DCHECK(!hostlist_callback_); Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_list_fetcher.h (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.h:25: "chromoting/v1/@me/hosts"; On 2017/04/05 22:30:55, joedow wrote: > I don't think this line needs to be broken, looks like a git cl format quirk git cl format!!! fixed. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_list_fetcher.h:27: "https://www-googleapis-test.sandbox.google.com/chromoting/v1/@me/hosts"; On 2017/04/05 22:30:55, joedow wrote: > Are you using this? I'd remove it if you don't intend to target the test > environment. I will remove this but I am going to add a todo to remind that we might want to make a test version of the app to connect to test envs. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.h (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:5: #ifndef REMOTING_CLIENT_IOS_FACADE_REMOTING_SERCICE_H_ On 2017/04/05 22:30:55, joedow wrote: > Can you fix this header guard (SERCICE is a typo): > REMOTING_CLIENT_IOS_FACADE_REMOTING_SERVICE_H_ Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:13: // |RemotingAuthenticationDelegate|'s are interesed in authentication related On 2017/04/05 22:30:55, joedow wrote: > nit: remove apostrophe (non-possessive) and fix interesed (interested) Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:23: // |RemotingHostListDelegate|'s are interested in notifications related to host On 2017/04/05 22:30:55, joedow wrote: > remove apostrophe Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:43: // Register to be a |RemotingAuthenticationDelegate|. On 2017/04/05 22:30:55, joedow wrote: > Does the caller need to unregister at some point? If that is a common iOS > pattern then feel free to ignore otherwise a comment would be good. No need to unregister, it is a pretty common iOS pattern to set a delegate and never remove it. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:46: // A cached answer if there is a currently authenticated user. On 2017/04/05 22:30:55, joedow wrote: > Isn't this just the current authentication state (i.e. no authenticated user > returns false otherwise true)? I think I'm getting thrown off by calling it the > cached answer instead of the current state. I said cache because it is not validating that the current auth token is still valid. The user could have an expired token or something for example. I did not want to imply that work would happen when you call this method. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:50: // of the applicaiton or OAuth Flow. On 2017/04/05 22:30:55, joedow wrote: > typo: applicaiton -> application Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:56: emai:(NSString*)email; On 2017/04/05 22:30:55, joedow wrote: > typo: emai -> rename to email HA! I had not gotten to the point where I can use refresh tokens so I did not catch this. :) https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:63: // delegate is the application will attempt to fetch a fresh host list. On 2017/04/05 22:30:55, joedow wrote: > Just curious, why is the host list fetched again? If the host list is already > fresh (for some definition of fresh), you could just post a task which will call > the delegate with the current host list. It's hard for me to tell if that would > be better without seeing the usage of this class. In reality there is only one object that will ever become the host list delegate and this will happen once per application cycle, so there is not a host list yet and setting this will cause the first time fetch of the host list and results in the delegate getting called back a short time later with the host list. It might be cleaner to set the delegate and then have them ask to refresh the host list. I will keep this in-mind when I am working on keeping the host list refreshed and that could rework this flow. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:12: #import "remoting/client/ios/facade/remoting_service.h" On 2017/04/05 22:30:56, joedow wrote: > In the header the includes were listed before the imports. Does the order > matter (by convention or functionally)? Objc guide says this: The standard order for header inclusion is the header for the current file, operating system headers, language library headers, and finally groups of headers for other dependencies. So I think I have been doing it close but not what the spec says. import first, then include for each type of import. I think I still need to understand exactly what the style guide is saying... https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:24: "google.com/talkgadget/oauth/chrome-remote-desktop/dev"; On 2017/04/05 22:30:56, joedow wrote: > Typically file scoped vars are put into an anonymous namespace (C++), I'm not > sure if ObjC is different or not but wanted to call it out. I have not seen any Obj-C files use namespace yet. A const char in objc is namespaced to the current file so it kinda does the same thing as an anon name space already. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:28: std::string auth_code, On 2017/04/05 22:30:55, joedow wrote: > pass string by const ref, unless ObjC is different This is Obj-C++ so const rules still work the same. Fixed. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:34: auth_code, false)); On 2017/04/05 22:30:55, joedow wrote: > nit: it can be useful to add a comment to bare true/false/int values, i.e. > /*is_service_account=*/false Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:45: std::string refresh_token, On 2017/04/05 22:30:55, joedow wrote: > pass by const ref for these? Done. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:110: _authDelegate = delegate; On 2017/04/05 22:30:55, joedow wrote: > Are you using nil as a way to unregister here? I think the typical pattern is > add*Observer and remove*Observer, that would also make it easier to maintain a > list of observers so multiple instances could register. add and set would imply that there can be more than one delegate. I only want one delegate, setBlahBlah is standard obj-c property setting method name. Getters = propName, Setter = setPropName https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:153: if (host_status == remoting::kHostStatusOnline) { On 2017/04/05 22:30:55, joedow wrote: > nit: you could make this a switch with a default so any changes to the original > enum would cause a compiler error (to tell the engineer to fix this logic as > well). Up to you for this one though as I'm not sure what new states we would > add. I like it. Nice idea. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:158: status = "UNKNOWN"; On 2017/04/05 22:30:56, joedow wrote: > Log UNREACHED() here? was not able to find usage of UNREACHED() , can you explain or show an example? https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:246: return _runtime; On 2017/04/05 22:30:56, joedow wrote: > Can you just return 'remoting::ChromotingClientRuntime::GetInstance()' here and > remove the member variable? Done.
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: This issue passed the CQ dry run.
lgtm after comments addressed. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_info.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:11: HostInfo::HostInfo() {} On 2017/04/07 18:16:15, nicholss wrote: > On 2017/04/05 22:30:55, joedow wrote: > > nit: newline > > Done. I don't see the newline in the latest patchset. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:39: LOG(ERROR) << "Response Status is " << response_status; On 2017/04/07 18:16:15, nicholss wrote: > On 2017/04/05 22:30:55, joedow wrote: > > I'd change this to say "Unknown response status: " so it is more explicit as > to > > why this is an error. > > Done. I don't see this in the latest patchset. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.h (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.h:63: // delegate is the application will attempt to fetch a fresh host list. On 2017/04/07 18:16:15, nicholss wrote: > On 2017/04/05 22:30:55, joedow wrote: > > Just curious, why is the host list fetched again? If the host list is already > > fresh (for some definition of fresh), you could just post a task which will > call > > the delegate with the current host list. It's hard for me to tell if that > would > > be better without seeing the usage of this class. > > In reality there is only one object that will ever become the host list delegate > and this will happen once per application cycle, so there is not a host list yet > and setting this will cause the first time fetch of the host list and results in > the delegate getting called back a short time later with the host list. > > It might be cleaner to set the delegate and then have them ask to refresh the > host list. I will keep this in-mind when I am working on keeping the host list > refreshed and that could rework this flow. SGTM https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:158: status = "UNKNOWN"; On 2017/04/07 18:16:16, nicholss wrote: > On 2017/04/05 22:30:56, joedow wrote: > > Log UNREACHED() here? > > was not able to find usage of UNREACHED() , can you explain or show an example? Sorry, the macro is 'NOTREACHED()'. https://codereview.chromium.org/2794013005/diff/40001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/40001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:29: const std::string auth_code, These are const now, but not const ref, they should be: 'const std::string& arg_name'.
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Sorry I made changes to a cruft file with the same name as one commented on... Moved those changes over to this CL now. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/host_info.cc (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:11: HostInfo::HostInfo() {} On 2017/04/10 16:09:59, joedow wrote: > On 2017/04/07 18:16:15, nicholss wrote: > > On 2017/04/05 22:30:55, joedow wrote: > > > nit: newline > > > > Done. > > I don't see the newline in the latest patchset. Ack! Same reason as below, fixed. https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/host_info.cc:39: LOG(ERROR) << "Response Status is " << response_status; On 2017/04/10 16:09:59, joedow wrote: > On 2017/04/07 18:16:15, nicholss wrote: > > On 2017/04/05 22:30:55, joedow wrote: > > > I'd change this to say "Unknown response status: " so it is more explicit as > > to > > > why this is an error. > > > > Done. > > I don't see this in the latest patchset. Sorry! I have a copy of this file in an unchecked in folder and my editor selected that copy and the files are almost the same, the other is my hacking version... FIXED https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/20001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:158: status = "UNKNOWN"; On 2017/04/10 16:09:59, joedow wrote: > On 2017/04/07 18:16:16, nicholss wrote: > > On 2017/04/05 22:30:56, joedow wrote: > > > Log UNREACHED() here? > > > > was not able to find usage of UNREACHED() , can you explain or show an > example? > > Sorry, the macro is 'NOTREACHED()'. Thanks! Updated. https://codereview.chromium.org/2794013005/diff/40001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2794013005/diff/40001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:29: const std::string auth_code, On 2017/04/10 16:09:59, joedow wrote: > These are const now, but not const ref, they should be: > 'const std::string& arg_name'. Done.
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: This issue passed the CQ dry run.
The CQ bit was checked by nicholss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2794013005/#ps60001 (title: "Fixing up code based on feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491853166107510,
"parent_rev": "75cc3b096e245801afa0607dc4f8adc03566ae16", "commit_rev":
"18f139664640da5a1b9fa73225ecbde332d5f39e"}
Message was sent while issue was closed.
Description was changed from ========== CRD iOS: Updating remoting service to use oauth and host list fetcher. This intergrates some work that has landed in remoting base in the last month, remoting/client/ios now as access to common oauth classes in remoting/base. This is the integration with those classes and it's use. The app is still in development so there are still mocked methods and functions but this is a working first pass. R=joedow@chromium.org BUG=652781 ========== to ========== CRD iOS: Updating remoting service to use oauth and host list fetcher. This intergrates some work that has landed in remoting base in the last month, remoting/client/ios now as access to common oauth classes in remoting/base. This is the integration with those classes and it's use. The app is still in development so there are still mocked methods and functions but this is a working first pass. R=joedow@chromium.org BUG=652781 Review-Url: https://codereview.chromium.org/2794013005 Cr-Commit-Position: refs/heads/master@{#463369} Committed: https://chromium.googlesource.com/chromium/src/+/18f139664640da5a1b9fa73225ec... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/18f139664640da5a1b9fa73225ec... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
