Registration of DevTools bridge in GCD.

Registration of DevTools bridge in GCD.

Code for registering and unregistering instance of DevTools bridge in GCD.
GCDRegistrationFragment is able to perform registration including:
1. Asking user for desired account to register.
2. Requesting OAuth permissions for GCD (may show confirmation activity to the user).
3. Registration in Cloud Messages if needed.

It also shows toasts with registration/unregistration results.

GCDRegistrationFragment is absstract and has no views. Testing implementation
shows button "Registed in GCD/Unregister (registered for ...)". Registration is not
persistent yet.

BUG=383418
TEST=New button in main activity of DevToolsBridgeTest.apk: register/unregister.

Committed: https://crrev.com/a2da02bd99d78de549b617510551ec5e1f7073d9
Cr-Commit-Position: refs/heads/master@{#302251}

Committed: https://crrev.com/ffb7ed195778fe9f7649da6b0f60b26f2cdefc4a
Cr-Commit-Position: refs/heads/master@{#302261}

[SeRya, 2014-10-30 17:16:15]

Patchset #2 (id:20001) has been deleted

[SeRya, 2014-10-30 17:38:49]

serya@chromium.org changed reviewers:
+ mnaganov@chromium.org

[SeRya, 2014-10-30 17:38:49]

PTAL

[mnaganov (inactive), 2014-10-30 18:18:22]

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:51:
* Creation registration ticket if the first step in instance registration.
Client must have
nit: The first sentence is incomprehensible. Should it be "Creation of a
registration ticket is the first step..."?

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:52:
* user credentials. If it registered it will be associated with the user. Next
step is patching
nit: Again, the sentence isn't very clear. Should it be "If the ticket has been
registered, it will be..."?

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:53:
* registration ticket.
nit: "patching registration ticket" -> "registration ticket patching".

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:70:
* GCM channel description and instance user-visible name.
nit: user-visible instance name

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:97:
* Delete registered instance (unregisters). If client has instance credentials
then
nit: Deletes a ...

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthClient.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthClient.java:79:
static OAuthResult readResponse(JsonReader reader, long startTime) throws
IOException {
It's better to specify units of time used. Otherwise, it's not clear, why do we
need to multiply by 1000 |expiresIn| but not |startTime|.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthResult.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthResult.java:13:
public final long expirationTime;
Same here.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java:8:
* Information what GCD provides when instance has registered. Instance id
nit: Information provided by GCD when ...

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java:9:
* could be used for:
nit: can be used

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:35:
* Fragment is abstract and not provide UI controls. Successor is responsible for
it.
nit: does not provide

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:35:
* Fragment is abstract and not provide UI controls. Successor is responsible for
it.
nit: Successor -> Descendant

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:36:
* It also should have actionable item for registration/unregistration which
invoke
nit: invokes

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:99:
showToast("Unregstered");
typo: Unregistered

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:104:
Log.d(TAG, "Unregistration failed", e);
Log.e perhaps? This seems like an error, not a debug message.

Also seen in a lot of places below.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:180:
Log.d(TAG, "Credential: id=" + mCredential.id + ", secret=" +
mCredential.secret);
Are you sure it is safe to log this information?

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/gcd/MessageReaderTest.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/gcd/MessageReaderTest.java:22:
"4/6V0jpup-OFTfJYyZ_dGKMIvv_MIM.4hQORMbRztoYJvIeHux6iLZT_e85kQI";
Is this data real? I don't think this test actually tries to access the cloud
service. Maybe, it is possible to use data that looks more artificial, e.g.
device id "11111111-2222-3333-...", email "robot@example.org", etc?

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/apiary/TestApiaryClientFactory.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/apiary/TestApiaryClientFactory.java:13:
return "AIzaSyAI1TKbOdqMQ5TltbBT15V5XaIILnDadhI";
Is it safe to have all this data in public access?

[SeRya, 2014-10-31 07:40:17]

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:51:
* Creation registration ticket if the first step in instance registration.
Client must have
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: The first sentence is incomprehensible. Should it be "Creation of a
> registration ticket is the first step..."?

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:52:
* user credentials. If it registered it will be associated with the user. Next
step is patching
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: Again, the sentence isn't very clear. Should it be "If the ticket has
been
> registered, it will be..."?

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:53:
* registration ticket.
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: "patching registration ticket" -> "registration ticket patching".

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:70:
* GCM channel description and instance user-visible name.
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: user-visible instance name

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/GCDClient.java:97:
* Delete registered instance (unregisters). If client has instance credentials
then
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: Deletes a ...

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthClient.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthClient.java:79:
static OAuthResult readResponse(JsonReader reader, long startTime) throws
IOException {
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> It's better to specify units of time used. Otherwise, it's not clear, why do
we
> need to multiply by 1000 |expiresIn| but not |startTime|.

Added unit suffixes.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthResult.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/OAuthResult.java:13:
public final long expirationTime;
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> Same here.

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java:8:
* Information what GCD provides when instance has registered. Instance id
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: Information provided by GCD when ...

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/gcd/InstanceCredential.java:9:
* could be used for:
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: can be used

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:35:
* Fragment is abstract and not provide UI controls. Successor is responsible for
it.
On 2014/10/30 18:18:22, mnaganov (cr) wrote:
> nit: Successor -> Descendant

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:35:
* Fragment is abstract and not provide UI controls. Successor is responsible for
it.
On 2014/10/30 18:18:22, mnaganov (cr) wrote:
> nit: does not provide

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:36:
* It also should have actionable item for registration/unregistration which
invoke
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> nit: invokes

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:99:
showToast("Unregstered");
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> typo: Unregistered

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:104:
Log.d(TAG, "Unregistration failed", e);
On 2014/10/30 18:18:21, mnaganov (cr) wrote:
> Log.e perhaps? This seems like an error, not a debug message.
> 
> Also seen in a lot of places below.

Done.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:180:
Log.d(TAG, "Credential: id=" + mCredential.id + ", secret=" +
mCredential.secret);
On 2014/10/30 18:18:22, mnaganov (cr) wrote:
> Are you sure it is safe to log this information?

I'd rather remove it.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/gcd/MessageReaderTest.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/android/javatests/src/org/chromium/components/devtools_bridge/gcd/MessageReaderTest.java:22:
"4/6V0jpup-OFTfJYyZ_dGKMIvv_MIM.4hQORMbRztoYJvIeHux6iLZT_e85kQI";
On 2014/10/30 18:18:22, mnaganov (cr) wrote:
> Is this data real? I don't think this test actually tries to access the cloud
> service. Maybe, it is possible to use data that looks more artificial, e.g.
> device id "11111111-2222-3333-...", email mailto:"robot@example.org", etc?

Data is real not not valid (the device unregistered). Data that look like real
data is self-descriptive. Some obfuscation is good.

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
File
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/apiary/TestApiaryClientFactory.java
(right):

https://codereview.chromium.org/677843006/diff/40001/components/devtools_brid...
components/devtools_bridge/test/android/javatests/src/org/chromium/components/devtools_bridge/apiary/TestApiaryClientFactory.java:13:
return "AIzaSyAI1TKbOdqMQ5TltbBT15V5XaIILnDadhI";
On 2014/10/30 18:18:22, mnaganov (cr) wrote:
> Is it safe to have all this data in public access?

I discussed it with @smok and he had no objections for using it in tests. In
production I will use regular way of dealing with API keys. Potentially it could
be used for DOS attach: exceeding quota for the project will prevent manual
testing. I don't it's a big risk for now.

[mnaganov (inactive), 2014-10-31 10:15:49]

LGTM % comments

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/ApiaryClientFactory.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/ApiaryClientFactory.java:51:
return "https://www.googleapis.com/auth/clouddevices";
Should this string be a constant?

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java:48:
private String mRregistrationId;
typo: mRegistrationId

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java:80:
Log.d(TAG, "Unexpected intent: " + intent);
Log.e ?

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:136:
Log.d(TAG, "Failed to get token: ", e);
Log.e ?

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:191:
Log.d(TAG, "Registration failed", e);
Log.e ?

[SeRya, 2014-10-31 12:07:05]

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/ApiaryClientFactory.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/ApiaryClientFactory.java:51:
return "https://www.googleapis.com/auth/clouddevices";
On 2014/10/31 10:15:49, mnaganov (cr) wrote:
> Should this string be a constant?

Done.

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java:48:
private String mRregistrationId;
On 2014/10/31 10:15:49, mnaganov (cr) wrote:
> typo: mRegistrationId

Done.

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/apiary/BlockingGCMRegistrar.java:80:
Log.d(TAG, "Unexpected intent: " + intent);
On 2014/10/31 10:15:49, mnaganov (cr) wrote:
> Log.e ?

Done.

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
File
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java
(right):

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:136:
Log.d(TAG, "Failed to get token: ", e);
On 2014/10/31 10:15:49, mnaganov (cr) wrote:
> Log.e ?

Done.

https://codereview.chromium.org/677843006/diff/60001/components/devtools_brid...
components/devtools_bridge/android/java/src/org/chromium/components/devtools_bridge/ui/GCDRegistrationFragment.java:191:
Log.d(TAG, "Registration failed", e);
On 2014/10/31 10:15:49, mnaganov (cr) wrote:
> Log.e ?

Done.

[SeRya, 2014-10-31 12:07:14]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-31 12:08:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/677843006/80001

[commit-bot: I haz the power, 2014-10-31 12:48:09]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-31 12:48:54]

Patchset 4 (id:??) landed as
https://crrev.com/a2da02bd99d78de549b617510551ec5e1f7073d9
Cr-Commit-Position: refs/heads/master@{#302251}

[Mike Lerman, 2014-10-31 13:02:24]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/693153003/ by mlerman@chromium.org.

The reason for reverting is: Only CL that landed before
http://build.chromium.org/p/chromium.mac/waterfall?builder=Mac%20Builder broke.
Going to revert this CL to try and get the builder working again..

[SeRya, 2014-10-31 14:20:50]

On 2014/10/31 13:02:24, Mike Lerman wrote:
> A revert of this CL (patchset #4 id:80001) has been created in
> https://codereview.chromium.org/693153003/ by mailto:mlerman@chromium.org.
> 
> The reason for reverting is: Only CL that landed before
> http://build.chromium.org/p/chromium.mac/waterfall?builder=Mac%20Builder
broke.
> Going to revert this CL to try and get the builder working again..

This CL is android-only and may not affect Mac builder. Going to reland.

[SeRya, 2014-10-31 14:20:59]

The CQ bit was checked by serya@chromium.org

[commit-bot: I haz the power, 2014-10-31 14:22:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/677843006/80001

[commit-bot: I haz the power, 2014-10-31 14:23:55]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-31 14:25:08]

Patchset 4 (id:??) landed as
https://crrev.com/ffb7ed195778fe9f7649da6b0f60b26f2cdefc4a
Cr-Commit-Position: refs/heads/master@{#302261}