Migrating host preferences, runtime, and gestures to chromium from internal.

Migrating host prefrences, runtime, and gestures to chromium from internal.

Moving the chromoting iOS app out of internal and into chromium will allow
us to find and fix bugs more easily and maintain the codebase without
special rights. This CL moves over a bit more of the app to chromium,
these files will be used to implemenet a demo app in the pending CL
2555803002.

BUG=671692
Review-Url: https://codereview.chromium.org/2628403002
Cr-Commit-Position: refs/heads/master@{#444756}
Committed: https://chromium.googlesource.com/chromium/src/+/b26df2b433fe9371a30f48e8476361ca71546f53

[nicholss, 2017-01-13 19:16:07]

Description was changed from

==========
Migrating host prefrences, runtime, and gestures to chromium from internal.

Moving the chromoting iOS app out of internal and into chromium will allow
us to find and fix bugs more easily and maintain the codebase without
special rights. This CL moves over a bit more of the app to chromium,
these files will be used to implemenet a demo app in the pending CL
2555803002.

BUG=671692
==========

to

==========
Migrating host prefrences, runtime, and gestures to chromium from internal.

Moving the chromoting iOS app out of internal and into chromium will allow
us to find and fix bugs more easily and maintain the codebase without
special rights. This CL moves over a bit more of the app to chromium,
these files will be used to implemenet a demo app in the pending CL
2555803002.

BUG=671692
==========

[nicholss, 2017-01-13 19:16:07]

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

[nicholss, 2017-01-13 19:20:34]

PTAL, thanks!

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
File remoting/client/ios/app_runtime.cc (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
remoting/client/ios/app_runtime.cc:41: // TODO(nicholss): Shutdown the app.
Not sure what needs to be here yet because the app has not finished being
ported/implemented. This is a reminder that the runtime should be shutdown
correctly.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/bridge/...
File remoting/client/ios/bridge/client_instance_unittest.mm (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/bridge/...
remoting/client/ios/bridge/client_instance_unittest.mm:190: [newHost
saveToKeychain];
Removing the SSO language. Attempting to make it generic for chromium.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/bridge/...
File remoting/client/ios/bridge/host_preferences.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/bridge/...
remoting/client/ios/bridge/host_preferences.h:20: // Commit the current idendity
to the keychain.
More language changing to remove SSO.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/client_...
File remoting/client/ios/client_gestures.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/client_...
remoting/client/ios/client_gestures.h:32: 
Client Gestures is a new class, untested yet and only testable during
integration (not there yet). This is better than how it was before: jammed
inside of the host view controller.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences_chromium.mm (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:4: 
This is an implementation for Chromium to allow the build to build without
missing symbols.

[Sergey Ulanov, 2017-01-18 22:20:35]

Mostly style nits

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
File remoting/client/ios/app_runtime.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
remoting/client/ios/app_runtime.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
2017, same in all  other new files

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:13: NSError*
writeHostPreferencesToKeychain(NSData*);
nit: add argument name

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:13: NSError*
writeHostPreferencesToKeychain(NSData*);
Should this be WriteHostPreferencesToKeychain() with upper-case W?
ObjC style guide doesn't say anything about function names, only methods, and
according to C++ style guide, WriteHostPreferencesToKeychain() is the right
name.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:13: NSError*
writeHostPreferencesToKeychain(NSData*);
The name suggests that this function stores preferences for a single host, but
it is called to store pairing info for all hosts. Maybe rename it to make it
clearer.
Please add a comments to make it clear what these functions are used for.

Also would it make sense to move these two functions to a separate header (e.g.
host_preferences_store)?

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:15: }
nit: add empty line

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:15: }
} // namespace ios
} // namespace remoting

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences.mm (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.mm:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
2017

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences_chromium.mm (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:13: return nil;
NOTIMPLEMENTED();

Also, would it be useful to save |data| in a static variable, which is then
returned from readHostPreferencesFromKeychain()?
That would allow to test pairing, even though the pairing info is not persisted
when the app is restarted.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:17: return nil;
NOTIMPLEMENTED()

[nicholss, 2017-01-18 23:21:50]

Thanks for reviewing, comments addressed. PTAL, thanks.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
File remoting/client/ios/app_runtime.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
remoting/client/ios/app_runtime.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> 2017, same in all  other new files

Acknowledged. But I have been trying to land these files (from another CL) since
2016 so it was up to date when the file was created. Does the date mean when it
was created or landed?

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:13: NSError*
writeHostPreferencesToKeychain(NSData*);
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> The name suggests that this function stores preferences for a single host, but
> it is called to store pairing info for all hosts. Maybe rename it to make it
> clearer.

A single host is not implied, it is managing the object called HostPreferences.

> Please add a comments to make it clear what these functions are used for.

Done

> Also would it make sense to move these two functions to a separate header
(e.g.
> host_preferences_store)?

Done.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:15: }
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> } // namespace ios
> } // namespace remoting

