Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1821823003:
Add a UrlInfo class for the Physical Web (Closed)
DescriptionAdd a UrlInfo class for the Physical Web
The UrlManager will store instances of UrlInfo instead of raw strings.
This will permit us to include distances when we are supplied with
those in the future.
BUG=596708
Committed: https://crrev.com/9c74b5484e8ca993ee2cc47b115806e483189b1b
Cr-Commit-Position: refs/heads/master@{#388370}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Include another TODO #Patch Set 3 : Rebase #Patch Set 4 : Use present tense in javadoc #
Total comments: 8
Patch Set 5 : Incorporate Yaron's comments #
Total comments: 2
Patch Set 6 : Remove the builder #
Total comments: 2
Patch Set 7 : Rebase on new test fixes #
Total comments: 1
Patch Set 8 : Register new file in java_sources.gni #Patch Set 9 : Rebase #Patch Set 10 : Remove VisibleForTesting where inappropriate #
Total comments: 2
Patch Set 11 : Add VisibleForTesting to both addUrl methods #Patch Set 12 : Add VisibleForTesting to removeUrl #Messages
Total messages: 58 (26 generated)
Description was changed from ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 ========== to ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 ==========
cco3@chromium.org changed reviewers: + mattreynolds@chromium.org
lgtm https://codereview.chromium.org/1821823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1821823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:207: * @return A set of nearby and resolved URLs, sorted by distance. We should add a comment indicating this is still TODO (okay, technically they are already sorted by distance since it's hard-coded as -1...)
https://codereview.chromium.org/1821823003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1821823003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:207: * @return A set of nearby and resolved URLs, sorted by distance. On 2016/04/06 18:33:41, mattreynolds wrote: > We should add a comment indicating this is still TODO (okay, technically they > are already sorted by distance since it's hard-coded as -1...) Done.
cco3@chromium.org changed reviewers: + nyquist@chromium.org
cco3@chromium.org changed reviewers: + yfriedman@chromium.org
Hi Yaron, would you be able to take a look at this change?
seems ok, just a few thoughts based on first read https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:11: * This class represents a scanned URL and information associated with that URL. Probably worth calling out the the the fact that comparisons are based solely on url - I didn't realize at first that this was really just a url with associated metadata but the rest doesn't really matter. It seems atypical https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:34: public static class Builder { this doesn't seem particularly interesting - not clear why it's necessary and we don't typically add these in chromium code (although it's more common in the support library) https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:148: return mUrl; Seems strange as presumably this is only used for debug output - why not include the distance?
Thank you! https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:11: * This class represents a scanned URL and information associated with that URL. On 2016/04/12 23:28:32, Yaron wrote: > Probably worth calling out the the the fact that comparisons are based solely on > url - I didn't realize at first that this was really just a url with associated > metadata but the rest doesn't really matter. It seems atypical I'll just compare the other fields if that's preferable. https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:34: public static class Builder { On 2016/04/12 23:28:32, Yaron wrote: > this doesn't seem particularly interesting - not clear why it's necessary and we > don't typically add these in chromium code (although it's more common in the > support library) We will eventually associate more data with Urls...metadata (title, description, favicon), timestamp of when it was found by the BLE scan, etc. Let me know if that doesn't sound like a sufficient use case for a builder and I should just remove it. https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:148: return mUrl; On 2016/04/12 23:28:32, Yaron wrote: > Seems strange as presumably this is only used for debug output - why not include > the distance? Done.
lgtm https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:34: public static class Builder { On 2016/04/13 01:08:43, cco3 wrote: > On 2016/04/12 23:28:32, Yaron wrote: > > this doesn't seem particularly interesting - not clear why it's necessary and > we > > don't typically add these in chromium code (although it's more common in the > > support library) > > We will eventually associate more data with Urls...metadata (title, description, > favicon), timestamp of when it was found by the BLE scan, etc. Let me know if > that doesn't sound like a sufficient use case for a builder and I should just > remove it. There's only like 2-3 examples of it in chromium that I'm aware of so would prefer to keep with consistent style and not introduce it. https://codereview.chromium.org/1821823003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:108: * This calculation simply uses the URL since it is the only critical information. nit: update
https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:34: public static class Builder { On 2016/04/13 15:39:26, Yaron wrote: > On 2016/04/13 01:08:43, cco3 wrote: > > On 2016/04/12 23:28:32, Yaron wrote: > > > this doesn't seem particularly interesting - not clear why it's necessary > and > > we > > > don't typically add these in chromium code (although it's more common in the > > > support library) > > > > We will eventually associate more data with Urls...metadata (title, > description, > > favicon), timestamp of when it was found by the BLE scan, etc. Let me know if > > that doesn't sound like a sufficient use case for a builder and I should just > > remove it. > > There's only like 2-3 examples of it in chromium that I'm aware of so would > prefer to keep with consistent style and not introduce it. Done. https://codereview.chromium.org/1821823003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:108: * This calculation simply uses the URL since it is the only critical information. On 2016/04/13 15:39:26, Yaron wrote: > nit: update Done.
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattreynolds@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps100001 (title: "Remove the builder")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
lgtm https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:108: compareValue = mUrl.compareTo(other.mUrl); Optional nit: Why is this not the initial value of compareValue?
This change is dependent on https://codereview.chromium.org/1891273005/ https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java (right): https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:108: compareValue = mUrl.compareTo(other.mUrl); On 2016/04/15 16:27:05, nyquist wrote: > Optional nit: Why is this not the initial value of compareValue? Done.
On 2016/04/15 21:58:32, cco3 wrote: > This change is dependent on https://codereview.chromium.org/1891273005/ > > https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java > (right): > > https://codereview.chromium.org/1821823003/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlInfo.java:108: > compareValue = mUrl.compareTo(other.mUrl); > On 2016/04/15 16:27:05, nyquist wrote: > > Optional nit: Why is this not the initial value of compareValue? > > Done. Matt, could you take a look at the new logic in UrlManager?
https://codereview.chromium.org/1821823003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1821823003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:264: List<UrlInfo> resolvedUrls = getCachedResolvedUrls(); Let's keep the resolved URL list as a string set, the extra UrlInfo fields are unused.
On 2016/04/18 20:01:56, mattreynolds wrote: > https://codereview.chromium.org/1821823003/diff/120001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java > (right): > > https://codereview.chromium.org/1821823003/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:264: > List<UrlInfo> resolvedUrls = getCachedResolvedUrls(); > Let's keep the resolved URL list as a string set, the extra UrlInfo fields are > unused. Matt and I talked in person. This cleanup is better suited for the next change I'm working on.
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps120001 (title: "Rebase on new test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps140001 (title: "Register new file in java_sources.gni")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/04/19 01:49:25, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) This is an even different error. What's going on?
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/1821823003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/160001
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps180001 (title: "Remove VisibleForTesting where inappropriate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/180001
On 2016/04/19 17:54:52, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1821823003/180001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1821823003/180001 Hi Yaron, would you be able to help me figure out what's going on here? linux_android_rel_ng has failed again with java.lang.NoSuchMethodError: org.chromium.chrome.browser.physicalweb.UrlManager.addUrl. Of course, everything builds and runs fine on my end.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/1821823003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (left): https://codereview.chromium.org/1821823003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:126: @VisibleForTesting Add back the @VisibleForTesting (to the String variant) and I suspect it'll work. SInce it's only failing in release mode that's a clue that it's most likely proguard. What I think is happening is that the string variant is never called from chrome apk so proguard is inlining it. Well, it could be called but it's still deciding to inline it so when the instrumentation test (separate apk) looks for the method, it can't be found. So short term, add @VisibleForTesting, long term, get rid of the overloaded function and hopefully the annotation can go too
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps200001 (title: "Add VisibleForTesting to both addUrl methods")
https://codereview.chromium.org/1821823003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (left): https://codereview.chromium.org/1821823003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:126: @VisibleForTesting On 2016/04/19 22:08:26, Yaron wrote: > Add back the @VisibleForTesting (to the String variant) and I suspect it'll > work. > > SInce it's only failing in release mode that's a clue that it's most likely > proguard. What I think is happening is that the string variant is never called > from chrome apk so proguard is inlining it. Well, it could be called but it's > still deciding to inline it so when the instrumentation test (separate apk) > looks for the method, it can't be found. So short term, add @VisibleForTesting, > long term, get rid of the overloaded function and hopefully the annotation can > go too Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/200001
The CQ bit was checked by cco3@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, yfriedman@chromium.org, mattreynolds@chromium.org Link to the patchset: https://codereview.chromium.org/1821823003/#ps220001 (title: "Add VisibleForTesting to removeUrl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821823003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821823003/220001
Message was sent while issue was closed.
Description was changed from ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 ========== to ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 ========== to ========== Add a UrlInfo class for the Physical Web The UrlManager will store instances of UrlInfo instead of raw strings. This will permit us to include distances when we are supplied with those in the future. BUG=596708 Committed: https://crrev.com/9c74b5484e8ca993ee2cc47b115806e483189b1b Cr-Commit-Position: refs/heads/master@{#388370} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9c74b5484e8ca993ee2cc47b115806e483189b1b Cr-Commit-Position: refs/heads/master@{#388370} |
