Adds EngineGeolocationFeature for Blimp Geolocation project.

blimp/common/proto/geolocation.proto

+// 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.
+//
+// Message definitions for geolocation messages.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+import "common.proto";
+
+package blimp;
+
+message ErrorMessage {

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

Remember that message names are scoped to the package, so you're creating a
blimp::ErrorMessage - there's nothing in that name to tie this to Geolocation.

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

Done.

+  // These values follow the W3C geolocation specification and can be returned
+  // to JavaScript without the need for a conversion.

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

The Blink interface for PositionError (see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/geoloc...)
has an enum that includes only the three valid error codes, though, and
instances can only be created with the enum, so we definitely need a conversion.

If you're arguing that we can static_cast<> then that's technically true, but we
still shouldn't - we're receiving this data across the network, so we should
treat it as "untrusted" and sanity-check all the values that we receive.

In the case of enums simply casting to a C++ enum from an integer has undefined
behaviour if the integer doesn't have one of the enums valid integer values.
e.g. if you have:

enum FooBar { FOO = 1, BAR = 2 }

bool foo(int wibble) {
  switch (static_cast<FooBar>(wibble)) {
  case FOO: return true;
  case BAR: return false;
  }
}

and then call foo(5) then the compiler is at liberty to create a program that
returns a random bool value, or even crashes.

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

Removed comment.

+  // https://dev.w3.org/geo/api/spec-source.html - 5.5 PositionError interface
+  enum ErrorCode {
+    ERROR_CODE_NONE = 0;

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

Do we need NONE as an error enum value? Surely by definition if you're receiving
an ErrorMessage then there was an error?

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

Sure. Good point. Since I'm making a different message for Location anyway,
Error_CODE_NONE would never show up. 

+    ERROR_CODE_PERMISSION_DENIED = 1;
+    ERROR_CODE_POSITION_UNAVAILABLE = 2;
+    ERROR_CODE_TIMEOUT = 3;
+  }
+
+  optional ErrorCode error_code = 1;
+  optional string error_message = 2;
+}
+
+message LocationMessage {

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

nit: We should use either Location or Geolocation consistently; the latter is
more precise so I'd suggest using that for the proto messages (where we're
overriding interfaces with LocationFoo naming, we should be consistent with the
interface though :-/ )

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

Done. 

+  // Refer to content/public/common/geoposition.h for explanation of

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

geoposition.h is part of the Chromium-specific Content-layer implementation of
this stuff - it's implementation-specific.

I'd recommend referring instead to the Geolocation API specification, e.g. to
the Position and Coordinates interfaces.  I'd also suggest actually mirroring
those interfaces in the protos, so that this would become:

message GeolocationPositionMessage {
  GeolocationCoordinates coords;

  // Need a comment to explain what this is relative to, since Client & Engine
have different clocks - do we need to do some kind of adaptation at the Engine
to convert to DOMTimestamp..?
  int64 timestamp_millis;
}

 

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

Added GeolocationCoordinates. 

As for the comment, done.

There probably should be something. As for this field, would it better serve to
have it as double seconds?

+  // these fields and associated units.
+  optional double latitude = 1;
+  optional double longitude = 2;
+  optional double altitude = 3;
+  optional double accuracy = 4;
+  optional double altitude_accuracy = 5;
+  optional double heading = 6;
+  optional double speed = 7;
+  optional int64 timestamp_millis = 8;
+}
+
+message UpdateListenStateMessage {

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

nit: Suggest GeolocationRequestMessage, GeolocationInterestMessage or
GeolocationListenerMessage and tweaking the enum to have values like
REQUEST_NONE/HIGH/LOW.

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

Done.

+  // These values represent the various listening states the server can have.
+  // A ListenState containing an accuracy level indicates that the server is
+  // waiting for either high or low accuracy position updates from the client.
+  // If a STOPPED listen state is sent, the server is no longer listening for
+  // updates.
+  enum ListenState {
+    STOPPED = 0;
+    ACCURACY_HIGH = 1;
+    ACCURACY_LOW = 2;
+  }
+
+  optional ListenState listen_state = 1;
+}
+
+message GeolocationMessage {
+  oneof type {
+    // Server => Client types.
+    UpdateListenStateMessage update_listen_state = 1;
+    EmptyMessage request_refresh = 2;
+
+    // Client => Server types.
+    LocationMessage location = 3;
+    ErrorMessage error = 4;
+  }
+}