Moving oauth code from host to base to allow code sharing between host and client.

Moving oauth code from host to base to allow code sharing between host and client.

BUG=652781
+rogerta@ for '+google_apis', DEPS change.

Review-Url: https://codereview.chromium.org/2661153003
Cr-Commit-Position: refs/heads/master@{#447878}
Committed: https://chromium.googlesource.com/chromium/src/+/f620a47cade760e6047bcaff139d12376f2f1014

[nicholss, 2017-01-31 16:31:30]

Description was changed from

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781
==========

to

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781
==========

[nicholss, 2017-01-31 16:31:30]

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

[nicholss, 2017-01-31 16:44:58]

The majority of work here is just moving the oauth related files into a common
place to be used for both client and host.

https://codereview.chromium.org/2661153003/diff/1/remoting/BUILD.gn
File remoting/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/BUILD.gn#newcode110
remoting/BUILD.gn:110: "//remoting/base:test_support",
I included base in base:test_support so it comes along with this dep. Not sure
if this is very clear and can dep on both if needed.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/BUILD.gn
File remoting/base/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/BUILD.gn#newc...
remoting/base/BUILD.gn:23: 
Will fix.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/fake_oauth_to...
File remoting/base/fake_oauth_token_getter.cc (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/fake_oauth_to...
remoting/base/fake_oauth_token_getter.cc:17: : status_(status),
user_email_(user_email), access_token_(access_token) {}
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/gaia_oauth_cl...
File remoting/base/gaia_oauth_client.cc (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/gaia_oauth_cl...
remoting/base/gaia_oauth_client.cc:18: :
gaia_oauth_client_(url_request_context_getter.get()) {}
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/logging.h
File remoting/base/logging.h (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/logging.h#new...
remoting/base/logging.h:17: #define CRD_LOG LOG(INFO)
I added CRD_LOG because HOST_LOG was used in the oauth code and it could be host
or client.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/mock_oauth_cl...
File remoting/base/mock_oauth_client.cc (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/mock_oauth_cl...
remoting/base/mock_oauth_client.cc:11: : user_email_(user_email),
refresh_token_(refresh_token) {}
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_client.h
File remoting/base/oauth_client.h (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_client....
remoting/base/oauth_client.h:23: typedef base::Callback<void(const std::string&
user_email,
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_helper.cc
File remoting/base/oauth_helper.cc (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_helper....
remoting/base/oauth_helper.cc:27: return
"https://www.googleapis.com/auth/chromoting "
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_helper_...
File remoting/base/oauth_helper_unittest.cc (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/base/oauth_helper_...
remoting/base/oauth_helper_unittest.cc:13: std::string Replace(const
std::string& s,
git cl format did these changes.

https://codereview.chromium.org/2661153003/diff/1/remoting/host/setup/BUILD.gn
File remoting/host/setup/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/1/remoting/host/setup/BUILD.g...
remoting/host/setup/BUILD.gn:37: "//remoting/base",
this should be :authorization.

[nicholss, 2017-01-31 16:45:10]

PTAL

[Sergey Ulanov, 2017-02-02 01:55:30]

LGTM after my comment is addressed

https://codereview.chromium.org/2661153003/diff/20001/remoting/base/logging.h
File remoting/base/logging.h (right):

https://codereview.chromium.org/2661153003/diff/20001/remoting/base/logging.h...
remoting/base/logging.h:17: #define CRD_LOG LOG(INFO)
HOST_LOG has clear purpose (messages that are logged to the host log), but
CRD_LOG doesn't. I'd prefer replacing HOST_LOG in the code you moved with
VLOG(1). These messages are not very useful anyway.

[nicholss, 2017-02-02 02:03:34]

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

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

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-02-02 06:29:00]

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

[commit-bot: I haz the power, 2017-02-02 06:29:01]

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-02-02 17:18:06]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-02-02 17:18:07]

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/2661153003/#ps40001
(title: "Changing CRD_LOG to VLOG(1).")

[commit-bot: I haz the power, 2017-02-02 17:18:53]

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-02 17:24:38]

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

[commit-bot: I haz the power, 2017-02-02 17:24:39]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nicholss, 2017-02-02 17:46:10]

The CQ bit was checked by nicholss@chromium.org

[commit-bot: I haz the power, 2017-02-02 17:46:42]

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-02 17:53:10]

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

[commit-bot: I haz the power, 2017-02-02 17:53:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nicholss, 2017-02-02 18:28:17]

Description was changed from

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781
==========

to

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781

+rogerta@ for '+google_apis', DEPS change.
==========

[nicholss, 2017-02-02 18:28:17]

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

[Roger Tawa OOO till Jul 10th, 2017-02-02 19:09:59]

Do you still need the google_apis deps in //remoting/host/DEPS?

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/BUILD.gn
File remoting/host/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/BUILD.gn#...
remoting/host/BUILD.gn:311: "//google_apis",
Still need this line?

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/setup/BUI...
File remoting/host/setup/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/setup/BUI...
remoting/host/setup/BUILD.gn:36: "//google_apis",
Should this line be removed?

[Sergey Ulanov, 2017-02-02 19:31:50]

On Thu, Feb 2, 2017 at 11:09 AM, <rogerta@chromium.org> wrote:

> Do you still need the google_apis deps in //remoting/host/DEPS?
>

There are still several places where remoting/host depends on google_apis
(mainly for API keys).

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nicholss, 2017-02-02 19:41:45]

The includes for google_apis is still valid in remoting/host. I am still working
on abstracting out the oauth key gathering code which will be a followup CL in
the coming week. At that time I will be sure to remove the DEPS on google_apis
inside of remoting/host, consolidating it all to remoting/base.

Thanks for looking!

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/BUILD.gn
File remoting/host/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/BUILD.gn#...
remoting/host/BUILD.gn:311: "//google_apis",
On 2017/02/02 19:09:59, Roger Tawa wrote:
> Still need this line?

Yes, service_urls.cc and signaling_connector.cc uses this still.

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/setup/BUI...
File remoting/host/setup/BUILD.gn (right):

https://codereview.chromium.org/2661153003/diff/40001/remoting/host/setup/BUI...
remoting/host/setup/BUILD.gn:36: "//google_apis",
On 2017/02/02 19:09:59, Roger Tawa wrote:
> Should this line be removed?

me2me_native_messaging_host and host_starter.cc uses google_api_keys.h to get
keys.

[nicholss, 2017-02-02 19:44:11]

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

[commit-bot: I haz the power, 2017-02-02 19:44:56]

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

[Roger Tawa OOO till Jul 10th, 2017-02-02 20:59:37]

lgtm

Thanks for explanation.

[commit-bot: I haz the power, 2017-02-02 21:29:19]

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

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

Dry run: This issue passed the CQ dry run.

[nicholss, 2017-02-02 21:34:27]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-02-02 21:34:28]

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/2661153003/#ps60001
(title: "Able to remove goole_apis include from host/win. Rest are valid
includes still.")

[commit-bot: I haz the power, 2017-02-02 21:35:04]

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

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

[commit-bot: I haz the power, 2017-02-02 21:41:11]

Failed to apply patch for remoting/host/win/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: remoting/host/win/BUILD.gn:119
  error: remoting/host/win/BUILD.gn: patch does not apply

Patch:       remoting/host/win/BUILD.gn
Index: remoting/host/win/BUILD.gn
diff --git a/remoting/host/win/BUILD.gn b/remoting/host/win/BUILD.gn
index
780cd508e0e1f15dfe09bac88d40fcd8d2e44369..080109fb02a9d15b7044b7a98dba5b2e1f84def6
100644
--- a/remoting/host/win/BUILD.gn
+++ b/remoting/host/win/BUILD.gn
@@ -119,7 +119,6 @@ source_set("win") {
     "//content/public/common",
     "//crypto",
     "//device/power_save_blocker",
-    "//google_apis",
     "//ipc",
     "//remoting/base",
     "//remoting/host/security_key",

[nicholss, 2017-02-02 22:15:21]

The CQ bit was checked by nicholss@chromium.org

[nicholss, 2017-02-02 22:15:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/2661153003/#ps80001
(title: "Merge branch 'master' into auth_token")

[commit-bot: I haz the power, 2017-02-02 22:15:48]

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-02 23:39:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486073721099030,
"parent_rev": "6c634d27d668dab8d440d5c7aeefc62fdaa85c8e", "commit_rev":
"f620a47cade760e6047bcaff139d12376f2f1014"}

[commit-bot: I haz the power, 2017-02-02 23:39:44]

Description was changed from

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781

+rogerta@ for '+google_apis', DEPS change.
==========

to

==========
Moving oauth code from host to base to allow code sharing between host and
client.

BUG=652781

+rogerta@ for '+google_apis', DEPS change.

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

[commit-bot: I haz the power, 2017-02-02 23:39:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f620a47cade760e6047bcaff139d...