Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1326593006:
Add the Physical Web to Chrome on Android (Closed)
DescriptionAdd the Physical Web to Chrome on Android
This change introduces a framework to receive URLs broadcasted by
BLE and other technologies. Those URLs are surfaced to the user
who is then offered the chance to visit them via Chrome.
BUG=529962
Committed: https://crrev.com/d5f409f4f580a17d4dc0bf705753c6f3cfd9d236
Cr-Commit-Position: refs/heads/master@{#351706}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : thread safe singleton #Patch Set 4 : Add an OWNERS file #
Total comments: 4
Patch Set 5 : Added xxxhdpi notification #Patch Set 6 : Remove extra line #
Total comments: 2
Patch Set 7 : Fix string casing #Patch Set 8 : Add UrlManager class #
Total comments: 24
Patch Set 9 : Addressed jdduke and nyquist's comments #
Total comments: 14
Patch Set 10 : Handled nyquist's comments. #Patch Set 11 : Fixed compile error with notifications #
Total comments: 4
Patch Set 12 : Handled jdduke's comments #Patch Set 13 : Moved switch to non-native section of ChromeSwitches.java #
Total comments: 2
Patch Set 14 : Removed cco3 from OWNERS #Patch Set 15 : Remove OWNERS file #Patch Set 16 : Suppress unconfirmed cast warning #
Total comments: 4
Patch Set 17 : Check an unchecked cast #
Total comments: 2
Patch Set 18 : Handled jdduke's comments #Messages
Total messages: 69 (22 generated)
cco3@chromium.org changed reviewers: + qinmin@chromium.org
https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:20: public static PhysicalWebBleClient getInstance(Application application) { It is not thread safe when creating the singleton https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:22: ChromeApplication chromeApplication = (ChromeApplication) application; This feels pretty strange that the class singleton method relies on another class to create itself. Why not put this method inside an interface called PhysicalWebBleClientFactory, and let ChromeApplication implements that
https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:20: public static PhysicalWebBleClient getInstance(Application application) { On 2015/09/09 21:15:42, qinmin wrote: > It is not thread safe when creating the singleton Done. https://codereview.chromium.org/1326593006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:22: ChromeApplication chromeApplication = (ChromeApplication) application; On 2015/09/09 21:15:42, qinmin wrote: > This feels pretty strange that the class singleton method relies on another > class to create itself. > Why not put this method inside an interface called PhysicalWebBleClientFactory, > and let ChromeApplication implements that It is a little weird, but I was just following a pattern I saw elsewhere in the codebase (See CustomTabsConnection.java) and it's come to make sense to me. There are quite a few classes that need ChromeApplication to create instances of themselves and none of them burden ChromeApplication with implementing an interface or storing the static instance. I can change it if you want, but I think this is the standard way of accomplishing the task.
lgtm
aurimas@chromium.org changed reviewers: + aurimas@chromium.org
not LGTM quite yet. There are a few things still that need fixing. - You are missing xxxhdpi asset for ic_physical_web_notificaiton. - Add BUG=529962 in the commit message and expand the commit message to explain more what this CL does. https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:18: ChromeVersionInfo.isLocalBuild() || ChromeVersionInfo.isDevBuild(); It seems like you are enabling this in Chrome Dev and that requires emailing Chrome launch review folks. Have you gone through that? https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:13: Remove the extra line
https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:18: ChromeVersionInfo.isLocalBuild() || ChromeVersionInfo.isDevBuild(); On 2015/09/10 17:09:03, aurimas wrote: > It seems like you are enabling this in Chrome Dev and that requires emailing > Chrome launch review folks. Have you gone through that? Is that a problem even though it's behind a flag? If so, I can remove it on dev. https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:13: On 2015/09/10 17:09:04, aurimas wrote: > Remove the extra line Done.
On 2015/09/11 17:10:46, cco3 wrote: > https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java > (right): > > https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:18: > ChromeVersionInfo.isLocalBuild() || ChromeVersionInfo.isDevBuild(); > On 2015/09/10 17:09:03, aurimas wrote: > > It seems like you are enabling this in Chrome Dev and that requires emailing > > Chrome launch review folks. Have you gone through that? > > Is that a problem even though it's behind a flag? If so, I can remove it on > dev. Should we be gating the feature on chrome version at all, given that this is behind a flag? If so, do we need to disable the flag entirely on chrome versions where it is not supported? > > https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java > (right): > > https://codereview.chromium.org/1326593006/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:13: > > On 2015/09/10 17:09:04, aurimas wrote: > > Remove the extra line > > Done.
newt@chromium.org changed reviewers: + newt@chromium.org
string drive-by https://codereview.chromium.org/1326593006/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1326593006/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2247: =1 {1 URL Nearby} Note: Android uses "Sentence case" for UI strings. So "nearby" should be lowercase here.
https://codereview.chromium.org/1326593006/diff/100001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1326593006/diff/100001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:2247: =1 {1 URL Nearby} On 2015/09/11 22:12:12, newt wrote: > Note: Android uses "Sentence case" for UI strings. So "nearby" should be > lowercase here. Done.
Before landing, can we get a bug opened for this feature, as well as some more info in the CL description?
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
Is there a follow-up patch for this? As far as I can tell this patch doesn't actually do anything, which is fine I suppose but it'd be nice to see how this plumbing will eventually be used before landing this change. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:13: * This class provides the basic interface to the Physical Web feature. This is the exact same comment as that for "PhysicalWebBleClient"? It's not at all clear why we need both this and the client? https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:16: public static boolean featureIsEnabled() { Nit: These public methods should have Javadoc comments. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:17: public class PhysicalWebBleClient { What does "Ble" mean in the class name? https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:18: private static AtomicReference<PhysicalWebBleClient> sInstance = Why atomic? Do you expect this to be used on multiple threads?
On 2015/09/15 16:46:39, jdduke wrote: > Is there a follow-up patch for this? As far as I can tell this patch doesn't > actually do anything, which is fine I suppose but it'd be nice to see how this > plumbing will eventually be used before landing this change. Nevermind, I see now...
nyquist@chromium.org changed reviewers: + nyquist@chromium.org
https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:25: private static AtomicReference<UrlManager> sInstance = new AtomicReference<UrlManager>(); final https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:32: private Context mContext; final https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:33: private NotificationManagerCompat mNotificationManager; final https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:88: mNotificationManager.cancelAll(); Does this cancel other notifications from Chrome, or only notifications from this class? https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:98: //PendingIntent pendingIntent = createReturnToAppPendingIntent(); Remove commented out line. https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:103: //.setContentIntent(pendingIntent) Remove commented out line.
https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:13: * This class provides the basic interface to the Physical Web feature. On 2015/09/15 16:46:39, jdduke wrote: > This is the exact same comment as that for "PhysicalWebBleClient"? It's not at > all clear why we need both this and the client? Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:16: public static boolean featureIsEnabled() { On 2015/09/15 16:46:39, jdduke wrote: > Nit: These public methods should have Javadoc comments. Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:17: public class PhysicalWebBleClient { On 2015/09/15 16:46:39, jdduke wrote: > What does "Ble" mean in the class name? Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:18: private static AtomicReference<PhysicalWebBleClient> sInstance = On 2015/09/15 16:46:39, jdduke wrote: > Why atomic? Do you expect this to be used on multiple threads? It was just a pattern for singletons that I saw in another class. Is there a different pattern you would recommend? https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:25: private static AtomicReference<UrlManager> sInstance = new AtomicReference<UrlManager>(); On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > final Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:32: private Context mContext; On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > final Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:33: private NotificationManagerCompat mNotificationManager; On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > final Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:88: mNotificationManager.cancelAll(); On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > Does this cancel other notifications from Chrome, or only notifications from > this class? Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:98: //PendingIntent pendingIntent = createReturnToAppPendingIntent(); On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > Remove commented out line. Done. https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:103: //.setContentIntent(pendingIntent) On 2015/09/15 18:16:46, nyquist (OOO - back 9-18) wrote: > Remove commented out line. Done.
https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:183: public static final String ENABLE_PHYSICAL_WEB = "enable-physical-web"; Could this be part of chrome://flags? https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS (right): https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS:1: cco3@chromium.org Do you have anyone else you can add to this? https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:23: private static final String TAG = "cr.PhysicalWeb"; They decided to change the format again. I think it's just "PhysicalWeb" now. https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:31: private static final int NOTIFICATION_ID = 23; Could you move this to org.chromium.chrome.browser.notifications.NotificationConstants? https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:62: Log.d(TAG, "Adding URL: " + url); Will this (and remove below) be logged in release builds? https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:105: private void updateNotification(Set<String> urls) { Could you investigate if it's feasible for physicalweb to use / expand the notification framework in org.chromium.chrome.browser.notification ?
Do we have any sense at all what the power impact here will be? Has the services implementation side of the feature been instrumented/tested?
https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1326593006/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:61: public void addUrl(String url) { Should you add validation of the incoming URL? Such as not null, and possibly even things like whether GURL (C++ version) would treat this as a valid URL?
https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java:183: public static final String ENABLE_PHYSICAL_WEB = "enable-physical-web"; On 2015/09/24 17:50:40, nyquist wrote: > Could this be part of chrome://flags? Yes. Do you mind if I relegate that to a subsequent change? https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS (right): https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS:1: cco3@chromium.org On 2015/09/24 17:50:40, nyquist wrote: > Do you have anyone else you can add to this? Done. https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:23: private static final String TAG = "cr.PhysicalWeb"; On 2015/09/24 17:50:40, nyquist wrote: > They decided to change the format again. I think it's just "PhysicalWeb" now. Done. https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:31: private static final int NOTIFICATION_ID = 23; On 2015/09/24 17:50:40, nyquist wrote: > Could you move this to > org.chromium.chrome.browser.notifications.NotificationConstants? Done. https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:61: public void addUrl(String url) { On 2015/09/24 17:56:48, nyquist wrote: > Should you add validation of the incoming URL? Such as not null, and possibly > even things like whether GURL (C++ version) would treat this as a valid URL? I don't think so. I'd rather this be a dumber utility and shift the burden on the classes using it. https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:62: Log.d(TAG, "Adding URL: " + url); On 2015/09/24 17:50:40, nyquist wrote: > Will this (and remove below) be logged in release builds? I don't really know what the best practices are around logging. Let me know if there are special tools to only log in certain builds. https://chromiumcodereview.appspot.com/1326593006/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:105: private void updateNotification(Set<String> urls) { On 2015/09/24 17:50:40, nyquist wrote: > Could you investigate if it's feasible for physicalweb to use / expand the > notification framework in org.chromium.chrome.browser.notification ? Nothing else actually uses the Proxy class in there. It seems only intended for use so that you can swap out the standard notification manager with a mock.
No objections on my end, though I would suggest we wait to land until the downstream bits are finalized/ready. https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:709: return new PhysicalWebBleClient(); Is there only one ChromeApplication per process? Or can there be multiple when we're in document (Hera) mode? https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://codereview.chromium.org/1326593006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:18: private static AtomicReference<PhysicalWebBleClient> sInstance = On 2015/09/16 17:41:28, cco3 wrote: > On 2015/09/15 16:46:39, jdduke wrote: > > Why atomic? Do you expect this to be used on multiple threads? > > It was just a pattern for singletons that I saw in another class. Is there a > different pattern you would recommend? I think a common pattern is something like: private static class LazyHolder { private static final Foo INSTANCE = new Foo(); } but I guess that won't work here. In any case, this class should only be accessed from one thread, so we don't really need the atomic guarantees. To make that concrete you could add a ThreadUtils.assertOnUiThread call at the start of the function, and just use a regular static member. https://codereview.chromium.org/1326593006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://codereview.chromium.org/1326593006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:33: * appropriatePhysicalWebBleClient implementation. Nit: space after appropriate. https://codereview.chromium.org/1326593006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1326593006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:87: // Restore the cached urls Nit: add periods to comment sentences (a few places in this file).
https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:709: return new PhysicalWebBleClient(); On 2015/09/28 22:38:34, jdduke wrote: > Is there only one ChromeApplication per process? Or can there be multiple when > we're in document (Hera) mode? No idea. What would be the implications if there were multiple? https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:18: private static AtomicReference<PhysicalWebBleClient> sInstance = On 2015/09/28 22:38:34, jdduke wrote: > On 2015/09/16 17:41:28, cco3 wrote: > > On 2015/09/15 16:46:39, jdduke wrote: > > > Why atomic? Do you expect this to be used on multiple threads? > > > > It was just a pattern for singletons that I saw in another class. Is there a > > different pattern you would recommend? > > I think a common pattern is something like: > > private static class LazyHolder { > private static final Foo INSTANCE = new Foo(); > } > > but I guess that won't work here. In any case, this class should only be > accessed from one thread, so we don't really need the atomic guarantees. To make > that concrete you could add a ThreadUtils.assertOnUiThread call at the start of > the function, and just use a regular static member. Done. https://chromiumcodereview.appspot.com/1326593006/diff/200001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:33: * appropriatePhysicalWebBleClient implementation. On 2015/09/28 22:38:34, jdduke wrote: > Nit: space after appropriate. Done. https://chromiumcodereview.appspot.com/1326593006/diff/200001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/200001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:87: // Restore the cached urls On 2015/09/28 22:38:34, jdduke wrote: > Nit: add periods to comment sentences (a few places in this file). Done.
On 2015/09/28 23:20:25, cco3 wrote: > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java > (right): > > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:709: > return new PhysicalWebBleClient(); > On 2015/09/28 22:38:34, jdduke wrote: > > Is there only one ChromeApplication per process? Or can there be multiple when > > we're in document (Hera) mode? > > No idea. What would be the implications if there were multiple? Well it might be a little weird (or breaking?) to have a singleton keyed to a ChromeApplication when there can be multiple. But nyquist@ assured me there should be just one ChromeApplication per (browser) process, so I don't think we need to worry.
On 2015/09/28 23:23:40, jdduke wrote: > On 2015/09/28 23:20:25, cco3 wrote: > > > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > > File > chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java > > (right): > > > > > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:709: > > return new PhysicalWebBleClient(); > > On 2015/09/28 22:38:34, jdduke wrote: > > > Is there only one ChromeApplication per process? Or can there be multiple > when > > > we're in document (Hera) mode? > > > > No idea. What would be the implications if there were multiple? > > Well it might be a little weird (or breaking?) to have a singleton keyed to a > ChromeApplication when there can be multiple. But nyquist@ assured me there > should be just one ChromeApplication per (browser) process, so I don't think we > need to worry. Anything else needed here?
On 2015/09/29 22:36:31, cco3 wrote: > On 2015/09/28 23:23:40, jdduke wrote: > > On 2015/09/28 23:20:25, cco3 wrote: > > > > > > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > > > File > > chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java > > > (right): > > > > > > > > > https://chromiumcodereview.appspot.com/1326593006/diff/140001/chrome/android/... > > > > > > chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:709: > > > return new PhysicalWebBleClient(); > > > On 2015/09/28 22:38:34, jdduke wrote: > > > > Is there only one ChromeApplication per process? Or can there be multiple > > when > > > > we're in document (Hera) mode? > > > > > > No idea. What would be the implications if there were multiple? > > > > Well it might be a little weird (or breaking?) to have a singleton keyed to a > > ChromeApplication when there can be multiple. But nyquist@ assured me there > > should be just one ChromeApplication per (browser) process, so I don't think > we > > need to worry. > > Anything else needed here? Not for that particular issue I don't think.
Hi aurimas, nyquist Anything else or does it look OK to commit?
lgtm
mostly lg, just a question about the OWNERS file. https://codereview.chromium.org/1326593006/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS (right): https://codereview.chromium.org/1326593006/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS:1: cco3@chromium.org I don't think we usually add non-committers to OWNERS-files. See https://www.chromium.org/developers/owners-files Depending on their familiarity with how we develop Android-code (Java for example) in Chromium, the other owners are OK I guess.
https://codereview.chromium.org/1326593006/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS (right): https://codereview.chromium.org/1326593006/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/OWNERS:1: cco3@chromium.org On 2015/09/30 18:46:39, nyquist wrote: > I don't think we usually add non-committers to OWNERS-files. > > See https://www.chromium.org/developers/owners-files > > Depending on their familiarity with how we develop Android-code (Java for > example) in Chromium, the other owners are OK I guess. Done.
lgtm. Please CC or add the new owners as reviewers so they know about their new responsibility :-)
cco3@chromium.org changed reviewers: + dvh@chromium.org, mmocny@chromium.org
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1326593006/#ps260001 (title: "Removed cco3 from OWNERS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/260001
On 2015/09/30 19:14:13, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1326593006/260001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1326593006/260001 I'd really prefer not to add any additional owners here. There's really not that much upstream code here, can we do without the owners change?
The CQ bit was unchecked by jdduke@chromium.org
On 2015/09/30 20:21:13, jdduke wrote: > On 2015/09/30 19:14:13, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1326593006/260001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1326593006/260001 > > I'd really prefer not to add any additional owners here. There's really not that > much upstream code here, can we do without the owners change? (Sorry, I didn't even notice the OWNERS change until Tommy called it out).
On 2015/09/30 20:23:20, jdduke wrote: > On 2015/09/30 20:21:13, jdduke wrote: > > On 2015/09/30 19:14:13, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/patch-status/1326593006/260001 > > > View timeline at > > > https://chromium-cq-status.appspot.com/patch-timeline/1326593006/260001 > > > > I'd really prefer not to add any additional owners here. There's really not > that > > much upstream code here, can we do without the owners change? > > (Sorry, I didn't even notice the OWNERS change until Tommy called it out). I assume you just want me to remove the OWNERS file entirely. Done.
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, nyquist@chromium.org, qinmin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1326593006/#ps280001 (title: "Remove OWNERS file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/280001
On 2015/09/30 21:13:04, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1326593006/280001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1326593006/280001 These failures are just http timeouts...will it retry automatically or is there something else I need to do?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
On 2015/09/30 21:34:21, cco3 wrote: > On 2015/09/30 21:13:04, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1326593006/280001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1326593006/280001 > > These failures are just http timeouts...will it retry automatically or is there > something else I need to do? OK, I figured it out. Getting the hang of this...
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, nyquist@chromium.org, qinmin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1326593006/#ps300001 (title: "Suppress unconfirmed cast warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/300001
https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:31: public static PhysicalWebBleClient getInstance(Application application) { Any reason we can't pass in a ChromeApplication directly?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, nyquist@chromium.org, qinmin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1326593006/#ps320001 (title: "Check an unchecked cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/320001
The CQ bit was unchecked by jdduke@chromium.org
https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:25: private static final AtomicReference<UrlManager> sInstance = new AtomicReference<UrlManager>(); Why do we need atomics here? This class isn't thread safe anyway, shouldn't it only ever be accessed on one thread? https://chromiumcodereview.appspot.com/1326593006/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:30: public static PhysicalWebBleClient getInstance(Application application) { Can't you just pass in ChromeApplication directly instead of performing the cast/check dance?
On 2015/09/30 21:56:23, jdduke wrote: > https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java > (right): > > https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:31: > public static PhysicalWebBleClient getInstance(Application application) { > Any reason we can't pass in a ChromeApplication directly? I was just going off the pattern I saw in CustomTabsConnection. I'll change it.
https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:31: public static PhysicalWebBleClient getInstance(Application application) { On 2015/09/30 21:56:23, jdduke wrote: > Any reason we can't pass in a ChromeApplication directly? I was just copying the pattern I saw in CustomTabsConnection, I'll change it. https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/300001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:25: private static final AtomicReference<UrlManager> sInstance = new AtomicReference<UrlManager>(); On 2015/10/01 00:49:28, jdduke wrote: > Why do we need atomics here? This class isn't thread safe anyway, shouldn't it > only ever be accessed on one thread? Another pattern I was just copying from elsewhere. https://chromiumcodereview.appspot.com/1326593006/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java (right): https://chromiumcodereview.appspot.com/1326593006/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBleClient.java:30: public static PhysicalWebBleClient getInstance(Application application) { On 2015/10/01 00:49:28, jdduke wrote: > Can't you just pass in ChromeApplication directly instead of performing the > cast/check dance? Done.
The CQ bit was checked by cco3@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/320001
The CQ bit was unchecked by cco3@chromium.org
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aurimas@chromium.org, nyquist@chromium.org, qinmin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1326593006/#ps340001 (title: "Handled jdduke's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1326593006/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1326593006/340001
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d5f409f4f580a17d4dc0bf705753c6f3cfd9d236 Cr-Commit-Position: refs/heads/master@{#351706} |
