[CacheStorage] Temporarily preserve recently opened caches from CacheStorageDispatcherHost

[CacheStorage] Temporarily preserved recently opened caches from CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients don't
trigger preservation. The CL also lowers the time that the cache is held open to 5
seconds from 30 as that should be more than enough time between cache actions on most
pages.

BUG=623159
Committed: https://crrev.com/848127113da87ca062b139a6ab1de1b16a6df1d4
Cr-Commit-Position: refs/heads/master@{#402798}

[jkarlin, 2016-06-24 20:13:50]

jkarlin@chromium.org changed reviewers:
+ nhiroki@chromium.org

[jkarlin, 2016-06-24 20:13:52]

nhiroki@ PTAL at everything, thank you!

[jkarlin, 2016-06-27 15:13:43]

Description was changed from

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. The CL also lowers the time that the
cache is held open to 5 seconds from 30 as that should be more than enough time
before cache actions on most pages.

BUG=623159
==========

to

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. The CL also lowers the time that the
cache is held open to 5 seconds from 30 as that should be more than enough time
between cache actions on most pages.

BUG=623159
==========

[jkarlin, 2016-06-27 15:14:50]

Description was changed from

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. The CL also lowers the time that the
cache is held open to 5 seconds from 30 as that should be more than enough time
between cache actions on most pages.

BUG=623159
==========

to

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients
don't
trigger preservation. The CL also lowers the time that the cache is held open to
5
seconds from 30 as that should be more than enough time between cache actions on
most
pages.

BUG=623159
==========

[nhiroki, 2016-06-28 06:22:34]

Looks good overall.

https://codereview.chromium.org/2100433003/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_dispatcher_host.h (right):

https://codereview.chromium.org/2100433003/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_dispatcher_host.h:183:
base::WeakPtrFactory<CacheStorageDispatcherHost> weak_ptr_factory_;
Is this usage of WeakPtrFactory safe?

According to this discussion, having WeakPtr to RefCountedThreadSafe object
(CacheStorageDispatcherHost inherits BrowserMessageFilter that is
RefCountedThreadSafe) seems to be quite controversial:
https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=foo...

[jkarlin, 2016-06-28 11:20:14]

Thanks! PTAL.

https://codereview.chromium.org/2100433003/diff/40001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_dispatcher_host.h (right):

https://codereview.chromium.org/2100433003/diff/40001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_dispatcher_host.h:183:
base::WeakPtrFactory<CacheStorageDispatcherHost> weak_ptr_factory_;
On 2016/06/28 06:22:34, nhiroki (slow) wrote:
> Is this usage of WeakPtrFactory safe?
> 
> According to this discussion, having WeakPtr to RefCountedThreadSafe object
> (CacheStorageDispatcherHost inherits BrowserMessageFilter that is
> RefCountedThreadSafe) seems to be quite controversial:
>
https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=foo...

Thanks, and good point. Fixed.

[nhiroki, 2016-06-29 09:15:39]

https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right):

https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_dispatcher_host.cc:368: this,
cache_handle->value()),
This may extend the lifetime of MessageFilter. Although I'm not really sure
whether this extension is harmful, I think it might be better to avoid it.

Instead of passing "this", we could make a cancelable task to stop preserving a
cache and cancel all scheduled tasks when the dispatcher host is to be destroyed
like this:

class Task {
  void Run() {
    dispatcher_host_->StopPreservingCache(cache_);
    dispatcher_host_->RemoveTask(task_id_);
    // This task is destroyed at this point.
  }

  CacheStorageDispatcherHost* dispatcher_host_;
  CacheStorageCache* cache_;
  int task_id_;
}

class CacheStorageDispatcherHost {
  ~CacheStorageDispatcherHost() { tasks_.Clear(); }

  Task* CreateTask(cache_handle) {
    Task* task = new Task(this, cache_handle->value());
    int task_id = tasks_.Add(task);
    task->set_task_id(task_id);
    return task;
  }

  void RemoveTask(task_id) {
    tasks_->Remove(task_id);
  }

  IDMap<Task, IDMapOwnPointer> tasks_;
}

// In CacheStorageDispatcherHost::OnCacheStorageOpenCallback
PostDelayedTask(
    FROM_HERE,
    base::Bind(&Task::Run, CreateTask(cache_handle)->GetWeakPtr()),
    delay);

[jkarlin, 2016-06-29 12:50:37]

Patchset #5 (id:80001) has been deleted

[jkarlin, 2016-06-29 12:52:53]

Thanks, PTAL.

https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right):

https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_dispatcher_host.cc:368: this,
cache_handle->value()),
On 2016/06/29 09:15:39, nhiroki (slow) wrote:
> This may extend the lifetime of MessageFilter. Although I'm not really sure
> whether this extension is harmful, I think it might be better to avoid it.
> 
> Instead of passing "this", we could make a cancelable task to stop preserving
a
> cache and cancel all scheduled tasks when the dispatcher host is to be
destroyed
> like this:
> 
> class Task {
>   void Run() {
>     dispatcher_host_->StopPreservingCache(cache_);
>     dispatcher_host_->RemoveTask(task_id_);
>     // This task is destroyed at this point.
>   }
> 
>   CacheStorageDispatcherHost* dispatcher_host_;
>   CacheStorageCache* cache_;
>   int task_id_;
> }
> 
> class CacheStorageDispatcherHost {
>   ~CacheStorageDispatcherHost() { tasks_.Clear(); }
> 
>   Task* CreateTask(cache_handle) {
>     Task* task = new Task(this, cache_handle->value());
>     int task_id = tasks_.Add(task);
>     task->set_task_id(task_id);
>     return task;
>   }
> 
>   void RemoveTask(task_id) {
>     tasks_->Remove(task_id);
>   }
> 
>   IDMap<Task, IDMapOwnPointer> tasks_;
> }
> 
> // In CacheStorageDispatcherHost::OnCacheStorageOpenCallback
> PostDelayedTask(
>     FROM_HERE,
>     base::Bind(&Task::Run, CreateTask(cache_handle)->GetWeakPtr()),
>     delay);

