Background service worker push message processing

chrome/browser/push_messaging/background_budget_service.cc

+// Copyright 2016 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 "chrome/browser/push_messaging/background_budget_service.h"
+
+#include "base/callback.h"
+#include "base/strings/string_number_conversions.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
+
+namespace chrome {

[Peter Beverloo, 2016/04/08 10:41:10]

Please don't put things in the chrome namespace.

[harkness, 2016/04/08 14:11:43]

Done.

+
+namespace {
+
+std::string GetBudgetStringForServiceWorker(Profile* profile,

[Peter Beverloo, 2016/04/08 10:41:10]

nit (1): s/ForServiceWorker/ForOrigin/
nit (2): All callers store this in |*_value|, method says |*_string|.
Consistency++? :)

[harkness, 2016/04/08 14:11:43]

Done.

+                                            const GURL& origin) {
+  const base::DictionaryValue* map =
+      profile->GetPrefs()->GetDictionary(prefs::kBackgroundBudgetMap);
+
+  std::string map_value;
+  map->GetStringWithoutPathExpansion(origin.spec(), &map_value);

[Peter Beverloo, 2016/04/08 10:41:10]

What happens when |origin| is not an origin, but a full URL? :-)

I would suggest adding the following DCHECK:

  DCHECK_EQ(origin.GetOrigin(), origin);

[harkness, 2016/04/08 14:11:43]

Done.

+  return map_value;
+}
+}  // namespace

[Peter Beverloo, 2016/04/08 10:41:10]

micro nit: blank line above this one

[harkness, 2016/04/08 14:11:43]

Done.

+
+// static
+void BackgroundBudgetService::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kBackgroundBudgetMap);
+}
+
+// static
+void BackgroundBudgetService::GetBudget(Profile* profile,
+                                        const GURL& origin,
+                                        const StringCallback& callback) {
+  // Temporarily, this is just moving the old grace period code into the new
+  // service.

[Peter Beverloo, 2016/04/08 10:41:11]

micro nit: This comment doesn't add much value. Would be better when phrased as
a TODO.

[harkness, 2016/04/08 14:11:43]

I just removed it.

+  const std::string map_value =
+      GetBudgetStringForServiceWorker(profile, origin);
+
+  callback.Run(map_value);

[Peter Beverloo, 2016/04/08 10:41:10]

Do you expect this to be asynchronous in the future?

If it will be, I would recommend posting a task rather than invoking
immediately. That avoids writing test code that inadvertently relies on
synchronous behaviour.

  base::ThreadTaskRunnerHandle::Get()->PostTask(
      FROM_HERE, base::Bind(callback, map_value));

[harkness, 2016/04/08 14:11:43]

I did a bit of investigation into the prefs storage, and it looks like as long
as we're not storing a significant amount of information there, it's a good long
term place for our storage. Because that's in-memory, there doesn't seem to be a
need for the lookup to be on a separate thread. 

[Peter Beverloo, 2016/04/08 14:29:38]

It's not about whether you're on the same thread or not, it's about whether
|callback| will be called synchronously or asynchronously :-). Right now your
tests would pass without pumping the message loops as well— that's fine, but it
will allow subtle mistakes where code assumes it's not necessary to pump the
loop.

That's why I suggested the PostTask call: it posts to the *same thread*, which
has the effect of making the response asynchronous.

If the code will by synchronous, let's not bother with callbacks at all?

+}
+
+// static
+void BackgroundBudgetService::StoreBudget(
+    Profile* profile,
+    const GURL& origin,
+    const std::string& notifications_shown,
+    const BudgetCallback& callback) {
+  DictionaryPrefUpdate update(profile->GetPrefs(), prefs::kBackgroundBudgetMap);
+  base::DictionaryValue* map = update.Get();
+  map->SetStringWithoutPathExpansion(origin.spec(), notifications_shown);
+}
+
+}  // namespace chrome