Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14362009:
Receive app notifications in SimpleCache, so we save our index file (Closed)
|
Created:
7 years, 8 months ago by felipeg Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWith this CL we can catch OnResume, OnPause, OnDestroy and OnStop notifications in SimpleCache, so we save our index before we get killed.
OnPause means the app lost focus (went to the background).
The idea is that we can write the index file to disk on every OnPause, and while the app is in the background we can aggressively increase the frequency we write the index file to disk.
BUG=233536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195845
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : sync #
Total comments: 20
Patch Set 7 : gavins comments #
Total comments: 38
Patch Set 8 : philippe comments #
Total comments: 44
Patch Set 9 : #Patch Set 10 : #
Total comments: 1
Messages
Total messages: 18 (0 generated)
another crazy idea guys
On 2013/04/18 22:09:09, felipeg1 wrote: > another crazy idea guys btw it works. It is running on my phone now
Nice! But what are the odds of landing this? I suspect fast shutdown is important.
Yes, we can land this. We just need an LGTM from Gavin, he is an owner in net/ We should have a LGTM from Philippe too, since he is the expert in this kind of notifications on Android. David can help as well.
Awesome. Do not land this without a review from Philippe on the android changes at least. https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... File net/android/simple_cache_activity_status_notifier.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.cc:41: if (callback_runner_.get() && !notify_callback_.is_null()) { Lose this .get() https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... File net/android/simple_cache_activity_status_notifier.h (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.h:26: class SimpleCacheActivityStatusNotifier { #if defined(OS_ANDROID) https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.h:47: #endif // defined(OS_ANDROID) (and probably the same around every #include) This is a pattern used quite a bit in Blink. The end result is that we no longer need most of our #if defined(...) over in the simple cache code. I'm not sure this is the best plan, it hides the platform specialization. OTOH it cuts a lot of cruft out of the simple cache code, it's hard to read through #ifdefs. Wait to hear from Philippe on this too. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_entry_impl.cc (left): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_entry_impl.cc:337: if (index_) Same here? https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_entry_impl.cc:66: // See comment on OpenEntry regarding inserting the entry in the index. This is from another CL, it should be removed here, right? https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.cc (left): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:28: // WriteToDisk at lest every 5 minutes. Did this really land without the spelling correction? https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:29: const int kMaxWriteToDiskDelaySecs = 300; Yeah, I guess we can lose this if we don't expect stale cache to be the norm. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:261: file_util::Delete(index_filename, /* recursive = */ false); The changes in here seem unrelated. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:225: // When the app is in the background we can afford to write the index much "can afford" or "do" ? We're just writing it more often in order to not risk death, right? https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.h:149: void ActivityStatusChanged(int activity_status); I'm not a fan of moving the activity_status around in an int. Why did you choose this vs an enum? To avoid having the mapping defined in the object in net/android? To avoid a larger #if defined() region?
https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... File net/android/simple_cache_activity_status_notifier.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.cc:41: if (callback_runner_.get() && !notify_callback_.is_null()) { On 2013/04/19 15:58:55, gavinp wrote: > Lose this .get() Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_entry_impl.cc (left): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_entry_impl.cc:337: if (index_) On 2013/04/19 15:58:55, gavinp wrote: > Same here? Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_entry_impl.cc:66: // See comment on OpenEntry regarding inserting the entry in the index. On 2013/04/19 15:58:55, gavinp wrote: > This is from another CL, it should be removed here, right? Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.cc (left): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:28: // WriteToDisk at lest every 5 minutes. On 2013/04/19 15:58:55, gavinp wrote: > Did this really land without the spelling correction? Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:29: const int kMaxWriteToDiskDelaySecs = 300; On 2013/04/19 15:58:55, gavinp wrote: > Yeah, I guess we can lose this if we don't expect stale cache to be the norm. Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:261: file_util::Delete(index_filename, /* recursive = */ false); On 2013/04/19 15:58:55, gavinp wrote: > The changes in here seem unrelated. I realized we should not delete the file, this just adds unnecessary IO . It is related because this CL is increasing the IO on the case the app is in the background. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.cc:225: // When the app is in the background we can afford to write the index much On 2013/04/19 15:58:55, gavinp wrote: > "can afford" or "do" ? We're just writing it more often in order to not risk > death, right? Done. https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/disk_cache/simple... net/disk_cache/simple/simple_index.h:149: void ActivityStatusChanged(int activity_status); On 2013/04/19 15:58:55, gavinp wrote: > I'm not a fan of moving the activity_status around in an int. Why did you choose > this vs an enum? To avoid having the mapping defined in the object in > net/android? To avoid a larger #if defined() region? lets do the enum
https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... File net/android/simple_cache_activity_status_notifier.h (right): https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.h:26: class SimpleCacheActivityStatusNotifier { On 2013/04/19 15:58:55, gavinp wrote: > #if defined(OS_ANDROID) This file is only used on android anyway. It makes no sense to have ifdef here. See other files under net/android/ none have ifdef OS_ANDROID. I can explain you later https://chromiumcodereview.appspot.com/14362009/diff/19/net/android/simple_ca... net/android/simple_cache_activity_status_notifier.h:47: On 2013/04/19 15:58:55, gavinp wrote: > #endif // defined(OS_ANDROID) > > (and probably the same around every #include) > > This is a pattern used quite a bit in Blink. > > The end result is that we no longer need most of our #if defined(...) over in > the simple cache code. I'm not sure this is the best plan, it hides the platform > specialization. OTOH it cuts a lot of cruft out of the simple cache code, it's > hard to read through #ifdefs. > > Wait to hear from Philippe on this too. AFAICT this is not how it is done on android. I upstreamed a bunch of files that followed the pattern I am using here. Philippe knows better
Thanks Felipe! See my comment about the threading issue. We can discuss this offline to see how you can address it. https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:15: * Used by the SimpleIndex in net/disk_cache/simple/ to listens to changes in Nit: the char limit is 100 in Java. https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:18: // extends BroadcastReceiver You can remove this line. https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:22: static public SimpleCacheActivityStatusNotifier NewInstance(int nativePtr) { Nit: s/NewInstance/newInstance https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:23: return new SimpleCacheActivityStatusNotifier(nativePtr); Nit: 4 space indent in Java (here and in a few other places). https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:26: public SimpleCacheActivityStatusNotifier(int nativePtr) { You can make this constructor private since this class is supposed to be instantiated through the static factory method above. https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:35: state == ActivityStatus.PAUSED || You can remove support for the PAUSED and DESTROYED state. See http://developer.android.com/training/basics/activity-lifecycle/pausing.html. https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:47: private int mNativePtr = 0; Nit: attributes go first usually in Java. You can also make it 'final' if you are sure that this pointer never needs to be reset (i.e. if the native side outlives the Java side). https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... File net/android/simple_cache_activity_status_notifier.cc (right): https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.cc:26: // TODO(felipeg): Do we have to call DetachFromVM somewhere ? I don't see No, you can remove this comment. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.cc:31: Java_SimpleCacheActivityStatusNotifier_NewInstance( The threading is dangerously subtle here. I guess this constructor is called on the IO thread but you are calling here the Java side (still on the IO thread) which accesses a UI thread singleton. We can have an offline discussion to address this. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.cc:43: if (callback_runner_ && !notify_callback_.is_null()) { I would add a DCHECK(callback_runner_) in the constructor, make its scoped_refptr non-reassignable (with const) and remove the callback_runner_ check here. This can cause subtle bugs. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.cc:54: } // net Nit: namespace net https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... File net/android/simple_cache_activity_status_notifier.h (right): https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:23: // This is used by the SimpleIndex in net/disk_cache/simple/ to listens to Nit: s/listens/listen https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:40: ActivityStatusChangedCallback; Nit: this needs to be indented by 4 spaces. Alternatively you can break the line above after '<' and also indent by 4 spaces. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:43: base::SingleThreadTaskRunner* callback_runner, Nit: input parameters go first. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:54: private: Nit: this needs to be indented by 1 space (same on line 27). https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:58: Nit: extra blank line. https://codereview.chromium.org/14362009/diff/7005/net/android/simple_cache_a... net/android/simple_cache_activity_status_notifier.h:61: } // namespace net Nit: a blank line should be added below. https://codereview.chromium.org/14362009/diff/7005/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/14362009/diff/7005/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:22: Nit: change not needed. https://codereview.chromium.org/14362009/diff/7005/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:27: const int kWriteToDiskDelay_ms = 20000; Nit: s/_ms/MSecs here and below. https://codereview.chromium.org/14362009/diff/7005/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/14362009/diff/7005/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:180: net::SimpleCacheActivityStatusNotifier activity_status_notifier_; So this is indeed constructed on the IO thread.
https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:26: public SimpleCacheActivityStatusNotifier(int nativePtr) { As we discussed this constructor is called on the IO thread but is accessing some UI thread-only state. You can retrieve the Android UI looper by doing Looper.getMainLooper(). See http://developer.android.com/reference/android/os/Looper.html. Note that ideally all the methods from this Java object should be directly called from the UI thread but as we discussed you can hardly inject the UI message loop into the disk cache (this might need to be confirmed though). https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: nativeNotifyActivityStatusChanged(mNativePtr, state); As we discussed this is called on the UI thread while your native object lives on the IO thread. You need to make sure that |mNativePtr| won't be dereferenced after its deletion. To do so you can add a destroy() java method that will be called by the native side on destruction. This method will do two things: - mNativePtr = 0 - unregister the observer Note that this destroy method will have to be called from the UI thread.
On 2013/04/22 11:59:34, Philippe wrote: > https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... > File > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java > (right): > > https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:26: > public SimpleCacheActivityStatusNotifier(int nativePtr) { > As we discussed this constructor is called on the IO thread but is accessing > some UI thread-only state. > > You can retrieve the Android UI looper by doing Looper.getMainLooper(). See > http://developer.android.com/reference/android/os/Looper.html. > > Note that ideally all the methods from this Java object should be directly > called from the UI thread but as we discussed you can hardly inject the UI > message loop into the disk cache (this might need to be confirmed though). > > https://codereview.chromium.org/14362009/diff/7005/net/android/java/src/org/c... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: > nativeNotifyActivityStatusChanged(mNativePtr, state); > As we discussed this is called on the UI thread while your native object lives > on the IO thread. You need to make sure that |mNativePtr| won't be dereferenced > after its deletion. > > To do so you can add a destroy() java method that will be called by the native > side on destruction. This method will do two things: > - mNativePtr = 0 > - unregister the observer > > Note that this destroy method will have to be called from the UI thread. Also please file a bug tracking the deletion of SimpleCacheActivityStatusNotifier.java and its native counter-part in favor of a native ActivityStatus class living in base/android/.
https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:15: * Used by the SimpleIndex in net/disk_cache/simple/ to listens to changes in On 2013/04/22 11:35:48, Philippe wrote: > Nit: the char limit is 100 in Java. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:18: // extends BroadcastReceiver On 2013/04/22 11:35:48, Philippe wrote: > You can remove this line. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:22: static public SimpleCacheActivityStatusNotifier NewInstance(int nativePtr) { On 2013/04/22 11:35:48, Philippe wrote: > Nit: s/NewInstance/newInstance Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:23: return new SimpleCacheActivityStatusNotifier(nativePtr); On 2013/04/22 11:35:48, Philippe wrote: > Nit: 4 space indent in Java (here and in a few other places). Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:26: public SimpleCacheActivityStatusNotifier(int nativePtr) { On 2013/04/22 11:35:48, Philippe wrote: > You can make this constructor private since this class is supposed to be > instantiated through the static factory method above. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:26: public SimpleCacheActivityStatusNotifier(int nativePtr) { On 2013/04/22 11:59:34, Philippe wrote: > As we discussed this constructor is called on the IO thread but is accessing > some UI thread-only state. > > You can retrieve the Android UI looper by doing Looper.getMainLooper(). See > http://developer.android.com/reference/android/os/Looper.html. > > Note that ideally all the methods from this Java object should be directly > called from the UI thread but as we discussed you can hardly inject the UI > message loop into the disk cache (this might need to be confirmed though). Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:35: state == ActivityStatus.PAUSED || On 2013/04/22 11:35:48, Philippe wrote: > You can remove support for the PAUSED and DESTROYED state. See > http://developer.android.com/training/basics/activity-lifecycle/pausing.html. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: nativeNotifyActivityStatusChanged(mNativePtr, state); On 2013/04/22 11:59:34, Philippe wrote: > As we discussed this is called on the UI thread while your native object lives > on the IO thread. You need to make sure that |mNativePtr| won't be dereferenced > after its deletion. > > To do so you can add a destroy() java method that will be called by the native > side on destruction. This method will do two things: > - mNativePtr = 0 > - unregister the observer > > Note that this destroy method will have to be called from the UI thread. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/java/sr... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:47: private int mNativePtr = 0; On 2013/04/22 11:35:48, Philippe wrote: > Nit: attributes go first usually in Java. You can also make it 'final' if you > are sure that this pointer never needs to be reset (i.e. if the native side > outlives the Java side). Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/simple_... File net/android/simple_cache_activity_status_notifier.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/simple_... net/android/simple_cache_activity_status_notifier.cc:26: // TODO(felipeg): Do we have to call DetachFromVM somewhere ? I don't see On 2013/04/22 11:35:48, Philippe wrote: > No, you can remove this comment. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/simple_... net/android/simple_cache_activity_status_notifier.cc:31: Java_SimpleCacheActivityStatusNotifier_NewInstance( On 2013/04/22 11:35:48, Philippe wrote: > The threading is dangerously subtle here. I guess this constructor is called on > the IO thread but you are calling here the Java side (still on the IO thread) > which accesses a UI thread singleton. We can have an offline discussion to > address this. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/simple_... net/android/simple_cache_activity_status_notifier.cc:43: if (callback_runner_ && !notify_callback_.is_null()) { On 2013/04/22 11:35:48, Philippe wrote: > I would add a DCHECK(callback_runner_) in the constructor, make its > scoped_refptr non-reassignable (with const) and remove the callback_runner_ > check here. This can cause subtle bugs. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/android/simple_... net/android/simple_cache_activity_status_notifier.cc:54: } // net On 2013/04/22 11:35:48, Philippe wrote: > Nit: namespace net Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/disk_cache/simp... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/7005/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:22: On 2013/04/22 11:35:48, Philippe wrote: > Nit: change not needed. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/disk_cache/simp... net/disk_cache/simple/simple_index.cc:27: const int kWriteToDiskDelay_ms = 20000; On 2013/04/22 11:35:48, Philippe wrote: > Nit: s/_ms/MSecs here and below. Done. https://chromiumcodereview.appspot.com/14362009/diff/7005/net/disk_cache/simp... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/14362009/diff/7005/net/disk_cache/simp... net/disk_cache/simple/simple_index.h:180: net::SimpleCacheActivityStatusNotifier activity_status_notifier_; On 2013/04/22 11:35:48, Philippe wrote: > So this is indeed constructed on the IO thread. Done.
LGTM, provided pliard also gives an LGTM. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... File net/android/simple_cache_activity_status_notifier.cc (right): https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:31: SimpleCacheActivityStatusNotifier::~SimpleCacheActivityStatusNotifier() { Why no thread check here? https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... File net/android/simple_cache_activity_status_notifier.h (right): https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.h:58: base::ThreadChecker io_thread_checker_; Can't refer to io_thread in net/. I know we have this vestigially in simple_cache right now, but we have to get rid of it, and better not add more. https://codereview.chromium.org/14362009/diff/24001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/14362009/diff/24001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.h:171: // Timestamp of the last time we wrote the index to disk. Is this comment still accurate? https://codereview.chromium.org/14362009/diff/24001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.h:182: bool app_on_background_; What's the non-android use of this bool? Is it just to avoid #ifdef throughout the simple_index.cc file?
Thanks Felipe! This looks much better now from the threading perspective. A few more nits though. My LG will follow. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: 2013. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:18: * Used by the SimpleIndex in net/disk_cache/simple/ to listens to changes in the android app state Nit: s/listens/listen https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:22: implements ActivityStatus.StateListener { Nit: this should fit on the line above I believe. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:23: // Pointer to native object. Nit: you can remove this comment. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:25: final private Looper mIoLooper; Nit: the access modifier keyword should be first. Applies to line 28 with 'static' as well. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:29: // Run IO Thread Nit: I would say here and below 'Run on the IO thread.'. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: final SimpleCacheActivityStatusNotifier this_ptr = this; I don't think you need this. 'this' should already be final (non-reassignable). https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:40: // Call the singleton's ActivityStatus in the UI thread. Nit: you can remove this comment. https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:66: public void onActivityStateChange(final int state) { Is 'final' needed here? https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:69: final SimpleCacheActivityStatusNotifier this_ptr = this; Nit: 4 space indent (although this line should be removed). https://codereview.chromium.org/14362009/diff/24001/net/android/java/src/org/... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:70: // Post task to IO Thread. Nit: you can remove this comment too. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... File net/android/simple_cache_activity_status_notifier.cc (right): https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:16: Nit: no blank line here. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:22: const ActivityStatusChangedCallback& notify_callback) : Nit: The ':' should be on the line below. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:25: CHECK(env); Nit: DCHECK() should be enough. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:31: SimpleCacheActivityStatusNotifier::~SimpleCacheActivityStatusNotifier() { On 2013/04/23 11:02:04, gavinp wrote: > Why no thread check here? +1 for a thread check here. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.cc:45: if (!notify_callback_.is_null()) Can we DCHECK(!notify_callback_.is_null()) in the constructor and remove this check? Please also make |notify_callback_| const in the header if you do that. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... File net/android/simple_cache_activity_status_notifier.h (right): https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.h:41: ActivityStatusChangedCallback; Nit: this still needs to be indented. https://codereview.chromium.org/14362009/diff/24001/net/android/simple_cache_... net/android/simple_cache_activity_status_notifier.h:61: } // namespace net Nit: blank line still needed below.
https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/04/23 11:34:25, Philippe wrote: > Nit: 2013. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:18: * Used by the SimpleIndex in net/disk_cache/simple/ to listens to changes in the android app state On 2013/04/23 11:34:25, Philippe wrote: > Nit: s/listens/listen Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:22: implements ActivityStatus.StateListener { On 2013/04/23 11:34:25, Philippe wrote: > Nit: this should fit on the line above I believe. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:23: // Pointer to native object. On 2013/04/23 11:34:25, Philippe wrote: > Nit: you can remove this comment. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:25: final private Looper mIoLooper; On 2013/04/23 11:34:25, Philippe wrote: > Nit: the access modifier keyword should be first. Applies to line 28 with > 'static' as well. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:25: final private Looper mIoLooper; On 2013/04/23 11:34:25, Philippe wrote: > Nit: the access modifier keyword should be first. Applies to line 28 with > 'static' as well. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:29: // Run IO Thread On 2013/04/23 11:34:25, Philippe wrote: > Nit: I would say here and below 'Run on the IO thread.'. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: final SimpleCacheActivityStatusNotifier this_ptr = this; On 2013/04/23 11:34:25, Philippe wrote: > I don't think you need this. 'this' should already be final (non-reassignable). Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:40: // Call the singleton's ActivityStatus in the UI thread. On 2013/04/23 11:34:25, Philippe wrote: > Nit: you can remove this comment. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:66: public void onActivityStateChange(final int state) { On 2013/04/23 11:34:25, Philippe wrote: > Is 'final' needed here? yes it is used inside the Runnable bellow https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:69: final SimpleCacheActivityStatusNotifier this_ptr = this; On 2013/04/23 11:34:25, Philippe wrote: > Nit: 4 space indent (although this line should be removed). Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:70: // Post task to IO Thread. On 2013/04/23 11:34:25, Philippe wrote: > Nit: you can remove this comment too. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... File net/android/simple_cache_activity_status_notifier.cc (right): https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.cc:16: On 2013/04/23 11:34:25, Philippe wrote: > Nit: no blank line here. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.cc:22: const ActivityStatusChangedCallback& notify_callback) : On 2013/04/23 11:34:25, Philippe wrote: > Nit: The ':' should be on the line below. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.cc:25: CHECK(env); On 2013/04/23 11:34:25, Philippe wrote: > Nit: DCHECK() should be enough. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.cc:31: SimpleCacheActivityStatusNotifier::~SimpleCacheActivityStatusNotifier() { On 2013/04/23 11:34:25, Philippe wrote: > On 2013/04/23 11:02:04, gavinp wrote: > > Why no thread check here? > > +1 for a thread check here. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.cc:45: if (!notify_callback_.is_null()) On 2013/04/23 11:34:25, Philippe wrote: > Can we DCHECK(!notify_callback_.is_null()) in the constructor and remove this > check? > > Please also make |notify_callback_| const in the header if you do that. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... File net/android/simple_cache_activity_status_notifier.h (right): https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.h:41: ActivityStatusChangedCallback; On 2013/04/23 11:34:25, Philippe wrote: > Nit: this still needs to be indented. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.h:58: base::ThreadChecker io_thread_checker_; On 2013/04/23 11:02:04, gavinp wrote: > Can't refer to io_thread in net/. I know we have this vestigially in > simple_cache right now, but we have to get rid of it, and better not add more. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... net/android/simple_cache_activity_status_notifier.h:61: } // namespace net On 2013/04/23 11:34:25, Philippe wrote: > Nit: blank line still needed below. Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... net/disk_cache/simple/simple_index.h:171: // Timestamp of the last time we wrote the index to disk. On 2013/04/23 11:02:04, gavinp wrote: > Is this comment still accurate? Done. https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... net/disk_cache/simple/simple_index.h:182: bool app_on_background_; On 2013/04/23 11:02:04, gavinp wrote: > What's the non-android use of this bool? Is it just to avoid #ifdef throughout > the simple_index.cc file? yes, it is to avoid unecessary polutting the code with ifdefs
On 2013/04/23 13:10:12, felipeg1 wrote: > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > File > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java > (right): > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:1: > // Copyright (c) 2012 The Chromium Authors. All rights reserved. > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: 2013. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:18: > * Used by the SimpleIndex in net/disk_cache/simple/ to listens to changes in the > android app state > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: s/listens/listen > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:22: > implements ActivityStatus.StateListener { > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: this should fit on the line above I believe. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:23: > // Pointer to native object. > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: you can remove this comment. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:25: > final private Looper mIoLooper; > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: the access modifier keyword should be first. Applies to line 28 with > > 'static' as well. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:25: > final private Looper mIoLooper; > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: the access modifier keyword should be first. Applies to line 28 with > > 'static' as well. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:29: > // Run IO Thread > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: I would say here and below 'Run on the IO thread.'. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:39: > final SimpleCacheActivityStatusNotifier this_ptr = this; > On 2013/04/23 11:34:25, Philippe wrote: > > I don't think you need this. 'this' should already be final > (non-reassignable). > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:40: > // Call the singleton's ActivityStatus in the UI thread. > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: you can remove this comment. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:66: > public void onActivityStateChange(final int state) { > On 2013/04/23 11:34:25, Philippe wrote: > > Is 'final' needed here? > yes > it is used inside the Runnable bellow > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:69: > final SimpleCacheActivityStatusNotifier this_ptr = this; > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: 4 space indent (although this line should be removed). > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/java/s... > net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:70: > // Post task to IO Thread. > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: you can remove this comment too. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > File net/android/simple_cache_activity_status_notifier.cc (right): > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.cc:16: > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: no blank line here. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.cc:22: const > ActivityStatusChangedCallback& notify_callback) : > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: The ':' should be on the line below. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.cc:25: CHECK(env); > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: DCHECK() should be enough. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.cc:31: > SimpleCacheActivityStatusNotifier::~SimpleCacheActivityStatusNotifier() { > On 2013/04/23 11:34:25, Philippe wrote: > > On 2013/04/23 11:02:04, gavinp wrote: > > > Why no thread check here? > > > > +1 for a thread check here. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.cc:45: if > (!notify_callback_.is_null()) > On 2013/04/23 11:34:25, Philippe wrote: > > Can we DCHECK(!notify_callback_.is_null()) in the constructor and remove this > > check? > > > > Please also make |notify_callback_| const in the header if you do that. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > File net/android/simple_cache_activity_status_notifier.h (right): > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.h:41: > ActivityStatusChangedCallback; > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: this still needs to be indented. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.h:58: base::ThreadChecker > io_thread_checker_; > On 2013/04/23 11:02:04, gavinp wrote: > > Can't refer to io_thread in net/. I know we have this vestigially in > > simple_cache right now, but we have to get rid of it, and better not add more. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/android/simple... > net/android/simple_cache_activity_status_notifier.h:61: } // namespace net > On 2013/04/23 11:34:25, Philippe wrote: > > Nit: blank line still needed below. > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... > File net/disk_cache/simple/simple_index.h (right): > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... > net/disk_cache/simple/simple_index.h:171: // Timestamp of the last time we wrote > the index to disk. > On 2013/04/23 11:02:04, gavinp wrote: > > Is this comment still accurate? > > Done. > > https://chromiumcodereview.appspot.com/14362009/diff/24001/net/disk_cache/sim... > net/disk_cache/simple/simple_index.h:182: bool app_on_background_; > On 2013/04/23 11:02:04, gavinp wrote: > > What's the non-android use of this bool? Is it just to avoid #ifdef throughout > > the simple_index.cc file? > > yes, it is to avoid unecessary polutting the code with ifdefs lgtm, thanks Felipe!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/14362009/38002
Message was sent while issue was closed.
Change committed as 195845
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/14362009/diff/38002/net/android/java/s... File net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java (right): https://chromiumcodereview.appspot.com/14362009/diff/38002/net/android/java/s... net/android/java/src/org/chromium/net/SimpleCacheActivityStatusNotifier.java:31: this.mIoLooper = Looper.myLooper(); Since the construction comes from the net thread (typically the Chrome IO thread), there's guaranteed to be no native looper. So this code makes Chrome crash on any activity state change notification on Android. When we make our next upload on this, let's make sure to add an android browser test that receives at least one activity status notification. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
