Make //content responsible for generating notification Ids

content/browser/notifications/notification_id_generator.cc

 // Copyright 2015 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/notifications/notification_id_generator.h"
 
 #include <sstream>
 
 #include "base/base64.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/sha1.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_context.h"
 #include "url/gurl.h"
 
 namespace content {
 namespace {
 
 const char kPersistentPrefix[] = "p:";

[Miguel Garcia, 2016/09/02 14:33:16]

perhaps document these constants?

[Peter Beverloo, 2016/09/05 15:11:01]

With what? I don't think "Prefix applied to notification ids for persistent
notifications" adds a tremendous amount of value. I've renamed them to
kPersistent*Notification*Prefix instead, does that help?

[Miguel Garcia, 2016/09/07 17:32:33]

Acknowledged.

 const char kNonPersistentPrefix[] = "n:";
 
 const char kSeparator = '#';
 
 // Computes a hash based on the path in which the |browser_context| is stored.
 // Since we only store the hash, SHA-1 is used to make the probability of
 // collisions negligible.
 std::string ComputeBrowserContextHash(BrowserContext* browser_context) {
   const base::FilePath path = browser_context->GetPath();
 
 #if defined(OS_WIN)
   std::string path_hash = base::SHA1HashString(base::WideToUTF8(path.value()));
 #else
   std::string path_hash = base::SHA1HashString(path.value());
 #endif
 
   return base::HexEncode(path_hash.c_str(), path_hash.length());
 }
 
 }  // namespace
 
 NotificationIdGenerator::NotificationIdGenerator(
-    BrowserContext* browser_context,
-    int render_process_id)
-    : browser_context_(browser_context),
-      render_process_id_(render_process_id) {}
+    BrowserContext* browser_context)
+    : browser_context_(browser_context) {}
 
 NotificationIdGenerator::~NotificationIdGenerator() {}
 
 // static
 bool NotificationIdGenerator::IsPersistentNotification(
     const base::StringPiece& notification_id) {
   return notification_id.starts_with(kPersistentPrefix);
 }
 
 // static
 bool NotificationIdGenerator::IsNonPersistentNotification(
     const base::StringPiece& notification_id) {
   return notification_id.starts_with(kNonPersistentPrefix);
 }
 
 std::string NotificationIdGenerator::GenerateForPersistentNotification(
     const GURL& origin,
     const std::string& tag,
     int64_t persistent_notification_id) const {
   DCHECK(origin.is_valid());
   DCHECK_EQ(origin, origin.GetOrigin());
 
   std::stringstream stream;
 
   stream << kPersistentPrefix;
   stream << ComputeBrowserContextHash(browser_context_);
   stream << base::IntToString(browser_context_->IsOffTheRecord());

[Miguel Garcia, 2016/09/02 14:33:16]

why do you need to pass this to generate the notification id?

[Peter Beverloo, 2016/09/05 15:11:01]

The ComputeBrowserContextHash() will yield the same value for normal and
incognito profiles I believe. OffTheRecordProfile::GetPath() returns the path of
its parent profile.

It's sad that Web Notifications don't work in Incognito mode. We shouldn't
continue to add barriers that work against making that happen.

   stream << origin;
 
   // Persistent notification ids are unique for the lifetime of the notification

[Miguel Garcia, 2016/09/02 14:33:16]

why does this comment belong here? Seems an odd place to document it.

[Peter Beverloo, 2016/09/05 15:11:01]

It's redundant with the class-level command. I've removed it.

   // database, orthogonal to the renderer that created the notification.
 
   stream << base::IntToString(!tag.empty());
   if (tag.size())
     stream << tag;
   else
     stream << base::Int64ToString(persistent_notification_id);
 
   return stream.str();
 }
 
 std::string NotificationIdGenerator::GenerateForNonPersistentNotification(
     const GURL& origin,
     const std::string& tag,
-    int non_persistent_notification_id) const {
+    int non_persistent_notification_id,
+    int render_process_id) const {
   DCHECK(origin.is_valid());
   DCHECK_EQ(origin, origin.GetOrigin());
 
   std::stringstream stream;
 
   stream << kNonPersistentPrefix;
   stream << ComputeBrowserContextHash(browser_context_);
   stream << base::IntToString(browser_context_->IsOffTheRecord());
   stream << origin;
 
   // Non-persistent notification ids are unique per renderer process when no

[Miguel Garcia, 2016/09/02 14:33:16]

Can you explain why you need the different rules for uniqueness? why not just
make all the ids unique forever regardless of the type?

[Peter Beverloo, 2016/09/05 15:11:01]

We currently don't store any state for non-persistent notifications, it's all
ephemeral. Solving this would mean we either (a) stick non-persistent
notifications in the Notification database, which serves no real purpose, or (b)
invent another mechanism for continuous Id counting.

The current mechanism works fine, I don't think we should add a bunch of
complexity to change it.

[Miguel Garcia, 2016/09/07 17:32:33]

Works for me

On 2016/09/05 15:11:01, Peter Beverloo wrote:

   // tag is being used. Tags still identify uniqueness for the given origin.
 
   stream << base::IntToString(!tag.empty());
   if (tag.empty()) {
-    stream << base::IntToString(render_process_id_);
+    stream << base::IntToString(render_process_id);
     stream << kSeparator;
 
     stream << base::IntToString(non_persistent_notification_id);
   } else {
     stream << tag;
   }
 
   return stream.str();
 }
 
 }  // namespace content