Using API key specified from js

Using API key specified from js

BUG=400617
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289511

[Charlie, 2014-08-13 20:20:13]

This is a pretty simple passthrough of one API parameter into the correct proto
in the server request. I refactored the HttpPost class a bit along the way.

abarth for url/ owners review
rkc & xiyuan for general review

[xiyuan, 2014-08-13 20:30:13]

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
File components/copresence/rpc/http_post.cc (right):

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
components/copresence/rpc/http_post.cc:48: api_key_ = google_apis::GetAPIKey();
nit: Don't indent inside #iddef here and below.

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
File components/copresence/rpc/http_post.h (right):

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
components/copresence/rpc/http_post.h:48: const google::protobuf::MessageLite*
request_proto);
Can you pass |request_proto| in Start() and get rid of the member
|request_proto_|?

[Charlie, 2014-08-13 20:39:20]

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
File components/copresence/rpc/http_post.cc (right):

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
components/copresence/rpc/http_post.cc:48: api_key_ = google_apis::GetAPIKey();
On 2014/08/13 20:30:13, xiyuan wrote:
> nit: Don't indent inside #iddef here and below.

At all? Looks weird, but ok.

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
File components/copresence/rpc/http_post.h (right):

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
components/copresence/rpc/http_post.h:48: const google::protobuf::MessageLite*
request_proto);
On 2014/08/13 20:30:13, xiyuan wrote:
> Can you pass |request_proto| in Start() and get rid of the member
> |request_proto_|?

Yeah I guess that's better. Done.

[abarth-chromium, 2014-08-13 20:44:05]

https://codereview.chromium.org/469883002/diff/20001/components/copresence/DEPS
File components/copresence/DEPS (right):

https://codereview.chromium.org/469883002/diff/20001/components/copresence/DE...
components/copresence/DEPS:8: "+url",
LGTM

[xiyuan, 2014-08-13 20:44:32]

lgtm

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
File components/copresence/rpc/http_post.cc (right):

https://codereview.chromium.org/469883002/diff/1/components/copresence/rpc/ht...
components/copresence/rpc/http_post.cc:48: api_key_ = google_apis::GetAPIKey();
On 2014/08/13 20:39:20, Charlie wrote:
> On 2014/08/13 20:30:13, xiyuan wrote:
> > nit: Don't indent inside #iddef here and below.
> 
> At all? Looks weird, but ok.

My bad. Please restore as PS1. I missed the "if" above it. :p

[Charlie, 2014-08-13 21:06:49]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-08-13 21:11:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckehoe@chromium.org/469883002/80001

[rkc, 2014-08-13 21:33:18]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2014-08-13 21:45:31]

https://codereview.chromium.org/469883002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/469883002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:174: api_key_ =
params->apiKey;
Um? api_key_ is declared in CopresenceService, you're setting it in
CopresenceSetApiKeyFunction. This code shouldn't even compile.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
File components/copresence/public/copresence_client_delegate.h (right):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
components/copresence/public/copresence_client_delegate.h:39: virtual const
DeviceFingerprint& GetDeviceFingerprint() const = 0;
This proto can be constructed on the Copresence side without requiring an
explicit API. If it at a later point it does need any data from the consumer,
let us get that data, not the proto itself. This API should not introduce any
dependencies on internal Copresence code.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/http_post.h (left):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/http_post.h:51: const std::string& tracing_token,
Why is this taken out of the constructor?

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:573:
http_post->set_api_key(delegate_->GetAPIKey());
Why are we not just passing the API key in the constructor? If we don't have an
API, we can pass in an empty string (which the post checks for anyway).

[Charlie, 2014-08-13 22:53:38]

https://codereview.chromium.org/469883002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence/copresence_api.cc (right):

https://codereview.chromium.org/469883002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/copresence/copresence_api.cc:174: api_key_ =
params->apiKey;
On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> Um? api_key_ is declared in CopresenceService, you're setting it in
> CopresenceSetApiKeyFunction. This code shouldn't even compile.

