Adds EngineGeolocationFeature for Blimp Geolocation project.

blimp/engine/feature/geolocation/engine_geolocation_feature.cc

Index: blimp/engine/feature/geolocation/engine_geolocation_feature.cc
diff --git a/blimp/engine/feature/geolocation/engine_geolocation_feature.cc b/blimp/engine/feature/geolocation/engine_geolocation_feature.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cd824e05223a57216b0755ba6bfe175e1b7c2863
--- /dev/null
+++ b/blimp/engine/feature/geolocation/engine_geolocation_feature.cc
@@ -0,0 +1,150 @@
+// Copyright 2016 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 "blimp/engine/feature/geolocation/engine_geolocation_feature.h"
+
+#include "base/memory/weak_ptr.h"
+#include "blimp/common/create_blimp_message.h"
+#include "blimp/common/proto/blimp_message.pb.h"
+#include "blimp/common/proto/geolocation.pb.h"
+#include "content/public/browser/location_provider.h"
+#include "content/public/common/geoposition.h"
+#include "net/base/net_errors.h"
+
+namespace blimp {
+namespace engine {
+namespace {
+
+content::Geoposition::ErrorCode GetErrorCode(

[Wez, 2016/07/12 21:35:14]

nit: why is this GetErrorCode but the next method is ConvertLocationMessage?

[CJ, 2016/07/13 21:49:20]

Done.

+    const ErrorMessage::ErrorCode& error_code) {
+  switch (error_code) {
+    case ErrorMessage::ERROR_CODE_NONE:
+      return content::Geoposition::ErrorCode::ERROR_CODE_NONE;
+    case ErrorMessage::ERROR_CODE_PERMISSION_DENIED:
+      return content::Geoposition::ErrorCode::ERROR_CODE_PERMISSION_DENIED;
+    case ErrorMessage::ERROR_CODE_POSITION_UNAVAILABLE:
+      return content::Geoposition::ErrorCode::ERROR_CODE_POSITION_UNAVAILABLE;
+    case ErrorMessage::ERROR_CODE_TIMEOUT:
+      return content::Geoposition::ErrorCode::ERROR_CODE_TIMEOUT;
+  }
+}
+
+void ConvertLocationMessage(content::Geoposition* output,
+                            const LocationMessage& location) {

[Wez, 2016/07/12 21:35:13]

nit: Any reason this can't be:

Geoposition ConvertLocationMessage(LocationMessage)

?

[CJ, 2016/07/13 21:49:20]

Done.

+  output->latitude = location.latitude();
+  output->longitude = location.longitude();
+  output->altitude = location.altitude();
+  output->accuracy = location.accuracy();
+  output->altitude_accuracy = location.altitude_accuracy();
+  output->heading = location.heading();
+  output->speed = location.speed();
+  output->timestamp = base::Time::FromJsTime(location.timestamp_millis());
+}
+
+}  // namespace
+
+EngineGeolocationFeature::EngineGeolocationFeature() : weak_factory_(this) {}
+
+EngineGeolocationFeature::~EngineGeolocationFeature() {}
+
+void EngineGeolocationFeature::set_outgoing_message_processor(
+    std::unique_ptr<BlimpMessageProcessor> message_processor) {
+  DCHECK(message_processor);
+  outgoing_message_processor_ = std::move(message_processor);
+}
+
+base::WeakPtr<EngineGeolocationFeature> EngineGeolocationFeature::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void EngineGeolocationFeature::UpdateListenState(bool enable_high_accuracy) {
+  GeolocationMessage* geolocation_message;
+  std::unique_ptr<BlimpMessage> blimp_message =
+      CreateBlimpMessage(&geolocation_message);
+
+  UpdateListenStateMessage* details =

[Wez, 2016/07/12 21:35:13]

nit: Name the variable after the field in the message, for clarity.

[CJ, 2016/07/13 21:49:21]

Done.

+      geolocation_message->mutable_update_listen_state();
+  if (enable_high_accuracy) {
+    details->set_listen_state(UpdateListenStateMessage::ACCURACY_HIGH);
+  } else {
+    details->set_listen_state(UpdateListenStateMessage::ACCURACY_LOW);
+  }
+
+  outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
+                                              net::CompletionCallback());
+}
+
+void EngineGeolocationFeature::StopListenState() {
+  CHECK(false);

[Wez, 2016/07/12 21:35:14]

This should break your tests, right? But the try-bots are green, so are your
tests not running?

[CJ, 2016/07/13 21:49:21]

Fixed. 

+  GeolocationMessage* geolocation_message;

[Wez, 2016/07/12 21:35:14]

nit: Here and elsewhere, initialize this to nullptr, in case CreateBlimpMessage
is buggy and doesn't.

[CJ, 2016/07/13 21:49:21]

Done.

+  std::unique_ptr<BlimpMessage> blimp_message =
+      CreateBlimpMessage(&geolocation_message);
+
+  UpdateListenStateMessage* details =
+      geolocation_message->mutable_update_listen_state();
+  details->set_listen_state(UpdateListenStateMessage::STOPPED);
+
+  outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
+                                              net::CompletionCallback());
+}
+
+void EngineGeolocationFeature::RequestRefresh() {
+  GeolocationMessage* geolocation_message;
+  std::unique_ptr<BlimpMessage> blimp_message =
+      CreateBlimpMessage(&geolocation_message);
+
+  geolocation_message->mutable_request_refresh();
+
+  outgoing_message_processor_->ProcessMessage(std::move(blimp_message),
+                                              net::CompletionCallback());
+}
+
+void EngineGeolocationFeature::SetUpdateCallback(
+    const base::Callback<void(const content::Geoposition&)>& callback) {
+  DCHECK(!callback.is_null());

[Wez, 2016/07/12 21:35:13]

See BLP comment - I don't think this check (here and elsewhere) is valid?

[CJ, 2016/07/13 21:49:20]

Done.

+
+  callback_ = callback;
+}
+
+void EngineGeolocationFeature::NotifyCallback(
+    const content::Geoposition& position) {

[Wez, 2016/07/12 21:35:13]

Why do you need this method to be part of the Delegate interface? Isn't it just
an implementation detail of this class?

[CJ, 2016/07/13 21:49:20]

Done.

+  callback_.Run(position);
+}
+
+void EngineGeolocationFeature::ProcessMessage(
+    std::unique_ptr<BlimpMessage> message,
+    const net::CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
+  DCHECK_EQ(BlimpMessage::kGeolocation, message->feature_case());
+
+  const GeolocationMessage& geolocation_message = message->geolocation();
+  content::Geoposition output;

[Wez, 2016/07/12 21:35:14]

You only need output in case of kLocation and kError, so define it there, not
here.

[CJ, 2016/07/13 21:49:20]

Done.

+
+  switch (geolocation_message.type_case()) {
+    case GeolocationMessage::kLocation: {
+      const LocationMessage location = geolocation_message.location();
+      ConvertLocationMessage(&output, location);
+      NotifyCallback(output);
+      break;
+    }
+    case GeolocationMessage::kError: {
+      const ErrorMessage error_message = geolocation_message.error();
+      output.error_message = error_message.error_message();
+      output.error_code = GetErrorCode(error_message.error_code());
+      NotifyCallback(output);
+      break;
+    }
+    case GeolocationMessage::kUpdateListenState:
+    case GeolocationMessage::kRequestRefresh:
+      NOTREACHED() << "Engine received unexpected geolocation type.";

[Wez, 2016/07/12 21:35:14]

NOTREACHED() means "this line of code will never ever be executed", which is
stronger than you intend, I think; you probably want DLOG(FATAL) << "Client sent
unexpected message type:" << type

(DLOG(FATAL) will compile to the same as NOTREACHED, I think, but IMO it more
clearly expresses the intent here - this is a protocol error, not a coding
error, basically)

[CJ, 2016/07/13 21:49:20]

Done.

+      break;
+    case GeolocationMessage::TYPE_NOT_SET:

[Wez, 2016/07/12 21:35:13]

nit: You can combine this with the two other unexpected cases, above.

[CJ, 2016/07/13 21:49:20]

Done.

+      NOTREACHED();
+      break;
+  }
+  callback.Run(net::OK);
+}
+
+}  // namespace engine
+}  // namespace blimp