Done.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences.h:15: }
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
File remoting/client/ios/host_preferences_chromium.mm (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:13: return nil;
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> NOTIMPLEMENTED();

Done.

> Also, would it be useful to save |data| in a static variable, which is then
> returned from readHostPreferencesFromKeychain()?
> That would allow to test pairing, even though the pairing info is not
persisted
> when the app is restarted.

That is is a good idea. I would have to work out a way to copy the data to a
local var, and I am not sure I care to work that out at the moment but I will
add a TODO to remember to get to that when I work on testing the keychain
functionality. I am not 100% sure this implementation will be the final way we
do it. I am just modifying it in the minimum way possible to be able to get this
into chromium from internal.

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/host_pr...
remoting/client/ios/host_preferences_chromium.mm:17: return nil;
On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> NOTIMPLEMENTED()

Done.

[nicholss, 2017-01-18 23:24:59]

Patchset #2 (id:20001) has been deleted

[Sergey Ulanov, 2017-01-18 23:55:50]

lgtm

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
File remoting/client/ios/app_runtime.h (right):

https://codereview.chromium.org/2628403002/diff/1/remoting/client/ios/app_run...
remoting/client/ios/app_runtime.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2017/01/18 23:21:49, nicholss wrote:
> On 2017/01/18 22:20:35, Sergey Ulanov wrote:
> > 2017, same in all  other new files
> 
> Acknowledged. But I have been trying to land these files (from another CL)
since
> 2016 so it was up to date when the file was created. Does the date mean when
it
> was created or landed?

IANAL, so I don't know what it means. Style guide says to write the year the
file is created, but how one defines file being created is ambiguous: it can be
argued it's a different file until it's landed.
In any case, in the past we always updated it for CLs that are landed on a year
different from the year when they were started.

https://codereview.chromium.org/2628403002/diff/40001/remoting/client/ios/hos...
File remoting/client/ios/host_preferences_chromium.mm (right):

https://codereview.chromium.org/2628403002/diff/40001/remoting/client/ios/hos...
remoting/client/ios/host_preferences_chromium.mm:1: // Copyright 2017 The
Chromium Authors. All rights reserved.
Rename this file to host_preferences_persistence_chromium.h

https://codereview.chromium.org/2628403002/diff/40001/remoting/client/ios/hos...
remoting/client/ios/host_preferences_chromium.mm:5: #import
"remoting/client/ios/host_preferences.h"
don't need this include. replace with host_preferences_persistence.h

[nicholss, 2017-01-19 00:31:57]

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

[commit-bot: I haz the power, 2017-01-19 00:33:20]

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, 2017-01-19 02:22:08]

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

[commit-bot: I haz the power, 2017-01-19 02:22:09]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-01-19 16:12:10]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-01-19 16:12:10]

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/2628403002/#ps60001
(title: "Rename impl to match new header file name.")

[commit-bot: I haz the power, 2017-01-19 16:12:27]

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-19 16:17:19]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484842330324850,
"parent_rev": "ffdc6fc479972e46c19680089af42f59ee1d9271", "commit_rev":
"b26df2b433fe9371a30f48e8476361ca71546f53"}

[commit-bot: I haz the power, 2017-01-19 16:17:48]

Description was changed from

==========
Migrating host prefrences, runtime, and gestures to chromium from internal.

Moving the chromoting iOS app out of internal and into chromium will allow
us to find and fix bugs more easily and maintain the codebase without
special rights. This CL moves over a bit more of the app to chromium,
these files will be used to implemenet a demo app in the pending CL
2555803002.

BUG=671692
==========

to

==========
Migrating host prefrences, runtime, and gestures to chromium from internal.

Moving the chromoting iOS app out of internal and into chromium will allow
us to find and fix bugs more easily and maintain the codebase without
special rights. This CL moves over a bit more of the app to chromium,
these files will be used to implemenet a demo app in the pending CL
2555803002.

BUG=671692

Review-Url: https://codereview.chromium.org/2628403002
Cr-Commit-Position: refs/heads/master@{#444756}
Committed:
https://chromium.googlesource.com/chromium/src/+/b26df2b433fe9371a30f48e84763...
==========

[commit-bot: I haz the power, 2017-01-19 16:17:49]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b26df2b433fe9371a30f48e84763...