Add PhysicalWebBroadcastService

chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java

+// Copyright 2017 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.
+
+package org.chromium.chrome.browser.physicalweb;
+
+import android.annotation.TargetApi;
+import android.app.Service;
+import android.content.Intent;
+import android.os.Build;
+import android.os.IBinder;
+
+/**
+ * Broadcasts Physical Web URLs via BLE.
+ **/
+// bluetooth.le.AdvertiseCallback and bluetooth.BluetoothAdapter require API level 21.

[nyquist, 2017/03/06 22:34:13]

Why is this a separate style of comment than the javadoc-one?

[iankc, 2017/03/07 02:10:39]

Done.

+// This will only be run on M and above.
+@TargetApi(Build.VERSION_CODES.LOLLIPOP)
+public class PhysicalWebBroadcastService extends Service {

[nyquist, 2017/03/06 22:34:13]

If there is an upper limit for how long time this will run, did you consider
just running as a background task instead? I think the correct approach is a
foreground service though.

[iankc, 2017/03/07 02:10:39]

There is no upper limit for how long it will be running. The user will have to
terminate broadcasting through a broadcast notification.

+    public static final String DISPLAY_URL_KEY = "display_url";
+
+    @Override
+    public int onStartCommand(Intent intent, int flags, int startId) {

[nyquist, 2017/03/06 22:34:13]

Will this be a foreground service?

[iankc, 2017/03/07 02:10:39]

This is not a foreground service. It is a background Service. However, the user
can stop the service through the notification.

+        String displayUrl = intent.getStringExtra(DISPLAY_URL_KEY);

[nyquist, 2017/03/06 22:34:13]

This will crash if there are no pending start-commands in the case of a restart
of the service after it being killed.

[iankc, 2017/03/07 02:10:39]

Done.

+        // This should never happen.

[nyquist, 2017/03/06 22:34:13]

Can you add a newline before this line?

[iankc, 2017/03/07 02:10:39]

Done.

+        if (displayUrl == null) {
+            stopSelf();
+            return START_STICKY;
+        }
+        // TODO(iankc): implement parsing, broadcasting, and notifications.

[nyquist, 2017/03/06 22:34:13]

Can you add a newline before this line to separate it from the edge-case above?

[iankc, 2017/03/07 02:10:39]

Done.

+        stopSelf();
+        return START_STICKY;

[nyquist, 2017/03/06 22:34:13]

Can you expand the JavaDoc for the whole class quite a bit documenting the
behavior?

- Expand a lot around the lifetime of this service?
- When will it be started?
- For how long will it run? If the user stops interacting with the device, will
there be a timeout?
- Is there a way to stop the publishing?
- Will there be an ongoing foreground notification?
- If the app crashes / is killed at some point; does the broadcast restart? What
do you do with the notifications in this case?
- How do you guarantee that this stays alive? (i.e. do you plan on doing
something with wakelocks, etc.)

The design doc is not publicly available from an @chromium.org for example,
which means all of the information related to this needs to go into the code as
comments instead.

[iankc, 2017/03/07 02:10:39]

I feel like I did all of this besides the guarantee that it stays alive. It
stays alive because the only way to terminate broadcasting is through the cancel
button on the notification. If the OS kills it then it will have to call
onStartCommand which will make it start broadcasting. If the OS kills it, it
won't be dead for long, and it will be highly improbable that the user tries to
kill the service during that timeframe. It will be seemless to the user, as if
nothing happened.

+    }
+
+    @Override
+    public IBinder onBind(Intent intent) {
+        return null;
+    }
+}