Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1634213002:
Append late URLs at the end of the nearby URL list (Closed)
|
Created:
4 years, 11 months ago by mattreynolds Modified:
4 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAppend late URLs at the end of the nearby URL list
When ListUrlsActivity is refreshed we take the current list of known nearby
URLs and send it to our URL resolution/metadata service. Previous to this
change, if new URLs were discovered after the initial list was sent, they
would not be sent to the resolution service or displayed in the activity.
This caused buggy behavior where nearby URLs would sometimes not be shown
if they were discovered too late.
With this change, URLs discovered after the initial resolution step will be
appended at the end of the list.
BUG=579099
Committed: https://crrev.com/891564febe894f4305c8df8876616ee8ef34a956
Cr-Commit-Position: refs/heads/master@{#371838}
Patch Set 1 #
Total comments: 6
Patch Set 2 : taking changes from cco3@ #
Total comments: 12
Patch Set 3 : use Collection instead of Set #Patch Set 4 : changes from mmocny@ #
Total comments: 2
Patch Set 5 : expand ternary operators, rename notifyOnUrlListChanged #
Messages
Total messages: 21 (6 generated)
mattreynolds@chromium.org changed reviewers: + cco3@chromium.org
https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:132: super.onDestroy(); this super call should come after other operations https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:72: mObservers = new ObserverList<UrlManagerObserver>(); Just a thought: if we are to go this route, it might make sense to make the notification creator an observer. We could create the UrlManager in PhysicalWeb class that would append the observer. https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManagerObserver.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManagerObserver.java:12: public interface UrlManagerObserver { would this be better as a subinterface of UrlManager? UrlManager.Listener?
https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:132: super.onDestroy(); On 2016/01/26 19:09:56, cco3 wrote: > this super call should come after other operations Done. Also moved the removeObserver call to onStop since onDestroy is not guaranteed to be called. https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:72: mObservers = new ObserverList<UrlManagerObserver>(); On 2016/01/26 19:09:56, cco3 wrote: > Just a thought: if we are to go this route, it might make sense to make the > notification creator an observer. We could create the UrlManager in PhysicalWeb > class that would append the observer. SGTM, I'll create a new CL for this https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManagerObserver.java (right): https://codereview.chromium.org/1634213002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManagerObserver.java:12: public interface UrlManagerObserver { On 2016/01/26 19:09:56, cco3 wrote: > would this be better as a subinterface of UrlManager? UrlManager.Listener? Done.
LGTM
mattreynolds@chromium.org changed reviewers: + newt@chromium.org
Hi newt, PTAL
lgtm after comment https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:417: Set<String> urls = new HashSet<String>(); Is using a Set necessary? i.e. do you need to remove duplicate URLs? If not, I'd use an ArrayList for its smaller memory footprint. HashSets are quite memory-heavy.
mmocny@chromium.org changed reviewers: + mmocny@chromium.org
https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:137: updateNotification(urlCountBefore == 0, urlCountAfter == 0); I like this refactoring. Thanks! https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:138: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); I'm concerned that the dual meaning of urlCount, depending on onboarding state, will cause us issues in the future. Especially since the actual observer event is "onResolvableUrlsAdded" -- we aren't at all supposed to resolve urls if onboarding. I don't see a reason we shouldn't just remove this line for now. https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:218: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); It seems unfortunate that we don't pass the actual PWS Metadata along here. If the Activity is in the foreground, we will: 1. Get an onFound event 2. Resolve via PWS to confirm it is "good" 3. Inform observers that we have a new URL 4. Re-resolve to show in the UI We could solve this by: a. passing along the metadata from step 2 b. building up a PWS cache, but using it only for specific cases. I think a. is a lot easier to start with. https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:417: Set<String> urls = new HashSet<String>(); On 2016/01/26 21:39:04, newt wrote: > Is using a Set necessary? i.e. do you need to remove duplicate URLs? If not, I'd > use an ArrayList for its smaller memory footprint. HashSets are quite > memory-heavy. ..and do you need to re-create this on each iteration? The consumers should not modify the argument, and you can use Collections.unmodifiableSet to enforce?
https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:138: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); On 2016/01/26 22:02:07, mmocny wrote: > I'm concerned that the dual meaning of urlCount, depending on onboarding state, > will cause us issues in the future. Especially since the actual observer event > is "onResolvableUrlsAdded" -- we aren't at all supposed to resolve urls if > onboarding. > > I don't see a reason we shouldn't just remove this line for now. This won't work, consider the case where a URL is found and resolved, then lost. When we find it again the URL will already be in the cached resolved set, so adding it to that set again won't change the size of the display set and we won't trigger the observer event. I agree the use of "urlCount" is confusing and inconsistent. I added a comment and renamed some things to be more explicit about what's being counted, PTAL. https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:218: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); On 2016/01/26 22:02:07, mmocny wrote: > It seems unfortunate that we don't pass the actual PWS Metadata along here. > > If the Activity is in the foreground, we will: > > 1. Get an onFound event > 2. Resolve via PWS to confirm it is "good" > 3. Inform observers that we have a new URL > 4. Re-resolve to show in the UI > > We could solve this by: > a. passing along the metadata from step 2 > b. building up a PWS cache, but using it only for specific cases. > > I think a. is a lot easier to start with. a. wouldn't work in the same case as above (URL found and resolved, then lost, then found again). b. would work if we cached PWS results for resolved URLs. But this adds a lot of complexity for little benefit, and it's nice that UrlManager currently needs to know very little about PwsMetadata. I think we should focus on building this functionality into our PW library. https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:417: Set<String> urls = new HashSet<String>(); On 2016/01/26 22:02:07, mmocny wrote: > On 2016/01/26 21:39:04, newt wrote: > > Is using a Set necessary? i.e. do you need to remove duplicate URLs? If not, > I'd > > use an ArrayList for its smaller memory footprint. HashSets are quite > > memory-heavy. > > ..and do you need to re-create this on each iteration? The consumers should not > modify the argument, and you can use Collections.unmodifiableSet to enforce? Done.
https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:138: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); On 2016/01/27 00:56:18, mattreynolds wrote: > On 2016/01/26 22:02:07, mmocny wrote: > > I'm concerned that the dual meaning of urlCount, depending on onboarding > state, > > will cause us issues in the future. Especially since the actual observer > event > > is "onResolvableUrlsAdded" -- we aren't at all supposed to resolve urls if > > onboarding. > > > > I don't see a reason we shouldn't just remove this line for now. > > This won't work, consider the case where a URL is found and resolved, then lost. > When we find it again the URL will already be in the cached resolved set, so > adding it to that set again won't change the size of the display set and we > won't trigger the observer event. > > I agree the use of "urlCount" is confusing and inconsistent. I added a comment > and renamed some things to be more explicit about what's being counted, PTAL. Okay, that makes sense. We may need a larger refactoring some day, but for now, how about just wrapping with if (!onboarding)and renaming it to notifyResolvableUrlsChanged or some such? https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:156: updateNotification(false, isUrlListEmptyAfter); How come we don't notify on removeUrl? I know the current observes don't care about this case, but we may as well be thorough.. https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:218: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); On 2016/01/27 00:56:18, mattreynolds wrote: > On 2016/01/26 22:02:07, mmocny wrote: > > It seems unfortunate that we don't pass the actual PWS Metadata along here. > > > > If the Activity is in the foreground, we will: > > > > 1. Get an onFound event > > 2. Resolve via PWS to confirm it is "good" > > 3. Inform observers that we have a new URL > > 4. Re-resolve to show in the UI > > > > We could solve this by: > > a. passing along the metadata from step 2 > > b. building up a PWS cache, but using it only for specific cases. > > > > I think a. is a lot easier to start with. > > a. wouldn't work in the same case as above (URL found and resolved, then lost, > then found again). Alright indeed. Perhaps we can just make it an optional value and re-resolve if it is null? > > b. would work if we cached PWS results for resolved URLs. But this adds a lot > of complexity for little benefit, and it's nice that UrlManager currently needs > to know very little about PwsMetadata. I think we should focus on building this > functionality into our PW library. Indeed, I would not add caching in this CL for sure.
On 2016/01/27 02:42:20, mmocny wrote: > https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java > (right): > > https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:138: > notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); > On 2016/01/27 00:56:18, mattreynolds wrote: > > On 2016/01/26 22:02:07, mmocny wrote: > > > I'm concerned that the dual meaning of urlCount, depending on onboarding > > state, > > > will cause us issues in the future. Especially since the actual observer > > event > > > is "onResolvableUrlsAdded" -- we aren't at all supposed to resolve urls if > > > onboarding. > > > > > > I don't see a reason we shouldn't just remove this line for now. > > > > This won't work, consider the case where a URL is found and resolved, then > lost. > > When we find it again the URL will already be in the cached resolved set, so > > adding it to that set again won't change the size of the display set and we > > won't trigger the observer event. > > > > I agree the use of "urlCount" is confusing and inconsistent. I added a > comment > > and renamed some things to be more explicit about what's being counted, PTAL. > > Okay, that makes sense. We may need a larger refactoring some day, but for now, > how about just wrapping with if (!onboarding)and renaming it to > notifyResolvableUrlsChanged or some such? Nevermind. If onboarding the ListView observer would not be registered because the activity would not be in view. This can be refactored some othertime, for now its fine. > > https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:156: > updateNotification(false, isUrlListEmptyAfter); > How come we don't notify on removeUrl? I know the current observes don't care > about this case, but we may as well be thorough.. > > https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:218: > notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); > On 2016/01/27 00:56:18, mattreynolds wrote: > > On 2016/01/26 22:02:07, mmocny wrote: > > > It seems unfortunate that we don't pass the actual PWS Metadata along here. > > > > > > If the Activity is in the foreground, we will: > > > > > > 1. Get an onFound event > > > 2. Resolve via PWS to confirm it is "good" > > > 3. Inform observers that we have a new URL > > > 4. Re-resolve to show in the UI > > > > > > We could solve this by: > > > a. passing along the metadata from step 2 > > > b. building up a PWS cache, but using it only for specific cases. > > > > > > I think a. is a lot easier to start with. > > > > a. wouldn't work in the same case as above (URL found and resolved, then lost, > > then found again). > > Alright indeed. Perhaps we can just make it an optional value and re-resolve if > it is null? Changed my mind, its more important to leave this readable until we add caching. The only time we do a double resolve is if you find new-never-before-seen-beacons while also having PW Activity open *and* having an active scan window running. That's a slim enough case that its not worth hacking in a temporary workaround. > > > > > b. would work if we cached PWS results for resolved URLs. But this adds a lot > > of complexity for little benefit, and it's nice that UrlManager currently > needs > > to know very little about PwsMetadata. I think we should focus on building > this > > functionality into our PW library. > > Indeed, I would not add caching in this CL for sure.
lgtm LGTM with the few nits left. Good changes, thanks for this! https://codereview.chromium.org/1634213002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:136: int notificationUrlsBefore = isOnboarding ? nearbyUrls.size() : displayableUrlsBefore; Nit: this use of ternary operator is just making this harder to read at this point. I think its time to just: int displayableUrlsBefore, notificationUrlsBefore; if (isOnboarding) { ... } else { ... }
https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:138: notifyOnUrlListChanged(urlCountBefore, urlCountAfter, url); On 2016/01/27 02:42:19, mmocny wrote: > On 2016/01/27 00:56:18, mattreynolds wrote: > > On 2016/01/26 22:02:07, mmocny wrote: > > > I'm concerned that the dual meaning of urlCount, depending on onboarding > > state, > > > will cause us issues in the future. Especially since the actual observer > > event > > > is "onResolvableUrlsAdded" -- we aren't at all supposed to resolve urls if > > > onboarding. > > > > > > I don't see a reason we shouldn't just remove this line for now. > > > > This won't work, consider the case where a URL is found and resolved, then > lost. > > When we find it again the URL will already be in the cached resolved set, so > > adding it to that set again won't change the size of the display set and we > > won't trigger the observer event. > > > > I agree the use of "urlCount" is confusing and inconsistent. I added a > comment > > and renamed some things to be more explicit about what's being counted, PTAL. > > Okay, that makes sense. We may need a larger refactoring some day, but for now, > how about just wrapping with if (!onboarding)and renaming it to > notifyResolvableUrlsChanged or some such? I renamed it but didn't add the onboarding check (it should be a no-op in onboarding anyway). https://codereview.chromium.org/1634213002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/1634213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:136: int notificationUrlsBefore = isOnboarding ? nearbyUrls.size() : displayableUrlsBefore; On 2016/01/27 03:01:54, mmocny wrote: > Nit: this use of ternary operator is just making this harder to read at this > point. I think its time to just: > > int displayableUrlsBefore, notificationUrlsBefore; > if (isOnboarding) { > ... > } else { > ... > } Done.
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org, newt@chromium.org, mmocny@chromium.org Link to the patchset: https://codereview.chromium.org/1634213002/#ps80001 (title: "expand ternary operators, rename notifyOnUrlListChanged")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1634213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1634213002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Append late URLs at the end of the nearby URL list When ListUrlsActivity is refreshed we take the current list of known nearby URLs and send it to our URL resolution/metadata service. Previous to this change, if new URLs were discovered after the initial list was sent, they would not be sent to the resolution service or displayed in the activity. This caused buggy behavior where nearby URLs would sometimes not be shown if they were discovered too late. With this change, URLs discovered after the initial resolution step will be appended at the end of the list. BUG=579099 ========== to ========== Append late URLs at the end of the nearby URL list When ListUrlsActivity is refreshed we take the current list of known nearby URLs and send it to our URL resolution/metadata service. Previous to this change, if new URLs were discovered after the initial list was sent, they would not be sent to the resolution service or displayed in the activity. This caused buggy behavior where nearby URLs would sometimes not be shown if they were discovered too late. With this change, URLs discovered after the initial resolution step will be appended at the end of the list. BUG=579099 Committed: https://crrev.com/891564febe894f4305c8df8876616ee8ef34a956 Cr-Commit-Position: refs/heads/master@{#371838} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/891564febe894f4305c8df8876616ee8ef34a956 Cr-Commit-Position: refs/heads/master@{#371838} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
