Adding domain and facade supporting classes for remoting iOS app.

Adding domain and facade supporting classes for remoting iOS app.

Adding domain type objects that come from other services and are
stored locally to keep state for the app to display and act on.
There will be more in the future, but this is all I am using
for the demo app at the moment.
The facade class is all mock data at the moment, the goal will be
for it to hide the impl details internal to remoting and act as
the facade between c++ and Obj-C.

R=sergeyu@chromium.org
BUG=652781
Review-Url: https://codereview.chromium.org/2681173002
Cr-Commit-Position: refs/heads/master@{#452105}
Committed: https://chromium.googlesource.com/chromium/src/+/1c19bd4c6f5128f467954364964288c697e4c306

[nicholss, 2017-02-08 21:45:42]

PTAL.

[Sergey Ulanov, 2017-02-09 00:52:38]

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/domain/...
File remoting/client/ios/domain/host.h (right):

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/domain/...
remoting/client/ios/domain/host.h:5: #ifndef REMOTING_CLIENT_IOS_DOMAIN_HOST_H_
I assume "domain" here doesn't refer to domains in DNS terms. Do we need to
separate this code in a directory separate from remoting_service.h?

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
File remoting/client/ios/facade/remoting_service.h (right):

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:16: -
(void)authenticateWithAccessCode:(NSString*)accessCode;
In OAuth it's usually referred to as access token, not access code. Access code
can be confused with 12-digit number in It2Me.
Also this method name is confusing. Should it be called setAccessToken?

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:18: - (NSArray<Host*>*)getHosts;
Will this the thread while the request is pending?

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:18: - (NSArray<Host*>*)getHosts;
This interface handles getting user info and getting host list from the
directory. These 2 calls are handled by 2 different services, so I'm not sure if
putting it all in the same class is the right thing to do. User-info query is
already handled by OAuth layer, see
https://codesearch.chromium.org/chromium/src/google_apis/gaia/gaia_oauth_clie...
, so I'm not sure we need it here.

[nicholss, 2017-02-09 16:12:40]

Will update code after we talk today at 10am. This CL and the thinking behind it
is the basis for our meeting.

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/domain/...
File remoting/client/ios/domain/host.h (right):

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/domain/...
remoting/client/ios/domain/host.h:5: #ifndef REMOTING_CLIENT_IOS_DOMAIN_HOST_H_
On 2017/02/09 00:52:37, Sergey Ulanov wrote:
> I assume "domain" here doesn't refer to domains in DNS terms. Do we need to
> separate this code in a directory separate from remoting_service.h?

I am using domain in the 'domain model' sense:
https://en.wikipedia.org/wiki/Domain_model

These are the objects that are a result of an API call that an application might
want to keep around to update state. I assumed this was a common term.

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
File remoting/client/ios/facade/remoting_service.h (right):

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:16: -
(void)authenticateWithAccessCode:(NSString*)accessCode;
On 2017/02/09 00:52:37, Sergey Ulanov wrote:
> In OAuth it's usually referred to as access token, not access code. Access
code
> can be confused with 12-digit number in It2Me.
> Also this method name is confusing. Should it be called setAccessToken? 

I was attempting to name this component:
https://tools.ietf.org/html/rfc6749#section-4.1.3 in other places in the code
base we call it an access_code. It seems according to the spec it is called
"authorization_code" so I can change to that.

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:18: - (NSArray<Host*>*)getHosts;
On 2017/02/09 00:52:37, Sergey Ulanov wrote:
> This interface handles getting user info and getting host list from the
> directory. These 2 calls are handled by 2 different services, so I'm not sure
if
> putting it all in the same class is the right thing to do. User-info query is
> already handled by OAuth layer, see
>
https://codesearch.chromium.org/chromium/src/google_apis/gaia/gaia_oauth_clie...
> , so I'm not sure we need it here.

Yes, combining two services into one common endpoint is the design pattern
called 'facade'. I am trying to minimize the touch points between C++ and Obj-C.

https://codereview.chromium.org/2681173002/diff/1/remoting/client/ios/facade/...
remoting/client/ios/facade/remoting_service.h:18: - (NSArray<Host*>*)getHosts;
On 2017/02/09 00:52:37, Sergey Ulanov wrote:
> Will this the thread while the request is pending?

The intent is to make a call into a C++ class that will handle the threading.
The code might change to be callback or emitter based rather than blocking as it
is now.

[nicholss, 2017-02-09 19:09:16]

Renaming access code to authorization code per spec language.

Any other concerns?

[Sergey Ulanov, 2017-02-10 19:02:02]

lgtm, but please see my comments

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
File remoting/client/ios/domain/host.h (right):

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/host.h:11: @interface Host : NSObject
I suggest renaming this to HostInfo. It would be more consistent with Android
and won't be confused with ChromotingHost in remoting/host

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
File remoting/client/ios/domain/user.h (right):

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/user.h:12: @interface User : NSObject
Maybe UserInfo?

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/user.h:14: @property(nonatomic, copy) NSString*
userId;
Not sure what we actually need this field. How would we use it?

[nicholss, 2017-02-13 22:10:11]

After renaming of the objects to *_info I think it makes it much more messy in
my opinion. Because these get pulled in as ObjC objects and the goal is to never
have a ChromotingHost object in the general ObjC codebase I don't think this was
necessary. 

This is clearly a style clash between location based organization and name-based
organization. To the UI, a Host object would be all it really knows about, and
to connect to a host the codepath would take a Host object and pass it to the
facade asking to be connected to this object (that holds the data necessary to
make the connection) and the result of that call would be an object that holds a
connection plus a rendering context.

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
File remoting/client/ios/domain/host.h (right):

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/host.h:11: @interface Host : NSObject
On 2017/02/10 19:02:01, Sergey Ulanov wrote:
> I suggest renaming this to HostInfo. It would be more consistent with Android
> and won't be confused with ChromotingHost in remoting/host

Sounds good. Will do.

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
File remoting/client/ios/domain/user.h (right):

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/user.h:12: @interface User : NSObject
On 2017/02/10 19:02:02, Sergey Ulanov wrote:
> Maybe UserInfo?

Sure, sounds good.

https://codereview.chromium.org/2681173002/diff/20001/remoting/client/ios/dom...
remoting/client/ios/domain/user.h:14: @property(nonatomic, copy) NSString*
userId;
On 2017/02/10 19:02:02, Sergey Ulanov wrote:
> Not sure what we actually need this field. How would we use it?

I am not 100% sure yet, this class is a placeholder.

[Sergey Ulanov, 2017-02-15 20:18:52]

On 2017/02/13 22:10:11, nicholss wrote:
> After renaming of the objects to *_info I think it makes it much more messy in
> my opinion. Because these get pulled in as ObjC objects and the goal is to
never
> have a ChromotingHost object in the general ObjC codebase I don't think this
was
> necessary. 
> 
> This is clearly a style clash between location based organization and
name-based
> organization. To the UI, a Host object would be all it really knows about, and
> to connect to a host the codepath would take a Host object and pass it to the
> facade asking to be connected to this object (that holds the data necessary to
> make the connection) and the result of that call would be an object that holds
a
> connection plus a rendering context.

The issue with calling these classes Host and User is that it makes it harder to
understand the purpose of these classes when reading the code. C++ Style guide
says the following: 
  Give as descriptive a name as possible, within reason. Do not worry about 
  saving horizontal space as it is far more important to make your code 
  immediately understandable by a new reader.

These classes are used just to pass Host/User info and don't do much themselves.
Info suffix makes it clear.

[Sergey Ulanov, 2017-02-15 20:19:08]

lgtm

[nicholss, 2017-02-16 18:37:25]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-02-16 18:38:35]

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-02-16 21:09:59]

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