Actually, there isn't even a need to run the callback on the
CacheStorageDispatcherHost. There is no need for it to hang onto the cache
handle. Since the cache handle is weak (it dies when the CacheStorage that owns
it dies), let's just keep the cache handle in the callback itself. Fixed.

[nhiroki, 2016-06-29 14:05:12]

On 2016/06/29 12:52:53, jkarlin wrote:
> Thanks, PTAL.
> 
>
https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
> File content/browser/cache_storage/cache_storage_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/2100433003/diff/60001/content/browser/cache_s...
> content/browser/cache_storage/cache_storage_dispatcher_host.cc:368: this,
> cache_handle->value()),
> On 2016/06/29 09:15:39, nhiroki (slow) wrote:
> > This may extend the lifetime of MessageFilter. Although I'm not really sure
> > whether this extension is harmful, I think it might be better to avoid it.
> > 
> > Instead of passing "this", we could make a cancelable task to stop
preserving
> a
> > cache and cancel all scheduled tasks when the dispatcher host is to be
> destroyed
> > like this:
> > 
> > class Task {
> >   void Run() {
> >     dispatcher_host_->StopPreservingCache(cache_);
> >     dispatcher_host_->RemoveTask(task_id_);
> >     // This task is destroyed at this point.
> >   }
> > 
> >   CacheStorageDispatcherHost* dispatcher_host_;
> >   CacheStorageCache* cache_;
> >   int task_id_;
> > }
> > 
> > class CacheStorageDispatcherHost {
> >   ~CacheStorageDispatcherHost() { tasks_.Clear(); }
> > 
> >   Task* CreateTask(cache_handle) {
> >     Task* task = new Task(this, cache_handle->value());
> >     int task_id = tasks_.Add(task);
> >     task->set_task_id(task_id);
> >     return task;
> >   }
> > 
> >   void RemoveTask(task_id) {
> >     tasks_->Remove(task_id);
> >   }
> > 
> >   IDMap<Task, IDMapOwnPointer> tasks_;
> > }
> > 
> > // In CacheStorageDispatcherHost::OnCacheStorageOpenCallback
> > PostDelayedTask(
> >     FROM_HERE,
> >     base::Bind(&Task::Run, CreateTask(cache_handle)->GetWeakPtr()),
> >     delay);
> 
> Actually, there isn't even a need to run the callback on the
> CacheStorageDispatcherHost. There is no need for it to hang onto the cache
> handle. Since the cache handle is weak (it dies when the CacheStorage that
owns
> it dies), let's just keep the cache handle in the callback itself. Fixed.

Neat! LGTM :)

[jkarlin, 2016-06-29 14:13:43]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2016-06-29 14:14:00]

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, 2016-06-29 14:17:38]

Description was changed from

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients
don't
trigger preservation. The CL also lowers the time that the cache is held open to
5
seconds from 30 as that should be more than enough time between cache actions on
most
pages.

BUG=623159
==========

to

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients
don't
trigger preservation. The CL also lowers the time that the cache is held open to
5
seconds from 30 as that should be more than enough time between cache actions on
most
pages.

BUG=623159
==========

[commit-bot: I haz the power, 2016-06-29 14:17:39]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-29 14:17:48]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 14:19:13]

Description was changed from

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients
don't
trigger preservation. The CL also lowers the time that the cache is held open to
5
seconds from 30 as that should be more than enough time between cache actions on
most
pages.

BUG=623159
==========

to

==========
[CacheStorage] Temporarily preserved recently opened caches from
CacheStorageDispatcherHost

Currently when a cache is opened it is preserved for 30 seconds by
content::CacheStorage so that, in case the cache is accessed again soon it
doesn't have to be reopened. This happened in
https://codereview.chromium.org/1470073003/.

We don't want to preserve every opened cache. Particularly, we don't want to
preserve caches opened by the QuotaManager when it's determining the size of
all of the caches. We only want to preserve caches opened on behalf of
documents.

This CL moves the preservation logic to the CacheStorageDispatcherHost, where
open calls from the document are made. That way open calls from other clients
don't
trigger preservation. The CL also lowers the time that the cache is held open to
5
seconds from 30 as that should be more than enough time between cache actions on
most
pages.

BUG=623159

Committed: https://crrev.com/848127113da87ca062b139a6ab1de1b16a6df1d4
Cr-Commit-Position: refs/heads/master@{#402798}
==========

[commit-bot: I haz the power, 2016-06-29 14:19:14]

Patchset 5 (id:??) landed as
https://crrev.com/848127113da87ca062b139a6ab1de1b16a6df1d4
Cr-Commit-Position: refs/heads/master@{#402798}