Added quota client for serviceworker. Enables 'clear past <time> data'.

content/browser/service_worker/service_worker_context_wrapper.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 
 #include <map>
 
+#include "base/barrier_closure.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/fileapi/chrome_blob_storage_context.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_observer.h"
 #include "content/browser/service_worker/service_worker_process_manager.h"
+#include "content/browser/service_worker/service_worker_quota_client.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/quota/quota_manager_proxy.h"
 
 namespace content {
 
 ServiceWorkerContextWrapper::ServiceWorkerContextWrapper(
     BrowserContext* browser_context)
@@ skipping 20 matching lines @@
   scoped_refptr<base::SequencedTaskRunner> cache_task_runner =
       BrowserThread::GetBlockingPool()
           ->GetSequencedTaskRunnerWithShutdownBehavior(
               BrowserThread::GetBlockingPool()->GetSequenceToken(),
               base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   InitInternal(user_data_directory,
                cache_task_runner,
                database_task_runner,
                disk_cache_thread,
                quota_manager_proxy);
+  if (quota_manager_proxy) {

[jsbell, 2014/10/08 19:26:41]

When is this NULL?

[dmurph, 2014/10/11 00:02:25]

Other classes checked if the proxy was null, but it looks like that shouldn't be
possible.

+    quota_manager_proxy->RegisterClient(new ServiceWorkerQuotaClient(this));
+  }
 }
 
 void ServiceWorkerContextWrapper::Shutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   process_manager_->Shutdown();
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&ServiceWorkerContextWrapper::ShutdownOnIO, this));
 }
@@ skipping 41 matching lines @@
       script_url,
       NULL /* provider_host */,
       base::Bind(&FinishRegistrationOnIO, continuation));
 }
 
 static void FinishUnregistrationOnIO(
     const ServiceWorkerContext::ResultCallback& continuation,
     ServiceWorkerStatusCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   BrowserThread::PostTask(
       BrowserThread::UI,

[michaeln, 2014/10/14 00:50:31]

Not related to your cl...

Oh, the wrapper class is coded up such that it assumes the Register and
Unregister methods are invoked on the UI thread. So it's intent on delivering
the completion callbacks on the UI thread too.

That behavior actually conflicts with the doc comments in the public .h file.
Originally reg/unreg where intended to be called on the UI thread, but it we
decided to make the interface callabled from the IO thread instead when it came
time to add the 'GetInfo' methods (except for the Terminate method). Apparently
the pre-existing impl of reg/unreg wasn't modified accordingly. This class is
crufty :(

I don't think there is a caller of the public reg/unreg methods atm.

[dmurph, 2014/10/14 21:04:50]

Should I add a TODO anywhere?

       FROM_HERE,
       base::Bind(continuation, status == SERVICE_WORKER_OK));
 }
 
 void ServiceWorkerContextWrapper::UnregisterServiceWorker(
     const GURL& pattern,
     const ResultCallback& continuation) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
     BrowserThread::PostTask(
         BrowserThread::IO,
@@ skipping 47 matching lines @@
   for (std::map<GURL, ServiceWorkerUsageInfo>::const_iterator it =
            origins.begin();
        it != origins.end();
        ++it) {
     usage_infos.push_back(it->second);
   }
 
   callback.Run(usage_infos);
 }
 
 namespace {

[jsbell, 2014/10/08 19:26:41]

nit: I think we like having blank lines inside at the start/end of namespace {}
blocks. Whatever clang-format says, though.

[dmurph, 2014/10/11 00:02:25]

clang-format didn't change it

-
-void EmptySuccessCallback(bool success) {
+void SuccessCollectorCallback(const base::Closure& done_closure,
+                              bool* overall_success, bool success) {
+  if (!success) {

[michaeln, 2014/10/08 23:09:15]

nit: {}'s not needed

[dmurph, 2014/10/11 00:02:25]

I would like them

+    *overall_success = false;
+  }
+  done_closure.Run();
 }
 
+void SuccessReportingCallback(
+    const bool* success, const ServiceWorkerContext::ResultCallback& callback) {

[jsbell, 2014/10/08 19:26:40]

nit: Per chromium style, put each arg on a separate line unless the whole
declaration fits on one line.

[dmurph, 2014/10/11 00:02:25]

Done.

+  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {

[michaeln, 2014/10/08 23:09:15]

Do you ever expect this to be called on some other thread? It looks like you can
DCHECK that you're on the IO thread. 

[dmurph, 2014/10/11 00:02:25]

Yes, this comes from the database runner, so I need to switch it back onto the
io thread for the callback (or else quotamanager will blow up).

+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&SuccessReportingCallback, success, callback));
+    return;
+  }
+
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  bool result = *success;
+  delete success;

[michaeln, 2014/10/08 23:09:15]

I'm not sure it's safe to assume that the final callback will get called. You
might want to look at wither scope_ptr<bool> and base::Passed or base::Owned to
get the closure to handled deletion of success when the closure is finally
destructed. See comments in callback.h for usage.

[dmurph, 2014/10/11 00:02:25]

Done.

+  callback.Run(result);
+}
+
+void EmptySuccessCallback(bool success) {}

[jsbell, 2014/10/08 19:26:41]

nit: leave this with { } wrapped to minimize diff

[dmurph, 2014/10/11 00:02:25]

Done.

 }  // namespace
 