It doesn't. I compiled the component, but not the API. Fixed.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
File components/copresence/public/copresence_client_delegate.h (right):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
components/copresence/public/copresence_client_delegate.h:39: virtual const
DeviceFingerprint& GetDeviceFingerprint() const = 0;
On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> This proto can be constructed on the Copresence side without requiring an
> explicit API. If it at a later point it does need any data from the consumer,
> let us get that data, not the proto itself. This API should not introduce any
> dependencies on internal Copresence code.

The client already depends on our protos, because the only thing it does is to
execute a ReportRequest. In fact maybe the protos should be in the public/
directory. Anyway, it will be easier for users of the component (e.g. from
Brillo) to set this correctly if it is exposed.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/http_post.h (left):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/http_post.h:51: const std::string& tracing_token,
On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> Why is this taken out of the constructor?

All parameters to the constructor are now required. If clients want to add a
tracing token, they should use a separate mutator.

The request proto has been taken out as part of the refactoring, at xiyuan's
suggestion. I can put it back in, but I don't think it's needed.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:573:
http_post->set_api_key(delegate_->GetAPIKey());
On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> Why are we not just passing the API key in the constructor? If we don't have
an
> API, we can pass in an empty string (which the post checks for anyway).

See previous comment. For Chrome, the API key is not required.

[rkc, 2014-08-13 23:11:37]

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
File components/copresence/public/copresence_client_delegate.h (right):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
components/copresence/public/copresence_client_delegate.h:39: virtual const
DeviceFingerprint& GetDeviceFingerprint() const = 0;
On 2014/08/13 22:53:37, Charlie wrote:
> On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> > This proto can be constructed on the Copresence side without requiring an
> > explicit API. If it at a later point it does need any data from the
consumer,
> > let us get that data, not the proto itself. This API should not introduce
any
> > dependencies on internal Copresence code.
> 
> The client already depends on our protos, because the only thing it does is to
> execute a ReportRequest. In fact maybe the protos should be in the public/
> directory. Anyway, it will be easier for users of the component (e.g. from
> Brillo) to set this correctly if it is exposed.

Report request is is required for every report to exist. DeviceFingerprint is a
technical detail that we didn't even need to set till now. Reusing a proto
instead of creating another in-between translation API for operations makes
sense. Having a client provide a technical detail of our API isn't something we
should get into.

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/http_post.h (left):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/http_post.h:51: const std::string& tracing_token,
On 2014/08/13 22:53:37, Charlie wrote:
> On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> > Why is this taken out of the constructor?
> 
> All parameters to the constructor are now required. If clients want to add a
> tracing token, they should use a separate mutator.
> 
> The request proto has been taken out as part of the refactoring, at xiyuan's
> suggestion. I can put it back in, but I don't think it's needed.

I am not happy about using mutators instead of passing arguments through the
constructor. In general, the internal details of a class should be abstracted
out and anything it needs for its operation should be given to the constructor.

In case this class decides to not save the API key but rather to simply setup
the URL Request in the constructor, now any code that is setting the API key
would now be non-functional. The same argument goes for the tracing token.

In fact, a majority of the implementation had to be moved to the Start method
because we actually now need this space between construction and 'real'
construction where a consumer of this class can set these values.

We do an empty string check on both values anyway (the consumer can just pass in
an empty string to the constructor for those values). I guess I am seeing cons
but I am not seeing any pros of having this as a mutator.

[xiyuan, 2014-08-13 23:15:59]

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
File components/copresence/rpc/http_post.h (left):

