Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322073003:
Cache PWS Results (Closed)
|
Created:
4 years, 3 months ago by hayesjordan Modified:
4 years, 3 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache Physical Web Service results
To reduce lantency when other parties request Physical Web data,
Physical Web Service (PWS) results need to be cached. Otherwise the
Physical Web would have to make another request to the PWS server to get
the data again.
BUG=636490
Committed: https://crrev.com/aaed874c386983f2d60e235ec6b3314e77474f66
Cr-Commit-Position: refs/heads/master@{#418008}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Address cco3 comments #Patch Set 3 : Update UrlManagerTests #
Messages
Total messages: 21 (6 generated)
hayesjordan@google.com changed reviewers: + cco3@chromium.org
Description was changed from ========== Cache PWS Results Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 ========== to ========== Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 ==========
https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:76: private final Map<String, PwsResult> mPwsResults; mPwsResultMap https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:250: public Set<String> getResolvedUrls() { Let's not use this convenience method in this class. It'd be better to actually use containsKey() etc. instead of creating a key set and then examining it. It'd be nice if we could get rid of it altogether at some point. It's needed in the diagnostics page, but there's probably something more direct we could provide it. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:254: public PwsResult getPwsResultByUrl(String url) { Why is this necessary? Why is it public? https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { Why is urlInfo needed? https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:332: if (prefs.getInt(PREFS_VERSION_KEY, 0) != PREFS_VERSION) { Increment the version number. If the version is old, remove physicalweb_resolved_urls. Add a comment that this cleanup code can be deleted in m57. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:384: private void setPwsResultCollectionInSharedPreferences( This isn't needed. Just put the code directly in putCachedPwsResults https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:412: private void putCachedPwsResults() { putCachedPwsResultMap()
https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:254: public PwsResult getPwsResultByUrl(String url) { On 2016/09/08 23:47:26, cco3 wrote: > Why is this necessary? Why is it public? My assumption was that when a URL was found, a listener would want to call this method to get the meta data for that URL. Should it be removed until needed? https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { On 2016/09/08 23:47:26, cco3 wrote: > Why is urlInfo needed? It is for registerNewDisplayableUrl. This method should only get called when adding the URL and meta data to the map. Is there another way this should be handled? https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:384: private void setPwsResultCollectionInSharedPreferences( On 2016/09/08 23:47:26, cco3 wrote: > This isn't needed. Just put the code directly in putCachedPwsResults Should I do the same with setUrlInfoCollectionInSharedPreferences and put it in putCachedUrlInfoMap?
https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:254: public PwsResult getPwsResultByUrl(String url) { On 2016/09/09 00:44:52, hayesjordan wrote: > On 2016/09/08 23:47:26, cco3 wrote: > > Why is this necessary? Why is it public? > > My assumption was that when a URL was found, a listener would want to call this > method to get the meta data for that URL. Should it be removed until needed? Yea, remove for now...I'm not certain we'll access it like this. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { On 2016/09/09 00:44:52, hayesjordan wrote: > On 2016/09/08 23:47:26, cco3 wrote: > > Why is urlInfo needed? > > It is for registerNewDisplayableUrl. This method should only get called when > adding the URL and meta data to the map. Is there another way this should be > handled? You could just lookup the urlinfo in the map https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:384: private void setPwsResultCollectionInSharedPreferences( On 2016/09/09 00:44:52, hayesjordan wrote: > On 2016/09/08 23:47:26, cco3 wrote: > > This isn't needed. Just put the code directly in putCachedPwsResults > > Should I do the same with setUrlInfoCollectionInSharedPreferences and put it in > putCachedUrlInfoMap? yes, thanks
https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { On 2016/09/09 01:04:41, cco3 wrote: > On 2016/09/09 00:44:52, hayesjordan wrote: > > On 2016/09/08 23:47:26, cco3 wrote: > > > Why is urlInfo needed? > > > > It is for registerNewDisplayableUrl. This method should only get called when > > adding the URL and meta data to the map. Is there another way this should be > > handled? > > You could just lookup the urlinfo in the map What happens when the UrlInfo is no longer mapped to the requestUrl?
Please also update our caching tests https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { On 2016/09/09 01:18:58, hayesjordan wrote: > On 2016/09/09 01:04:41, cco3 wrote: > > On 2016/09/09 00:44:52, hayesjordan wrote: > > > On 2016/09/08 23:47:26, cco3 wrote: > > > > Why is urlInfo needed? > > > > > > It is for registerNewDisplayableUrl. This method should only get called when > > > adding the URL and meta data to the map. Is there another way this should be > > > handled? > > > > You could just lookup the urlinfo in the map > > What happens when the UrlInfo is no longer mapped to the requestUrl? If it's no longer mapped to the requestUrl, then it shouldn't be displayble.
https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:76: private final Map<String, PwsResult> mPwsResults; On 2016/09/08 23:47:26, cco3 wrote: > mPwsResultMap Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:250: public Set<String> getResolvedUrls() { On 2016/09/08 23:47:26, cco3 wrote: > Let's not use this convenience method in this class. It'd be better to actually > use containsKey() etc. instead of creating a key set and then examining it. > > It'd be nice if we could get rid of it altogether at some point. It's needed in > the diagnostics page, but there's probably something more direct we could > provide it. Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:254: public PwsResult getPwsResultByUrl(String url) { On 2016/09/09 01:04:41, cco3 wrote: > On 2016/09/09 00:44:52, hayesjordan wrote: > > On 2016/09/08 23:47:26, cco3 wrote: > > > Why is this necessary? Why is it public? > > > > My assumption was that when a URL was found, a listener would want to call > this > > method to get the meta data for that URL. Should it be removed until needed? > > Yea, remove for now...I'm not certain we'll access it like this. Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { On 2016/09/09 16:43:20, cco3 wrote: > On 2016/09/09 01:18:58, hayesjordan wrote: > > On 2016/09/09 01:04:41, cco3 wrote: > > > On 2016/09/09 00:44:52, hayesjordan wrote: > > > > On 2016/09/08 23:47:26, cco3 wrote: > > > > > Why is urlInfo needed? > > > > > > > > It is for registerNewDisplayableUrl. This method should only get called > when > > > > adding the URL and meta data to the map. Is there another way this should > be > > > > handled? > > > > > > You could just lookup the urlinfo in the map > > > > What happens when the UrlInfo is no longer mapped to the requestUrl? > > If it's no longer mapped to the requestUrl, then it shouldn't be displayble. Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:332: if (prefs.getInt(PREFS_VERSION_KEY, 0) != PREFS_VERSION) { On 2016/09/08 23:47:26, cco3 wrote: > Increment the version number. If the version is old, remove > physicalweb_resolved_urls. Add a comment that this cleanup code can be deleted > in m57. Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:384: private void setPwsResultCollectionInSharedPreferences( On 2016/09/09 01:04:41, cco3 wrote: > On 2016/09/09 00:44:52, hayesjordan wrote: > > On 2016/09/08 23:47:26, cco3 wrote: > > > This isn't needed. Just put the code directly in putCachedPwsResults > > > > Should I do the same with setUrlInfoCollectionInSharedPreferences and put it > in > > putCachedUrlInfoMap? > > yes, thanks Done. https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:412: private void putCachedPwsResults() { On 2016/09/08 23:47:26, cco3 wrote: > putCachedPwsResultMap() Done.
On 2016/09/09 19:09:59, hayesjordan wrote: > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java > (right): > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:76: > private final Map<String, PwsResult> mPwsResults; > On 2016/09/08 23:47:26, cco3 wrote: > > mPwsResultMap > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:250: > public Set<String> getResolvedUrls() { > On 2016/09/08 23:47:26, cco3 wrote: > > Let's not use this convenience method in this class. It'd be better to > actually > > use containsKey() etc. instead of creating a key set and then examining it. > > > > It'd be nice if we could get rid of it altogether at some point. It's needed > in > > the diagnostics page, but there's probably something more direct we could > > provide it. > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:254: > public PwsResult getPwsResultByUrl(String url) { > On 2016/09/09 01:04:41, cco3 wrote: > > On 2016/09/09 00:44:52, hayesjordan wrote: > > > On 2016/09/08 23:47:26, cco3 wrote: > > > > Why is this necessary? Why is it public? > > > > > > My assumption was that when a URL was found, a listener would want to call > > this > > > method to get the meta data for that URL. Should it be removed until needed? > > > > Yea, remove for now...I'm not certain we'll access it like this. > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:303: > private void addResolvedUrl(UrlInfo urlInfo, PwsResult pwsResult) { > On 2016/09/09 16:43:20, cco3 wrote: > > On 2016/09/09 01:18:58, hayesjordan wrote: > > > On 2016/09/09 01:04:41, cco3 wrote: > > > > On 2016/09/09 00:44:52, hayesjordan wrote: > > > > > On 2016/09/08 23:47:26, cco3 wrote: > > > > > > Why is urlInfo needed? > > > > > > > > > > It is for registerNewDisplayableUrl. This method should only get called > > when > > > > > adding the URL and meta data to the map. Is there another way this > should > > be > > > > > handled? > > > > > > > > You could just lookup the urlinfo in the map > > > > > > What happens when the UrlInfo is no longer mapped to the requestUrl? > > > > If it's no longer mapped to the requestUrl, then it shouldn't be displayble. > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:332: > if (prefs.getInt(PREFS_VERSION_KEY, 0) != PREFS_VERSION) { > On 2016/09/08 23:47:26, cco3 wrote: > > Increment the version number. If the version is old, remove > > physicalweb_resolved_urls. Add a comment that this cleanup code can be > deleted > > in m57. > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:384: > private void setPwsResultCollectionInSharedPreferences( > On 2016/09/09 01:04:41, cco3 wrote: > > On 2016/09/09 00:44:52, hayesjordan wrote: > > > On 2016/09/08 23:47:26, cco3 wrote: > > > > This isn't needed. Just put the code directly in putCachedPwsResults > > > > > > Should I do the same with setUrlInfoCollectionInSharedPreferences and put it > > in > > > putCachedUrlInfoMap? > > > > yes, thanks > > Done. > > https://codereview.chromium.org/2322073003/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:412: > private void putCachedPwsResults() { > On 2016/09/08 23:47:26, cco3 wrote: > > putCachedPwsResultMap() > > Done. Update tests please.
Updated tests
On 2016/09/09 22:43:17, hayesjordan wrote: > Updated tests LGTM
cco3@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm
The CQ bit was checked by hayesjordan@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 ========== to ========== Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 ========== to ========== Cache Physical Web Service results To reduce lantency when other parties request Physical Web data, Physical Web Service (PWS) results need to be cached. Otherwise the Physical Web would have to make another request to the PWS server to get the data again. BUG=636490 Committed: https://crrev.com/aaed874c386983f2d60e235ec6b3314e77474f66 Cr-Commit-Position: refs/heads/master@{#418008} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aaed874c386983f2d60e235ec6b3314e77474f66 Cr-Commit-Position: refs/heads/master@{#418008} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