-void ServiceWorkerContextWrapper::DeleteForOrigin(const GURL& origin_url) {
+void ServiceWorkerContextWrapper::DeleteForOrigin(
+    const GURL& origin_url, const ResultCallback& done) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   context_core_->storage()->GetAllRegistrations(base::Bind(
       &ServiceWorkerContextWrapper::DidGetAllRegistrationsForDeleteForOrigin,
       this,
-      origin_url));
+      origin_url,
+      done));
+}
+
+void ServiceWorkerContextWrapper::DeleteForOrigin(const GURL& origin_url) {
+  DeleteForOrigin(origin_url, base::Bind(&EmptySuccessCallback));
 }
 
 void ServiceWorkerContextWrapper::DidGetAllRegistrationsForDeleteForOrigin(
     const GURL& origin,
+    const ResultCallback& done,
     const std::vector<ServiceWorkerRegistrationInfo>& registrations) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  std::set<GURL> applicable_patterns;

[jsbell, 2014/10/08 19:26:40]

For brevity, how about just 'scopes' ?

(We'll probably rename 'pattern' to 'scope' generally at some point, to match
the spec.)

[dmurph, 2014/10/11 00:02:25]

Done.

   for (std::vector<ServiceWorkerRegistrationInfo>::const_iterator it =
            registrations.begin();
        it != registrations.end();
        ++it) {
     const ServiceWorkerRegistrationInfo& registration_info = *it;
     if (origin == registration_info.pattern.GetOrigin()) {

[jsbell, 2014/10/08 19:26:41]

nit: can drop the braces here

[dmurph, 2014/10/11 00:02:25]

I would rather keep them :)

-      UnregisterServiceWorker(registration_info.pattern,
-                              base::Bind(&EmptySuccessCallback));
+      applicable_patterns.insert(registration_info.pattern);
     }
   }
+  bool *success = new bool(true);

[michaeln, 2014/10/08 23:09:15]

maybe call this overall_success for clarity throughout to differentiate from a
single success?

[dmurph, 2014/10/11 00:02:25]

Done.

+  base::Closure barrier = base::BarrierClosure(

[michaeln, 2014/10/08 23:09:15]

neat, hadn't seen barrier closure before

[dmurph, 2014/10/11 00:02:25]

Acknowledged.

+      applicable_patterns.size(),
+      base::Bind(&SuccessReportingCallback, success, done));

[michaeln, 2014/10/08 23:09:15]

base::Owned(overall_success) here or maybe
base::Passed(&localscopedptr_to_success)?

[dmurph, 2014/10/11 00:02:25]

Done.

+
+  for (const GURL& pattern : applicable_patterns) {
+    UnregisterServiceWorker(
+        pattern, base::Bind(&SuccessCollectorCallback, barrier, success));

[michaeln, 2014/10/08 23:09:15]

base::Unretained(overall_success) here?

[dmurph, 2014/10/11 00:02:25]

Unneeded.  Only applicable to refcounted objects or where the object where the
method is being called on isn't refcounted.

+  }
 }
 
 void ServiceWorkerContextWrapper::AddObserver(
     ServiceWorkerContextObserver* observer) {
   observer_list_->AddObserver(observer);
 }
 
 void ServiceWorkerContextWrapper::RemoveObserver(
     ServiceWorkerContextObserver* observer) {
   observer_list_->RemoveObserver(observer);
 }
 
 void ServiceWorkerContextWrapper::SetBlobParametersForCache(
     net::URLRequestContextGetter* request_context,
     ChromeBlobStorageContext* blob_storage_context) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (context_core_ && request_context && blob_storage_context) {
     context_core_->SetBlobParametersForCache(
         request_context->GetURLRequestContext(),
         blob_storage_context->context()->AsWeakPtr());
   }
 }
 
 void ServiceWorkerContextWrapper::InitInternal(

[michaeln, 2014/10/08 23:28:46]

I think it'd be nicer of the 'client' class where implemented in terms of the
'core' instead of the 'wrapper'? I think of the 'wrapper' as a necessary evil in
order to interop with stuff in chrome the run on the ui thread. Mostly sw core
logic is happily single threaded on the IO thread. The 'client' class fits that
bill as well as the QM proper.

I'd be nice to eliminate the rawptr held by the client to the wrapper. The
'core' supports a weakptr and the client is only used on the io thread. Maybe
the initialization has two parts.
- creation on the ui thread and handed to RegisterClient on the UI thread
- then give it a weakptr to the core in here
You'd have to be careful about handing it off to RegistereClient and then
retaining a ptr of some kind in order to poke at it again in InitInternal, but
RefPtr to the rescue for that.

[dmurph, 2014/10/11 00:02:25]

After offline discussion, we decided that the functionality should move to the
core (unregister multiple workers), but the client should hook up to the wrapper
for easy testability (easy to mock out wrapper).

     const base::FilePath& user_data_directory,
     const scoped_refptr<base::SequencedTaskRunner>& stores_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& database_task_runner,
     const scoped_refptr<base::SingleThreadTaskRunner>& disk_cache_thread,
     storage::QuotaManagerProxy* quota_manager_proxy) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
     BrowserThread::PostTask(
         BrowserThread::IO,
         FROM_HERE,
         base::Bind(&ServiceWorkerContextWrapper::InitInternal,
@@ skipping 25 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (status != SERVICE_WORKER_OK) {
     context_core_.reset();
     return;
   }
   context_core_.reset(new ServiceWorkerContextCore(context_core_.get(), this));
   DVLOG(1) << "Restarted ServiceWorkerContextCore successfully.";
 }
 
 }  // namespace content