Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741103002:
Create Broadcast Notification (Closed)
DescriptionCreate Broadcast Notification
This change adds notifications to the PhysicalWebBroadcastService. It is
the only way to disable broadcasting from a users perspective. It
implements a broadcastReceiver to catch when the user hits the stop
button on the notification.
The change from commit() to apply() in the fetchDisplayUrl actually
fixes a StrictMode violation.
BUG=685856
Review-Url: https://codereview.chromium.org/2741103002
Cr-Commit-Position: refs/heads/master@{#456842}
Committed: https://chromium.googlesource.com/chromium/src/+/d7bfcc3fa626adb9876f2d0ef925131decd70aa8
Patch Set 1 #
Total comments: 28
Patch Set 2 : Conleys nits 1 #
Total comments: 2
Patch Set 3 : Error Logging #
Total comments: 9
Patch Set 4 : Teds Nits 1 #Patch Set 5 : Using ChromeNotificationBuilder #Patch Set 6 : Adding to Notification Builder #Patch Set 7 : Stylizing #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Create Broadcast Notification This change adds notifications to the PhysicalWebBroadcastService. It is the only way to disable broadcasting from a users perspective. It implements a broadcastReceiver to catch when the user hits the stop button on the notification. The change from commit() to apply() in the fetchDisplayUrl actually fixes a StrictMode violation. BUG=685856 ========== to ========== Create Broadcast Notification This change adds notifications to the PhysicalWebBroadcastService. It is the only way to disable broadcasting from a users perspective. It implements a broadcastReceiver to catch when the user hits the stop button on the notification. The change from commit() to apply() in the fetchDisplayUrl actually fixes a StrictMode violation. BUG=685856 ==========
iankc@google.com changed reviewers: + cco3@chromium.org, mattreynolds@google.com
Creates the notifications.
mattreynolds@chromium.org changed reviewers: + mattreynolds@chromium.org
lgtm https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:40: * service get's restarted it will restart broadcasting as well by gathering the URL from get's -> gets https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:41: * shared preferences. This will create a seemless experience to the user by behaving as seamless https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:84: // TODO(iankc): implement parsing, broadcasting, and notifications. is this TODONE now?
https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:64: Log.d(TAG, "Stop Service"); "Stop button pressed on Physical Web broadcast notification" (or remove it altogether) https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:75: if (displayUrl == null) { Log.e inside of here https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:81: filter.addAction(STOP_SERVICE); Just pass this to the constructor. registerReceiver(mBroadcastReceiver, new IntentFilter(STOP_SERVICE)); https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:117: * Surfaces a notification to the user that the Physical Web is broadcasting. The notification The first sentence should be on its own line. Start a new line for the rest of the text. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:118: * specifies the URL that is being broadcast and cannot be swiped away. You might want to mention that it has a stop button. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:122: context.registerReceiver(mBroadcastReceiver, new IntentFilter(STOP_SERVICE)); Haven't you already registered this? https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:125: NotificationManager notificationManager = You should be using NotificationManagerProxy https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:140: NotificationManagerProxy notificationProxy = new NotificationManagerProxyImpl( I think it'd be better to call the variable notificationManager https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2625: <message name="IDS_PHYSICAL_WEB_BROADCAST_NOTIFICATION" desc="The message that will be displayed when the url attempted to share is being broadcasted successfully."> Suggestions below: when a Physical Web URL is broadcasted successfully. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2626: Webpage is broadcasting "Website", I think. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2628: <message name="IDS_PHYSICAL_WEB_STOP_BROADCAST" desc="The message that will be displayed in the notification to stop broadcasting."> The label for the button within a notification that stops a Physical Web broadcast.
https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:40: * service get's restarted it will restart broadcasting as well by gathering the URL from On 2017/03/09 22:59:42, mattreynolds wrote: > get's -> gets Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:41: * shared preferences. This will create a seemless experience to the user by behaving as On 2017/03/09 22:59:42, mattreynolds wrote: > seamless Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:64: Log.d(TAG, "Stop Service"); On 2017/03/10 23:21:38, cco3 wrote: > "Stop button pressed on Physical Web broadcast notification" (or remove it > altogether) Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:75: if (displayUrl == null) { On 2017/03/10 23:21:38, cco3 wrote: > Log.e inside of here Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:81: filter.addAction(STOP_SERVICE); On 2017/03/10 23:21:38, cco3 wrote: > Just pass this to the constructor. > > registerReceiver(mBroadcastReceiver, new IntentFilter(STOP_SERVICE)); Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:84: // TODO(iankc): implement parsing, broadcasting, and notifications. On 2017/03/09 22:59:42, mattreynolds wrote: > is this TODONE now? Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:117: * Surfaces a notification to the user that the Physical Web is broadcasting. The notification On 2017/03/10 23:21:38, cco3 wrote: > The first sentence should be on its own line. Start a new line for the rest of > the text. Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:118: * specifies the URL that is being broadcast and cannot be swiped away. On 2017/03/10 23:21:38, cco3 wrote: > You might want to mention that it has a stop button. Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:122: context.registerReceiver(mBroadcastReceiver, new IntentFilter(STOP_SERVICE)); On 2017/03/10 23:21:38, cco3 wrote: > Haven't you already registered this? Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:125: NotificationManager notificationManager = On 2017/03/10 23:21:38, cco3 wrote: > You should be using NotificationManagerProxy Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:140: NotificationManagerProxy notificationProxy = new NotificationManagerProxyImpl( On 2017/03/10 23:21:38, cco3 wrote: > I think it'd be better to call the variable notificationManager Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2625: <message name="IDS_PHYSICAL_WEB_BROADCAST_NOTIFICATION" desc="The message that will be displayed when the url attempted to share is being broadcasted successfully."> On 2017/03/10 23:21:38, cco3 wrote: > Suggestions below: > when a Physical Web URL is broadcasted successfully. Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2626: Webpage is broadcasting On 2017/03/10 23:21:38, cco3 wrote: > "Website", I think. Done. https://codereview.chromium.org/2741103002/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2628: <message name="IDS_PHYSICAL_WEB_STOP_BROADCAST" desc="The message that will be displayed in the notification to stop broadcasting."> On 2017/03/10 23:21:38, cco3 wrote: > The label for the button within a notification that stops a Physical Web > broadcast. Done.
lgtm https://codereview.chromium.org/2741103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:65: } Maybe log an error after this, because we shouldn't get here.
Hey Tommy, Here is a change for adding notifications to Physical Web Sharing. Let me know if we're overloading you with reviews. Thanks! https://codereview.chromium.org/2741103002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:65: } On 2017/03/11 00:09:46, cco3 wrote: > Maybe log an error after this, because we shouldn't get here. Done.
iankc@google.com changed reviewers: + nyquist@chromium.org
iankc@google.com changed reviewers: + tedchoc@chromium.org - nyquist@chromium.org
Hey Ted, Here is a change for adding notifications to Physical Web Sharing. Let me know if we're overloading you with reviews. Thanks!
tedchoc@chromium.org changed reviewers: + awdf@chromium.org
https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:53: private static final String STOP_SERVICE = "stop_service"; I would use a more fully defined action, something like org.chromium.chrome.browser.physicalweb.stop_service In an attempt to avoid collisions with other components (although it looks like we might use com.google.android.apps.chrome as a more common prefix, but I don't think that matters). https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:63: if (action.equals(STOP_SERVICE)) { I would do STOP_SERVICE.equals(action) to ensure you can never NPE here https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:66: Log.e(TAG, "Unrecognized Broadcast Received"); should this be in an else? https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:127: notificationManager.notify(NotificationConstants.NOTIFICATION_ID_PHYSICAL_WEB, @awdf - do we need to do anything different notification-y things these days? Do we need to use AppHooks.get().createChromeNotificationBuilder(...) here instead?
https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:127: notificationManager.notify(NotificationConstants.NOTIFICATION_ID_PHYSICAL_WEB, On 2017/03/13 23:36:30, Ted C wrote: > @awdf - do we need to do anything different notification-y things these days? > Do we need to use AppHooks.get().createChromeNotificationBuilder(...) here > instead? Yes. For now, instead of 'new NotificationCompat.Builder(context)' you should call AppHooks.get().createChromeNotificationBuilder(..), see https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... for an example (you can use the same parameters). Then the ChromeNotificationBuilder it returns should have all the setters you need here. Email me or nyquist@ if you have any questions. Cheers!
Made awdf's change to use Apphooks.get().chromeNotificationBuilder https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java (right): https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:53: private static final String STOP_SERVICE = "stop_service"; On 2017/03/13 23:36:30, Ted C wrote: > I would use a more fully defined action, something like > > org.chromium.chrome.browser.physicalweb.stop_service > > In an attempt to avoid collisions with other components (although it looks like > we might use com.google.android.apps.chrome as a more common prefix, but I don't > think that matters). Done. https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:63: if (action.equals(STOP_SERVICE)) { On 2017/03/13 23:36:30, Ted C wrote: > I would do STOP_SERVICE.equals(action) to ensure you can never NPE here Done. https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:66: Log.e(TAG, "Unrecognized Broadcast Received"); On 2017/03/13 23:36:31, Ted C wrote: > should this be in an else? Done. https://codereview.chromium.org/2741103002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:127: notificationManager.notify(NotificationConstants.NOTIFICATION_ID_PHYSICAL_WEB, On 2017/03/14 13:06:31, awdf wrote: > On 2017/03/13 23:36:30, Ted C wrote: > > @awdf - do we need to do anything different notification-y things these days? > > Do we need to use AppHooks.get().createChromeNotificationBuilder(...) here > > instead? > > Yes. > > For now, instead of 'new NotificationCompat.Builder(context)' you should call > AppHooks.get().createChromeNotificationBuilder(..), see > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > for an example (you can use the same parameters). Then the > ChromeNotificationBuilder it returns should have all the setters you need here. > > Email me or nyquist@ if you have any questions. Cheers! Done.
This is ready to be reviewed again.
lgtm
On 2017/03/14 19:58:36, Ted C wrote: > lgtm AppHooks call lgtm
The CQ bit was checked by iankc@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, cco3@chromium.org Link to the patchset: https://codereview.chromium.org/2741103002/#ps120001 (title: "Stylizing")
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": 120001, "attempt_start_ts": 1489524553712820,
"parent_rev": "7f5d55a7e2e1ca2cdc10b3cd7e38685f87215584", "commit_rev":
"d7bfcc3fa626adb9876f2d0ef925131decd70aa8"}
Message was sent while issue was closed.
Description was changed from ========== Create Broadcast Notification This change adds notifications to the PhysicalWebBroadcastService. It is the only way to disable broadcasting from a users perspective. It implements a broadcastReceiver to catch when the user hits the stop button on the notification. The change from commit() to apply() in the fetchDisplayUrl actually fixes a StrictMode violation. BUG=685856 ========== to ========== Create Broadcast Notification This change adds notifications to the PhysicalWebBroadcastService. It is the only way to disable broadcasting from a users perspective. It implements a broadcastReceiver to catch when the user hits the stop button on the notification. The change from commit() to apply() in the fetchDisplayUrl actually fixes a StrictMode violation. BUG=685856 Review-Url: https://codereview.chromium.org/2741103002 Cr-Commit-Position: refs/heads/master@{#456842} Committed: https://chromium.googlesource.com/chromium/src/+/d7bfcc3fa626adb9876f2d0ef925... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d7bfcc3fa626adb9876f2d0ef925... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