https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
components/copresence/rpc/http_post.h:51: const std::string& tracing_token,
On 2014/08/13 23:11:36, Rahul Chaturvedi wrote:
> On 2014/08/13 22:53:37, Charlie wrote:
> > On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> > > Why is this taken out of the constructor?
> > 
> > All parameters to the constructor are now required. If clients want to add a
> > tracing token, they should use a separate mutator.
> > 
> > The request proto has been taken out as part of the refactoring, at xiyuan's
> > suggestion. I can put it back in, but I don't think it's needed.
> 
> I am not happy about using mutators instead of passing arguments through the
> constructor. In general, the internal details of a class should be abstracted
> out and anything it needs for its operation should be given to the
constructor.
> 
> In case this class decides to not save the API key but rather to simply setup
> the URL Request in the constructor, now any code that is setting the API key
> would now be non-functional. The same argument goes for the tracing token.
> 
> In fact, a majority of the implementation had to be moved to the Start method
> because we actually now need this space between construction and 'real'
> construction where a consumer of this class can set these values.
> 
> We do an empty string check on both values anyway (the consumer can just pass
in
> an empty string to the constructor for those values). I guess I am seeing cons
> but I am not seeing any pros of having this as a mutator.

If we decided to add back |request_proto| to ctor, I would prefer to extract
info out of it instead of storing the pointer, e.g. store the serialized request
data.

[Charlie, 2014-08-13 23:16:52]

On 2014/08/13 23:11:37, Rahul Chaturvedi wrote:
>
https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
> File components/copresence/public/copresence_client_delegate.h (right):
> 
>
https://codereview.chromium.org/469883002/diff/80001/components/copresence/pu...
> components/copresence/public/copresence_client_delegate.h:39: virtual const
> DeviceFingerprint& GetDeviceFingerprint() const = 0;
> On 2014/08/13 22:53:37, Charlie wrote:
> > On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> > > This proto can be constructed on the Copresence side without requiring an
> > > explicit API. If it at a later point it does need any data from the
> consumer,
> > > let us get that data, not the proto itself. This API should not introduce
> any
> > > dependencies on internal Copresence code.
> > 
> > The client already depends on our protos, because the only thing it does is
to
> > execute a ReportRequest. In fact maybe the protos should be in the public/
> > directory. Anyway, it will be easier for users of the component (e.g. from
> > Brillo) to set this correctly if it is exposed.
> 
> Report request is is required for every report to exist. DeviceFingerprint is
a
> technical detail that we didn't even need to set till now. Reusing a proto
> instead of creating another in-between translation API for operations makes
> sense. Having a client provide a technical detail of our API isn't something
we
> should get into.
> 
>
https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
> File components/copresence/rpc/http_post.h (left):
> 
>
https://codereview.chromium.org/469883002/diff/80001/components/copresence/rp...
> components/copresence/rpc/http_post.h:51: const std::string& tracing_token,
> On 2014/08/13 22:53:37, Charlie wrote:
> > On 2014/08/13 21:45:31, Rahul Chaturvedi wrote:
> > > Why is this taken out of the constructor?
> > 
> > All parameters to the constructor are now required. If clients want to add a
> > tracing token, they should use a separate mutator.
> > 
> > The request proto has been taken out as part of the refactoring, at xiyuan's
> > suggestion. I can put it back in, but I don't think it's needed.
> 
> I am not happy about using mutators instead of passing arguments through the
> constructor. In general, the internal details of a class should be abstracted
> out and anything it needs for its operation should be given to the
constructor.
> 
> In case this class decides to not save the API key but rather to simply setup
> the URL Request in the constructor, now any code that is setting the API key
> would now be non-functional. The same argument goes for the tracing token.
> 
> In fact, a majority of the implementation had to be moved to the Start method
> because we actually now need this space between construction and 'real'
> construction where a consumer of this class can set these values.
> 
> We do an empty string check on both values anyway (the consumer can just pass
in
> an empty string to the constructor for those values). I guess I am seeing cons
> but I am not seeing any pros of having this as a mutator.

I'm not really following what you're saying on either point. When is a good time
to VC about this?

[rkc, 2014-08-13 23:58:34]

Discussed offline, Charlie is working on the changes.
I won't be on again now for some time, otherwise everything else looks fine, so
lgtm.

[Charlie, 2014-08-14 00:19:17]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2014-08-14 00:23:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckehoe@chromium.org/469883002/140001

[commit-bot: I haz the power, 2014-08-14 06:30:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

Committed patchset #8 (140001) as 289511