Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2719493003:
Add PhysicalWebBroadcastService (Closed)
DescriptionAdd PhysicalWebBroadcastService
This change creates the skeleton for the PhysicalWebBroadcastService, which will be the service that handles the Physical Web Sharing Feature.
Here is a background to what the Service will do:
This background service will start when the user activates the Physical Web Sharing item in the sharing chooser intent. If the URL received from the triggering Tab is validated by the Physical Web Service then it will encode the URL into an Eddystone URL and broadcast the URL through BLE.
To notify and provide the user with the ability to terminate broadcasting a persistent notification with a cancel button will be created. This cancel button will stop the service and will be the only way for the user to stop the service.
If the user terminates Chrome or the OS shuts down this service
mid-broadcasting, when the service get's restarted it will restart broadcasting as well by gathering the URL from shared preferences. This will create a seemless experience to the user by behaving as though broadcasting is not connected to the Chrome application.
BUG=685856
Review-Url: https://codereview.chromium.org/2719493003
Cr-Commit-Position: refs/heads/master@{#455211}
Committed: https://chromium.googlesource.com/chromium/src/+/f2ef57e81267138c5f8fabc2ea7044b59600238a
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixing nits #
Total comments: 1
Patch Set 3 : Making Change for Previous land of ShareActivity #
Total comments: 6
Patch Set 4 : Always Stopping Self #
Total comments: 1
Patch Set 5 : Adding TODO #Patch Set 6 : rebasing with master #
Total comments: 25
Patch Set 7 : Improving Comments #
Total comments: 16
Patch Set 8 : Tommys Nits 1 #Patch Set 9 : Updating Commit Message #
Messages
Total messages: 26 (7 generated)
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@chromium.org
This is just the entry for the PhysicalWebBroadcastService. Getting this template in would help a lot with multitasking on it.
https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:650: <!-- Service for PhysicalWebBroadcasting --> for Broadcasting a Physical Web URL https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:24: private static final String TAG = "PhysicalWeb"; This tag isn't needed yet. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:26: @Override Remove this method https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:34: if (displayUrl == null) { Add a note that this should never happen. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:43: Log.v(TAG, "On Destroy"); Remove this log https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:59: protected ChromeActivity getTriggeringActivity() { If we have to add code like this, I'd prefer you make a change to create an abstract ShareActivity first.
Because Ted doesn't see the need for the abstract class now, there is a little duplicated code (getTriggeringActivity) that exists within the printShareActivity as well. I think that we can put this in and then make the case for abstraction later. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/Android... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/Android... chrome/android/java/AndroidManifest.xml:650: <!-- Service for PhysicalWebBroadcasting --> On 2017/02/25 00:09:25, cco3 wrote: > for Broadcasting a Physical Web URL Done. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:24: private static final String TAG = "PhysicalWeb"; On 2017/02/25 00:09:26, cco3 wrote: > This tag isn't needed yet. Done. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:26: @Override On 2017/02/25 00:09:25, cco3 wrote: > Remove this method Done. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:34: if (displayUrl == null) { On 2017/02/25 00:09:25, cco3 wrote: > Add a note that this should never happen. Done. https://codereview.chromium.org/2719493003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:43: Log.v(TAG, "On Destroy"); On 2017/02/25 00:09:25, cco3 wrote: > Remove this log Done.
https://codereview.chromium.org/2719493003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:65: protected ChromeActivity getTriggeringActivity() { Since we don't have an abstract class, could we turn this into a static method that belongs to ShareHelper.java (or one of those classes) and takes an Intent?
On 2017/03/02 19:01:08, cco3 wrote: > https://codereview.chromium.org/2719493003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java > (right): > > https://codereview.chromium.org/2719493003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:65: > protected ChromeActivity getTriggeringActivity() { > Since we don't have an abstract class, could we turn this into a static method > that belongs to ShareHelper.java (or one of those classes) and takes an Intent? nm, ignore this.
The ShareActivity Landed so here is the updated PhysicalWebShareActivity that launches the PhysicalWebBroadcastService. This should be the final PhysicalWebShareActivity, all other changes will go into the service!
https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:30: return START_STICKY; go ahead and stopself() here and leave a TODO https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:34: public void onDestroy() { Get rid of this https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:27: * Pre-conditions for Physical Web Sharing to be enabled: It be good to have a summary line before this: "Returns whether we may show this sharing option in the share sheet."
Now it immediately turns off the service. https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:30: return START_STICKY; On 2017/03/02 23:46:07, cco3 wrote: > go ahead and stopself() here and leave a TODO Done. https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:34: public void onDestroy() { On 2017/03/02 23:46:07, cco3 wrote: > Get rid of this Done. https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:27: * Pre-conditions for Physical Web Sharing to be enabled: On 2017/03/02 23:46:07, cco3 wrote: > It be good to have a summary line before this: > "Returns whether we may show this sharing option in the share sheet." Done.
https://codereview.chromium.org/2719493003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:30: stopSelf(); Add a TODO
added todo.
lgtm
iankc@google.com changed reviewers: + nyquist@chromium.org
Hey Tommy, Here is a change that adds the PhysicalWebBroadcastService to the PhysicalWeb Sharing Feature. It's going to be the meat of the project but this is just the template for more changes to come.
https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:16: // bluetooth.le.AdvertiseCallback and bluetooth.BluetoothAdapter require API level 21. Why is this a separate style of comment than the javadoc-one? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:19: public class PhysicalWebBroadcastService extends Service { 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. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:23: public int onStartCommand(Intent intent, int flags, int startId) { Will this be a foreground service? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:24: String displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); This will crash if there are no pending start-commands in the case of a restart of the service after it being killed. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:25: // This should never happen. Can you add a newline before this line? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:30: // TODO(iankc): implement parsing, broadcasting, and notifications. Can you add a newline before this line to separate it from the edge-case above? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:32: return START_STICKY; 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. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:18: protected void handleShareAction(ChromeActivity triggeringActivity) { Should this be protected by some finch flag? Or is that at a different level? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:19: String url = triggeringActivity.getActivityTab().getUrl(); Can the activity tab ever be null? Say the user taps the tab switcher immediately after hitting share or some other weird edge-case? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:23: startService(intent); Is this guaranteed to only pinky-promise be called while Chrome is still in the foreground? https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:31: */ Would probably be helpful with a @return here. (remember empty line to separate the two for readability)
Hey Tommy, I hope the JavaDocs can clear things up. The prototype behaves in the way the docs describe so it's all possible, but they'll be in future CLs. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:16: // bluetooth.le.AdvertiseCallback and bluetooth.BluetoothAdapter require API level 21. On 2017/03/06 22:34:13, nyquist wrote: > Why is this a separate style of comment than the javadoc-one? Done. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:19: public class PhysicalWebBroadcastService extends Service { On 2017/03/06 22:34:13, nyquist wrote: > 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. There is no upper limit for how long it will be running. The user will have to terminate broadcasting through a broadcast notification. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:23: public int onStartCommand(Intent intent, int flags, int startId) { On 2017/03/06 22:34:13, nyquist wrote: > Will this be a foreground service? This is not a foreground service. It is a background Service. However, the user can stop the service through the notification. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:24: String displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); On 2017/03/06 22:34:13, nyquist wrote: > This will crash if there are no pending start-commands in the case of a restart > of the service after it being killed. Done. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:25: // This should never happen. On 2017/03/06 22:34:13, nyquist wrote: > Can you add a newline before this line? Done. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:30: // TODO(iankc): implement parsing, broadcasting, and notifications. On 2017/03/06 22:34:13, nyquist wrote: > Can you add a newline before this line to separate it from the edge-case above? Done. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:32: return START_STICKY; On 2017/03/06 22:34:13, nyquist wrote: > 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. 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. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:18: protected void handleShareAction(ChromeActivity triggeringActivity) { On 2017/03/06 22:34:14, nyquist wrote: > Should this be protected by some finch flag? Or is that at a different level? This is protected in the featureIsAvailable() method below, that uses a finch feature flag to tell if it should be enabled. The finch feature flag lives in PhysicalWeb.sharingIsEnabled() method. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:19: String url = triggeringActivity.getActivityTab().getUrl(); On 2017/03/06 22:34:14, nyquist wrote: > Can the activity tab ever be null? Say the user taps the tab switcher > immediately after hitting share or some other weird edge-case? Yes, the activity tab can be null! However, in the ShareActivity (the abstract class that this activity inherits from) it checks to make sure it's not null before calling handleShareAction. https://cs.corp.google.com/clankium/src/chrome/android/java/src/org/chromium/... https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:23: startService(intent); On 2017/03/06 22:34:14, nyquist wrote: > Is this guaranteed to only pinky-promise be called while Chrome is still in the > foreground? This should only be called when chrome is in the foreground. The only way to call onCreate() is through the share picker, which has to be in the foreground. Also, we handle for when the triggering chromeActivity is null, which might also handle this case. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:31: */ On 2017/03/06 22:34:14, nyquist wrote: > Would probably be helpful with a @return here. (remember empty line to separate > the two for readability) Done.
I think now you have some more meat you could put into the CL description as well (from the helpful JavaDoc you wrote). https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java (right): https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:18: protected void handleShareAction(ChromeActivity triggeringActivity) { On 2017/03/07 02:10:39, iankc wrote: > On 2017/03/06 22:34:14, nyquist wrote: > > Should this be protected by some finch flag? Or is that at a different level? > > This is protected in the featureIsAvailable() method below, that uses a finch > feature flag to tell if it should be enabled. The finch feature flag lives in > PhysicalWeb.sharingIsEnabled() method. Acknowledged. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:19: String url = triggeringActivity.getActivityTab().getUrl(); On 2017/03/07 02:10:40, iankc wrote: > On 2017/03/06 22:34:14, nyquist wrote: > > Can the activity tab ever be null? Say the user taps the tab switcher > > immediately after hitting share or some other weird edge-case? > > Yes, the activity tab can be null! However, in the ShareActivity (the abstract > class that this activity inherits from) it checks to make sure it's not null > before calling handleShareAction. > https://cs.corp.google.com/clankium/src/chrome/android/java/src/org/chromium/... Acknowledged. https://codereview.chromium.org/2719493003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:23: startService(intent); On 2017/03/07 02:10:39, iankc wrote: > On 2017/03/06 22:34:14, nyquist wrote: > > Is this guaranteed to only pinky-promise be called while Chrome is still in > the > > foreground? > > This should only be called when chrome is in the foreground. The only way to > call onCreate() is through the share picker, which has to be in the foreground. > Also, we handle for when the triggering chromeActivity is null, which might also > handle this case. Acknowledged. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:20: * sharing chooser intent. If the URL received from the triggering Tab is a validated Remove " a " https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:28: * If the user terminates Chrome or the OS shutsdown this service mid-broadcasting, when the service "shuts down" https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:43: private boolean mStartedByRestart; Is the plan to use this for something later? Otherwise, it could be a local variable. The JavaDoc hints to that this will be used in the future maybe though. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:47: String displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); I think you want to call fetchBroadcastData() here. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:65: private String fetchBroadcastData(Intent intent) { For now, this just retrieves the display URL, so maybe just use that as a method name too, until it expands to return something more? I.e. fetchDisplayUrl(...)? https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:68: mStartedByRestart = intent == null; I like that you split this out to a variable to signify what a null-intent means. Thanks! https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:71: displayUrl = sharedPrefs.getString(PREVIOUS_BROADCAST_URL_KEY, null); You can probably just return this directly here, especially if you change the method name. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:75: displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); I'd just declare displayUrl here.
This change creates the skeleton for the PhysicalWebBroadcastService, which will be the service that handles the Physical Web Sharing Feature. Here is a background to what the Service will do: This background service will start when the user activates the Physical Web Sharing item in the sharing chooser intent. If the URL received from the triggering Tab is validated by the Physical Web Service then it will encode the URL into an Eddystone URL and broadcast the URL through BLE. To notify and provide the user with the ability to terminate broadcasting a persistent notification with a cancel button will be created. This cancel button will stop the service and will be the only way for the user to stop the serive. If the user terminates Chrome or the OS shuts down this service mid-broadcasting, when the service get's restarted it will restart broadcasting as well by gathering the URL from shared preferences. This will create a seemless experience to the user by behaving as though broadcasting is not connected to the Chrome application. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:20: * sharing chooser intent. If the URL received from the triggering Tab is a validated On 2017/03/07 06:58:33, nyquist wrote: > Remove " a " Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:28: * If the user terminates Chrome or the OS shutsdown this service mid-broadcasting, when the service On 2017/03/07 06:58:33, nyquist wrote: > "shuts down" Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:43: private boolean mStartedByRestart; On 2017/03/07 06:58:33, nyquist wrote: > Is the plan to use this for something later? Otherwise, it could be a local > variable. The JavaDoc hints to that this will be used in the future maybe > though. Yeah, there are minor changes to the procedure when it is "startedByRestart", so it's useful to make it an instance attribute. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:47: String displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); On 2017/03/07 06:58:33, nyquist wrote: > I think you want to call fetchBroadcastData() here. Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:65: private String fetchBroadcastData(Intent intent) { On 2017/03/07 06:58:33, nyquist wrote: > For now, this just retrieves the display URL, so maybe just use that as a method > name too, until it expands to return something more? I.e. fetchDisplayUrl(...)? Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:68: mStartedByRestart = intent == null; On 2017/03/07 06:58:33, nyquist wrote: > I like that you split this out to a variable to signify what a null-intent > means. Thanks! Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:71: displayUrl = sharedPrefs.getString(PREVIOUS_BROADCAST_URL_KEY, null); On 2017/03/07 06:58:33, nyquist wrote: > You can probably just return this directly here, especially if you change the > method name. Done. https://codereview.chromium.org/2719493003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:75: displayUrl = intent.getStringExtra(DISPLAY_URL_KEY); On 2017/03/07 06:58:33, nyquist wrote: > I'd just declare displayUrl here. Done.
Description was changed from ========== Add PhysicalWebBroadcastService This change adds the PhysicalWebBroadcastService, which performs no action other than accepting a URL from the PhysicalWebShareActivity. BUG=685856 ========== to ========== Add PhysicalWebBroadcastService This change creates the skeleton for the PhysicalWebBroadcastService, which will be the service that handles the Physical Web Sharing Feature. Here is a background to what the Service will do: This background service will start when the user activates the Physical Web Sharing item in the sharing chooser intent. If the URL received from the triggering Tab is validated by the Physical Web Service then it will encode the URL into an Eddystone URL and broadcast the URL through BLE. To notify and provide the user with the ability to terminate broadcasting a persistent notification with a cancel button will be created. This cancel button will stop the service and will be the only way for the user to stop the service. If the user terminates Chrome or the OS shuts down this service mid-broadcasting, when the service get's restarted it will restart broadcasting as well by gathering the URL from shared preferences. This will create a seemless experience to the user by behaving as though broadcasting is not connected to the Chrome application. BUG=685856 ==========
lgtm. thanks!
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org Link to the patchset: https://codereview.chromium.org/2719493003/#ps160001 (title: "Updating Commit Message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488910245283380,
"parent_rev": "bf2bb744c704f16735d19c617f69beaba236db9b", "commit_rev":
"f2ef57e81267138c5f8fabc2ea7044b59600238a"}
Message was sent while issue was closed.
Description was changed from ========== Add PhysicalWebBroadcastService This change creates the skeleton for the PhysicalWebBroadcastService, which will be the service that handles the Physical Web Sharing Feature. Here is a background to what the Service will do: This background service will start when the user activates the Physical Web Sharing item in the sharing chooser intent. If the URL received from the triggering Tab is validated by the Physical Web Service then it will encode the URL into an Eddystone URL and broadcast the URL through BLE. To notify and provide the user with the ability to terminate broadcasting a persistent notification with a cancel button will be created. This cancel button will stop the service and will be the only way for the user to stop the service. If the user terminates Chrome or the OS shuts down this service mid-broadcasting, when the service get's restarted it will restart broadcasting as well by gathering the URL from shared preferences. This will create a seemless experience to the user by behaving as though broadcasting is not connected to the Chrome application. BUG=685856 ========== to ========== Add PhysicalWebBroadcastService This change creates the skeleton for the PhysicalWebBroadcastService, which will be the service that handles the Physical Web Sharing Feature. Here is a background to what the Service will do: This background service will start when the user activates the Physical Web Sharing item in the sharing chooser intent. If the URL received from the triggering Tab is validated by the Physical Web Service then it will encode the URL into an Eddystone URL and broadcast the URL through BLE. To notify and provide the user with the ability to terminate broadcasting a persistent notification with a cancel button will be created. This cancel button will stop the service and will be the only way for the user to stop the service. If the user terminates Chrome or the OS shuts down this service mid-broadcasting, when the service get's restarted it will restart broadcasting as well by gathering the URL from shared preferences. This will create a seemless experience to the user by behaving as though broadcasting is not connected to the Chrome application. BUG=685856 Review-Url: https://codereview.chromium.org/2719493003 Cr-Commit-Position: refs/heads/master@{#455211} Committed: https://chromium.googlesource.com/chromium/src/+/f2ef57e81267138c5f8fabc2ea70... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f2ef57e81267138c5f8fabc2ea70... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
