Adding the Copresence RpcHandler and HttpPost helper.

components/copresence/rpc/http_post.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/copresence/rpc/http_post.h"
+
+#include <google/protobuf/message_lite.h>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "components/copresence/copresence_switches.h"
+#include "content/public/browser/browser_context.h"

[willchan no longer on Chromium, 2014/08/08 01:05:58]

Where is this used?

[Charlie, 2014/08/08 21:57:52]

It's outdated. Deleted.

+#include "google_apis/google_api_keys.h"
+#include "net/base/load_flags.h"
+#include "net/base/url_util.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+
+namespace copresence {
+
+namespace {
+
+const char kTracingTokenField[] = "trace";
+const char kApiKeyField[] = "key";
+
+}  // namespace
+
+HttpPost::HttpPost(net::URLRequestContextGetter* url_context_getter,
+                   const std::string& server_host,
+                   const std::string& rpc_name,
+                   const google::protobuf::MessageLite& request_proto,
+                   const ResponseCallback& response_callback)
+                   : response_callback_(response_callback) {
+  // Create the base URL to call.
+  GURL url(server_host + "/" + rpc_name);
+
+  // Add the Chrome API key.
+  // TODO(ckehoe): Replace this with an app-specific key
+  //               once the server API supports it.
+  DCHECK(google_apis::HasKeysConfigured());
+  url = net::AppendQueryParameter(url, kApiKeyField, google_apis::GetAPIKey());
+
+  // Add the tracing token, if specified.
+  CommandLine* command_line = CommandLine::ForCurrentProcess();

[willchan no longer on Chromium, 2014/08/08 01:05:58]

Only my personal preference (so you should feel free to ignore):
Please pass this in instead of accessing CommandLine. CommandLine is a global
variable. Generally speaking, it's bad to directly access global variables
instead of passing in the input. I'm a firm believer in only accessing command
line flags fairly close to main(), and then passing in the flag values to the
relevant code.

[rkc, 2014/08/08 01:29:07]

We'd have to access this global object 'somewhere'. I don't see anywhere this
can go where the decrease in the distance from main will be more than an
epsilon.

[willchan no longer on Chromium, 2014/08/08 18:01:20]

Replying just to your point, but again, it's just a personal preference, so
consider it non-blocking for this review. I think even an epsilon is important.
For example, let's say we have a bunch of factory employees. The manager tells
them that lunch time begins when the sun is at its highest point in the sky.

Now, the sun and the sky is part of the global environment. Do the workers
really care about the sun and the sky here? No, they just need to know when it's
time for lunch. And if the sky is cloudy, it's silly for all of them not to get
lunch. Instead of having a global dependency, the manager should just tell the
workers when lunch time is, or scope it to something like a clock. This also
makes it more testable, since managers can change when they announce lunch time,
or can change the time on the clock. It's much easier to do that, rather than
change the position of the sun in the sky.

It's true, it might be a small epsilon. Maybe the managers themselves decide
lunch time based on the position of the sun in the sky. But at least all the
workers don't have to. That's still a win. Enforcing good encapsulation
boundaries and explicitly passing dependencies is good code hygiene and we
should encourage this.

[Charlie, 2014/08/08 21:57:52]

I don't get your argument. Why should main() "own" the flags? This flag doesn't,
and shouldn't, concern anyone but copresence. Keeping it localized here seems
like better encapsulation to me.

There will always be global dependencies. Most of Chrome can't function without
the base:: and net:: packages. What we don't want to do is to store state in
them when that state should belong to us (think strtok vs. strtok_r). But we
aren't changing the flag values. The user happens to have just one command line
on which to put flags, and this is the way we pull out our piece.

All of that said: you wanted a test for this, so it has to be injectable. So
RpcHandler will inject it (from the command line). There's your epsilon.

[willchan no longer on Chromium, 2014/08/08 22:31:01]

But the point is that it's not localized :) The concept of a command line is an
application level concept, shared by the entire application. For example, what
if your code is embedded in a non-browser application that uses command line
flags different, perhaps via libcef
(https://code.google.com/p/chromiumembedded/)? Or what if we want to let an
enterprise group policy override your setting? Or what if we wanted to configure
this via Finch?

Basically, you're saying that your configuration comes from page 25 line 10 of
this magic book of instructions. If anyone wants to configure you, they should
go find this magic book (the base::CommandLine object), open it up to page 25
(the name of the flag), and rewrite line 10 (which is where the flag value is
stored). And I'm saying, why do we need a whole book just to give you a single
instruction?

Recall the whole multiprofile and componentization project. Folks thought for
the longest time that they would always have access to a single Profile object,
and always pulled their data out from that global Profile*. Well, it turns out
in some new Chromium ports, we may have multiple profiles, and sometimes we
might not have Profiles at all. So we had to have a massive refactoring project
to get rid of Profile* usage across large chunks of the code base.

Lesson learned: stop reading from global variables (like the Profile* returned
by GetDefaultProfile()) with large scopes. You should explicitly pass your
parameters.
That's definitely an improvement, and I think it still can be improved :) And as
I said before, this is non-blocking, so go ahead and ignore it. But I hope that
you will see why this is suboptimal.

+  if (command_line->HasSwitch(switches::kCopresenceTracingToken)) {
+    url = net::AppendQueryParameter(

[willchan no longer on Chromium, 2014/08/08 18:01:20]

I don't see a unit test for this. Can you add one?

[Charlie, 2014/08/08 21:57:52]

Done.

+        url,
+        kTracingTokenField,
+        "token:" + command_line->GetSwitchValueASCII(
+            switches::kCopresenceTracingToken));
+  }
+
+  // Serialize the proto for transmission.
+  std::string request_data;
+  DCHECK(request_proto.SerializeToString(&request_data));
+
+  // Configure and send the request.
+  url_fetcher_.reset(net::URLFetcher::Create(
+      kUrlFetcherId, url, net::URLFetcher::POST, this));
+  url_fetcher_->SetRequestContext(url_context_getter);
+  url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE |
+                             net::LOAD_DISABLE_CACHE |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
+  url_fetcher_->SetUploadData("application/x-protobuf", request_data);
+  url_fetcher_->Start();
+}
+
+HttpPost::~HttpPost() {}
+
+void HttpPost::Cancel() {
+  // This deletes the url_fetcher_, which aborts the request.
+  delete this;
+}
+
+void HttpPost::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(url_fetcher_.get(), source);
+
+  // Gather response info.
+  std::string response;
+  source->GetResponseAsString(&response);
+  int response_code = source->GetResponseCode();
+
+  // Log any errors.
+  if (response_code < 0) {
+    LOG(ERROR) << "Couldn't contact the Copresence server at "
+               << source->GetURL();
+  } else if (response_code != net::HTTP_OK) {
+    LOG(ERROR) << "Copresence request got HTTP response code " << response_code
+               << ". Response:\n" << response;
+  }
+
+  // Return the response.
+  response_callback_.Run(response_code, response, this);
+  delete this;
+}
+
+}  // namespace copresence