Store Physical Web URL data in memory

Store Physical Web URL data in memory

Instead of repeatedly reading and deserializing nearby URLs and
resolved URLs, this change keeps them in memory.  (There are not
many URLs stored at one time, so memory shouldn't be an issue).
This also paves the way for implementing a smarter cache in the
UrlManager.

BUG=598106, 596668
Committed: https://crrev.com/3cb9951a5aa2edb978f434f31221b00bbc631ba0
Cr-Commit-Position: refs/heads/master@{#392671}

[cco3, 2016-04-20 00:33:26]

cco3@chromium.org changed reviewers:
+ mattreynolds@chromium.org

[mattreynolds, 2016-05-04 19:54:08]

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:76:
private Map<String, UrlInfo> mAllUrls;
How do new URLs get added to mAllUrls?

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:230:
Set<String> intersection = new HashSet<>(mNearbyUrls);
I think we can use an ArrayList for intersection since we don't rely on any
Set-specific behavior.

[cco3, 2016-05-06 23:13:41]

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:76:
private Map<String, UrlInfo> mAllUrls;
On 2016/05/04 19:54:08, mattreynolds wrote:
> How do new URLs get added to mAllUrls?

Done.

https://codereview.chromium.org/1894073006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:230:
Set<String> intersection = new HashSet<>(mNearbyUrls);
On 2016/05/04 19:54:08, mattreynolds wrote:
> I think we can use an ArrayList for intersection since we don't rely on any
> Set-specific behavior.

I assumed sets would be better for retainAll.

[mattreynolds, 2016-05-06 23:38:10]

lgtm

[cco3, 2016-05-06 23:40:05]

cco3@chromium.org changed reviewers:
+ nyquist@chromium.org

[mattreynolds, 2016-05-09 18:48:58]

lgtm

[cco3, 2016-05-09 23:02:44]

Hi nyquist@, would you be able to take a look at this change?

[nyquist, 2016-05-10 00:00:31]

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:75:
private Set<String> mNearbyUrls;
Could these be final? If so, remember to move them above the non-final members,
if there are any.

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:141:
public void addUrl(UrlInfo urlInfo) {
Could you add a test for this and removeUrl and their respective getFoo-methods?

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:201:
mNearbyUrls.remove(urlInfo.getUrl());
Should this also be removed from the mUrlInfoMap?

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:304:
if (prefsVersion != PREFS_VERSION) {
Can you add a test for this upgrade path?

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:352:
private void setStringSetInSharedPreferences(String preferenceName, Set<String>
urls) {
Optional nit: |urls| seem like a strange name when you look at the method name.
Just |strings| maybe?

[cco3, 2016-05-10 17:23:02]

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:75:
private Set<String> mNearbyUrls;
On 2016/05/10 00:00:31, nyquist wrote:
> Could these be final? If so, remember to move them above the non-final
members,
> if there are any.

Done.

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:141:
public void addUrl(UrlInfo urlInfo) {
On 2016/05/10 00:00:31, nyquist wrote:
> Could you add a test for this and removeUrl and their respective
getFoo-methods?

I already have several tests for add and remove.  Is there some specific
functionality you think I might be missing?

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:201:
mNearbyUrls.remove(urlInfo.getUrl());
On 2016/05/10 00:00:31, nyquist wrote:
> Should this also be removed from the mUrlInfoMap?

No, mUrlInfoMap is to be a cache that has a superset of mNearbyUrls.  Right now,
we clear it everytime you startup chrome.  There's a followup change after this
that will stop doing that and manage the cache more reasonably.

https://codereview.chromium.org/1894073006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:304:
if (prefsVersion != PREFS_VERSION) {
On 2016/05/10 00:00:31, nyquist wrote:
> Can you add a test for this upgrade path?

None of this is new code, just reorganized.  I'll send you a distinct change.

[nyquist, 2016-05-10 17:57:59]

lgtm

[cco3, 2016-05-10 18:00:28]

The CQ bit was checked by cco3@chromium.org

[cco3, 2016-05-10 18:00:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/1894073006/#ps60001
(title: "Make final members final")

[commit-bot: I haz the power, 2016-05-10 18:01:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1894073006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1894073006/60001

[commit-bot: I haz the power, 2016-05-10 19:09:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-10 19:11:16]

Description was changed from

==========
Store Physical Web URL data in memory

Instead of repeatedly reading and deserializing nearby URLs and
resolved URLs, this change keeps them in memory.  (There are not
many URLs stored at one time, so memory shouldn't be an issue).
This also paves the way for implementing a smarter cache in the
UrlManager.

BUG=598106,596668
==========

to

==========
Store Physical Web URL data in memory

Instead of repeatedly reading and deserializing nearby URLs and
resolved URLs, this change keeps them in memory.  (There are not
many URLs stored at one time, so memory shouldn't be an issue).
This also paves the way for implementing a smarter cache in the
UrlManager.

BUG=598106,596668

Committed: https://crrev.com/3cb9951a5aa2edb978f434f31221b00bbc631ba0
Cr-Commit-Position: refs/heads/master@{#392671}
==========

[commit-bot: I haz the power, 2016-05-10 19:11:16]

Patchset 4 (id:??) landed as
https://crrev.com/3cb9951a5aa2edb978f434f31221b00bbc631ba0
Cr-Commit-Position: refs/heads/master@{#392671}