[commit-bot: I haz the power, 2017-02-16 21:10:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nicholss, 2017-02-21 22:45:43]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-02-21 22:46:39]

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-02-22 00:50:08]

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

[commit-bot: I haz the power, 2017-02-22 00:50:09]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[nicholss, 2017-02-22 16:17:03]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-02-22 16:17:44]

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-02-22 17:19:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487780223666480,
"parent_rev": "cab2befb20c03d2d91eedf162e92ffe8db80e44f", "commit_rev":
"1c19bd4c6f5128f467954364964288c697e4c306"}

[commit-bot: I haz the power, 2017-02-22 17:20:21]

Description was changed from

==========
Adding domain and facade supporting classes for remoting iOS app.

Adding domain type objects that come from other services and are
stored locally to keep state for the app to display and act on.
There will be more in the future, but this is all I am using
for the demo app at the moment.
The facade class is all mock data at the moment, the goal will be
for it to hide the impl details internal to remoting and act as
the facade between c++ and Obj-C.

R=sergeyu@chromium.org
BUG=652781
==========

to

==========
Adding domain and facade supporting classes for remoting iOS app.

Adding domain type objects that come from other services and are
stored locally to keep state for the app to display and act on.
There will be more in the future, but this is all I am using
for the demo app at the moment.
The facade class is all mock data at the moment, the goal will be
for it to hide the impl details internal to remoting and act as
the facade between c++ and Obj-C.

R=sergeyu@chromium.org
BUG=652781

Review-Url: https://codereview.chromium.org/2681173002
Cr-Commit-Position: refs/heads/master@{#452105}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c19bd4c6f5128f4679543649642...
==========

[commit-bot: I haz the power, 2017-02-22 17:20:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1c19bd4c6f5128f4679543649642...