Add a cache for recently viewed webpages.

Add a cache for recently viewed webpages.

This allows us to avoid re-parsing metadata in certain cases.

BUG=693650
Review-Url: https://codereview.chromium.org/2820453002
Cr-Commit-Position: refs/heads/master@{#464577}
Committed: https://chromium.googlesource.com/chromium/src/+/9cccac38eeff5383d850ca65039f89b1091cbeeb

[dproctor, 2017-04-13 05:34:54]

Description was changed from

==========
Add a cache for recently viewed webpages. This allows us to avoid re-parsing
metadata in certain cases.
BUG=
==========

to

==========
Add a cache for recently viewed webpages. This allows us to avoid re-parsing
metadata in certain cases.
BUG=
==========

[wychen, 2017-04-13 07:05:00]

wychen@chromium.org changed reviewers:
+ wychen@chromium.org

[wychen, 2017-04-13 07:05:01]

Also add BUG=693633

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:28: //
the visit was in the last CACHE_VISIT_CUTOFF_MS milliseconds, then we don't
parse the page,
and the *parsing* was in the last ...

Our expiration is based on parsing, not visiting, right?

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:47: //
There are three conditions that can occur with respect to the cache.
We might want to add UMA to track the distribution of these conditions.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:87: //
visited.
it was *parsed*

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:77:
verify(mCopylessPaste).getEntities(any(CopylessPaste.GetEntitiesResponse.class));
verify(mReporter).reportWebPageView(any(WebPage.class))?

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:87:
verify(mReporter).reportWebPageView(eq("http://www.test.com"), eq("My neat
website"));
Do we want a verifyNoMoreInteractions(mCopylessPaste) to make sure
mCopylessPaste doesn't get used again when cache hit?

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:116:
verify(mReporter, never()).reportWebPageView(any(String.class),
any(String.class));
Probably verifyZeroInteractions(mReporter) to catch all.

[dproctor, 2017-04-13 17:23:10]

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:28: //
the visit was in the last CACHE_VISIT_CUTOFF_MS milliseconds, then we don't
parse the page,
On 2017/04/13 07:05:00, wychen wrote:
> and the *parsing* was in the last ...
> 
> Our expiration is based on parsing, not visiting, right?

Thanks, yeah, the comparison is made based on the last time the page was parsed,
but cache eviction is based on the last visit time.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:87: //
visited.
On 2017/04/13 07:05:00, wychen wrote:
> it was *parsed*

Done.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/04/13 07:05:01, wychen wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:77:
verify(mCopylessPaste).getEntities(any(CopylessPaste.GetEntitiesResponse.class));
On 2017/04/13 07:05:00, wychen wrote:
> verify(mReporter).reportWebPageView(any(WebPage.class))?

Done.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:87:
verify(mReporter).reportWebPageView(eq("http://www.test.com"), eq("My neat
website"));
On 2017/04/13 07:05:00, wychen wrote:
> Do we want a verifyNoMoreInteractions(mCopylessPaste) to make sure
> mCopylessPaste doesn't get used again when cache hit?

Done.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:116:
verify(mReporter, never()).reportWebPageView(any(String.class),
any(String.class));
On 2017/04/13 07:05:01, wychen wrote:
> Probably verifyZeroInteractions(mReporter) to catch all.

Done.

[dproctor, 2017-04-13 17:34:58]

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:47: //
There are three conditions that can occur with respect to the cache.
On 2017/04/13 07:05:00, wychen wrote:
> We might want to add UMA to track the distribution of these conditions.

How about we add this in a follow-up cl?

[wychen, 2017-04-13 18:03:56]

Description was changed from

==========
Add a cache for recently viewed webpages. This allows us to avoid re-parsing
metadata in certain cases.
BUG=
==========

to

==========
Add a cache for recently viewed webpages.

This allows us to avoid re-parsing metadata in certain cases.

BUG=693650
==========

[wychen, 2017-04-13 18:04:45]

wychen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[wychen, 2017-04-13 18:04:46]

Maria, could you take a look?

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:47: //
There are three conditions that can occur with respect to the cache.
On 2017/04/13 17:34:58, dproctor wrote:
> On 2017/04/13 07:05:00, wychen wrote:
> > We might want to add UMA to track the distribution of these conditions.
> 
> How about we add this in a follow-up cl?

No problem.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:77:
verify(mCopylessPaste).getEntities(any(CopylessPaste.GetEntitiesResponse.class));
On 2017/04/13 17:23:09, dproctor wrote:
> On 2017/04/13 07:05:00, wychen wrote:
> > verify(mReporter).reportWebPageView(any(WebPage.class))?
> 
> Done.

Sorry, I meant keeping both.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:87:
verify(mReporter).reportWebPageView(eq("http://www.test.com"), eq("My neat
website"));
On 2017/04/13 17:23:09, dproctor wrote:
> On 2017/04/13 07:05:00, wychen wrote:
> > Do we want a verifyNoMoreInteractions(mCopylessPaste) to make sure
> > mCopylessPaste doesn't get used again when cache hit?
> 
> Done.

Why removing verify(mCopylessPaste)? We still want to make sure it is called
exactly once, right?

[dproctor, 2017-04-13 18:12:24]

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java
(right):

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:77:
verify(mCopylessPaste).getEntities(any(CopylessPaste.GetEntitiesResponse.class));
On 2017/04/13 18:04:45, wychen wrote:
> On 2017/04/13 17:23:09, dproctor wrote:
> > On 2017/04/13 07:05:00, wychen wrote:
> > > verify(mReporter).reportWebPageView(any(WebPage.class))?
> > 
> > Done.
> 
> Sorry, I meant keeping both.

Done.

https://codereview.chromium.org/2820453002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/AppIndexingUtilTest.java:87:
verify(mReporter).reportWebPageView(eq("http://www.test.com"), eq("My neat
website"));
On 2017/04/13 18:04:45, wychen wrote:
> On 2017/04/13 17:23:09, dproctor wrote:
> > On 2017/04/13 07:05:00, wychen wrote:
> > > Do we want a verifyNoMoreInteractions(mCopylessPaste) to make sure
> > > mCopylessPaste doesn't get used again when cache hit?
> > 
> > Done.
> 
> Why removing verify(mCopylessPaste)? We still want to make sure it is called
> exactly once, right?

Done.

[Maria, 2017-04-13 18:29:11]

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:37:
mPageCache = new LruCache<String, CacheEntry>(cacheSize);
This will likely pre-allocate some memory for the cache. Could you instantiate
the cache lazily instead so that low end devices don't end up using memory for a
feature they don't use?

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:40:
public void extractCopylessPasteMetadata(final Tab tab) {
this should have javadoc

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:80: if
(entry == null || getElapsedTime() - entry.lastSeenTimeMs >
CACHE_VISIT_CUTOFF_MS) {
parenthesis around the second expression

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:87: //
parsed.
javadoc instead of inline doc

[dproctor, 2017-04-13 18:53:35]

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:37:
mPageCache = new LruCache<String, CacheEntry>(cacheSize);
On 2017/04/13 18:29:11, Maria wrote:
> This will likely pre-allocate some memory for the cache. Could you instantiate
> the cache lazily instead so that low end devices don't end up using memory for
a
> feature they don't use?

Good call, done.

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:40:
public void extractCopylessPasteMetadata(final Tab tab) {
On 2017/04/13 18:29:11, Maria wrote:
> this should have javadoc

Done.

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:80: if
(entry == null || getElapsedTime() - entry.lastSeenTimeMs >
CACHE_VISIT_CUTOFF_MS) {
On 2017/04/13 18:29:11, Maria wrote:
> parenthesis around the second expression

Done.

https://codereview.chromium.org/2820453002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:87: //
parsed.
On 2017/04/13 18:29:11, Maria wrote:
> javadoc instead of inline doc

Done.

[Maria, 2017-04-13 18:58:34]

lgtm

https://codereview.chromium.org/2820453002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:139:
private LruCache<String, CacheEntry> getPageCache() {
nit: move above class definition

[dproctor, 2017-04-13 19:50:34]

https://codereview.chromium.org/2820453002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java
(right):

https://codereview.chromium.org/2820453002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/AppIndexingUtil.java:139:
private LruCache<String, CacheEntry> getPageCache() {
On 2017/04/13 18:58:34, Maria wrote:
> nit: move above class definition

Done.

[dproctor, 2017-04-13 20:47:35]

The CQ bit was checked by dproctor@google.com

[dproctor, 2017-04-13 20:47:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2820453002/#ps80001
(title: "move method")

[commit-bot: I haz the power, 2017-04-13 20:48:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 22:35:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492116455751530,
"parent_rev": "d85987aa5fdd5785787516c6b30c3e98844cab3e", "commit_rev":
"9cccac38eeff5383d850ca65039f89b1091cbeeb"}

[commit-bot: I haz the power, 2017-04-13 22:36:42]

Description was changed from

==========
Add a cache for recently viewed webpages.

This allows us to avoid re-parsing metadata in certain cases.

BUG=693650
==========

to

==========
Add a cache for recently viewed webpages.

This allows us to avoid re-parsing metadata in certain cases.

BUG=693650

Review-Url: https://codereview.chromium.org/2820453002
Cr-Commit-Position: refs/heads/master@{#464577}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cccac38eeff5383d850ca65039f...
==========

[commit-bot: I haz the power, 2017-04-13 22:36:42]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9cccac38eeff5383d850ca65039f...