Have SimpleCache index listen to android activity status notifications.

Have SimpleCache index listen to android activity status notifications.

Based on earlier work by felipeg, together with some base/android
changes from digit, we now respond to activity status notifications by
greatly increasing the flush latency. This should ensure that we
almost always have a fresh index on android at startup.

R=felipeg@chromium.org, pliard@chromium.org
BUG=233536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198290

[felipeg, 2013-05-03 10:03:00]

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:572: } else if (state ==
base::android::ACTIVITY_STATE_STOPPED) {
I know Philippe told you that STATE_STOPPED is enough here, but I tested with my
phone and it is not enough.

When the app goes to background it always receive a STATE_PAUSE but only
sometimes it receives STATE_STOPPED

Same is valid for when the app is going to get killed.
Sometimes it receives STATE_DESTROYED and sometimes it receives STATE_STOPPED

We should handle all states to be sure we will write the index to disk.

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:168: #if defined(OS_ANDROID)
I know it would mix member vars with functions, but I guess it would be less
polluted if you had only one Ifdef.
Your call

[Philippe, 2013-05-03 10:11:47]

LGTM but to make this work we will need to add an extra call to ActivityStatus
in Main.java (downstream) in order to fire the notification for the STOPPED
event (only PAUSED is hooked currently). I'm preparing a CL fixing this (I
already have a pending CL doing it but with many other things).

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:572: } else if (state ==
base::android::ACTIVITY_STATE_STOPPED) {
On 2013/05/03 10:03:00, felipeg wrote:
> I know Philippe told you that STATE_STOPPED is enough here, but I tested with
my
> phone and it is not enough.
> 
> When the app goes to background it always receive a STATE_PAUSE but only
> sometimes it receives STATE_STOPPED
> 
> Same is valid for when the app is going to get killed.
> Sometimes it receives STATE_DESTROYED and sometimes it receives STATE_STOPPED
> 
> We should handle all states to be sure we will write the index to disk.

STOPPED is supposed to be enough. Its support was added recently to Main.java
but the call to ActivityStatus to actually fire the notification for clients is
missing. I will prepare a CL fixing this right now. Please keep your code as it
is.

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:79: : public
base::SupportsWeakPtr<SimpleIndex>
Nit: the brace was happy there :)

[Philippe, 2013-05-03 10:24:25]

On 2013/05/03 10:11:47, Philippe wrote:
> LGTM but to make this work we will need to add an extra call to ActivityStatus
> in Main.java (downstream) in order to fire the notification for the STOPPED
> event (only PAUSED is hooked currently). I'm preparing a CL fixing this (I
> already have a pending CL doing it but with many other things).
> 
>
https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
> net/disk_cache/simple/simple_index.cc:572: } else if (state ==
> base::android::ACTIVITY_STATE_STOPPED) {
> On 2013/05/03 10:03:00, felipeg wrote:
> > I know Philippe told you that STATE_STOPPED is enough here, but I tested
with
> my
> > phone and it is not enough.
> > 
> > When the app goes to background it always receive a STATE_PAUSE but only
> > sometimes it receives STATE_STOPPED
> > 
> > Same is valid for when the app is going to get killed.
> > Sometimes it receives STATE_DESTROYED and sometimes it receives
STATE_STOPPED
> > 
> > We should handle all states to be sure we will write the index to disk.
> 
> STOPPED is supposed to be enough. Its support was added recently to Main.java
> but the call to ActivityStatus to actually fire the notification for clients
is
> missing. I will prepare a CL fixing this right now. Please keep your code as
it
> is.
> 
>
https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
> File net/disk_cache/simple/simple_index.h (right):
> 
>
https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
> net/disk_cache/simple/simple_index.h:79: : public
> base::SupportsWeakPtr<SimpleIndex>
> Nit: the brace was happy there :)

I think we will have to revert the native ActivityStatus patch unfortunately
until we can fix the flakiness issue (see
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...).

[pasko-google - do not use, 2013-05-03 10:30:54]

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:161:
base::Bind(&SimpleIndex::OnActivityStateChange, AsWeakPtr())),
this operation feels a bit too non-trivial for a constructor, also makes the
formatting ugly, should better do it in Initialize()

https://codereview.chromium.org/14771009/diff/8005/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:385:
base::TimeDelta::FromMilliseconds(delay),
So at maximum it can write the index every 100ms, if an index takes 50ms to
write, it could lead to a noticeable impact on the foreground apps. Can we
measure it?

SimpleCache.IndexTimeSinceLastBackgroundWrite?

[gavinp, 2013-05-03 12:16:53]

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:161:
base::Bind(&SimpleIndex::OnActivityStateChange, AsWeakPtr())),
On 2013/05/03 10:30:54, pasko wrote:
> this operation feels a bit too non-trivial for a constructor, also makes the
> formatting ugly, should better do it in Initialize()

Done.

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:385:
base::TimeDelta::FromMilliseconds(delay),
On 2013/05/03 10:30:54, pasko wrote:
> So at maximum it can write the index every 100ms, if an index takes 50ms to
> write, it could lead to a noticeable impact on the foreground apps. Can we
> measure it?
> 
> SimpleCache.IndexTimeSinceLastBackgroundWrite?

I think in a followup; we currently measure index write time on the IO thread,
which makes getting this right tricky for metrics.

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.h (right):

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
net/disk_cache/simple/simple_index.h:79: : public
base::SupportsWeakPtr<SimpleIndex>
On 2013/05/03 10:11:47, Philippe wrote:
> Nit: the brace was happy there :)

Done.

https://chromiumcodereview.appspot.com/14771009/diff/8005/net/disk_cache/simp...
net/disk_cache/simple/simple_index.h:168: #if defined(OS_ANDROID)
On 2013/05/03 10:03:00, felipeg wrote:
> I know it would mix member vars with functions, but I guess it would be less
> polluted if you had only one Ifdef.
> Your call

Done.

[gavinp, 2013-05-03 14:25:17]

This issue is now waiting on https://codereview.chromium.org/14767011 , and
should go in shortly after that.

Thanks Philippe!

[felipeg, 2013-05-03 18:02:58]

On 2013/05/03 14:25:17, gavinp wrote:
> This issue is now waiting on https://codereview.chromium.org/14767011 , and
> should go in shortly after that.
> 
> Thanks Philippe!

LGTM
please someone remember to add support (in another CL) for the STOPPED action in
the ActivityStatus.java thing, that Philippe said.

[commit-bot: I haz the power, 2013-05-04 04:27:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14771009/16001

[gavinp, 2013-05-04 05:00:29]

Committed patchset #7 manually as r198290 (presubmit successful).

[cbentzel, 2013-05-31 19:55:15]

I wonder if it would be cleaner to have SimpleIndex accessors to specify flush
intervals and have a separate file which does the Android-specific event
listening. 

[And yes, this landed a while ago]...