Omnibox - ClipboardRecentContent - Make Proper Singleton

components/open_from_clipboard/clipboard_recent_content.cc

Index: components/open_from_clipboard/clipboard_recent_content.cc
diff --git a/components/open_from_clipboard/clipboard_recent_content.cc b/components/open_from_clipboard/clipboard_recent_content.cc
index a31a9d6f76aca9e1c9d9246573a3d54aecedde26..739a8c7ed7e33ab246862032f6bc2dfd03529e32 100644
--- a/components/open_from_clipboard/clipboard_recent_content.cc
+++ b/components/open_from_clipboard/clipboard_recent_content.cc
@@ -7,8 +7,6 @@
 #include "url/url_constants.h"
 
 namespace {
-ClipboardRecentContent* g_clipboard_recent_content = nullptr;
-
 // Schemes appropriate for suggestion by ClipboardRecentContent.
 const char* kAuthorizedSchemes[] = {
     url::kAboutScheme, url::kDataScheme, url::kHttpScheme, url::kHttpsScheme,
@@ -24,17 +22,17 @@ const char* kAuthorizedSchemes[] = {
 ClipboardRecentContent::ClipboardRecentContent() {}
 
 ClipboardRecentContent::~ClipboardRecentContent() {
-  g_clipboard_recent_content = nullptr;
 }
 
 // static
 ClipboardRecentContent* ClipboardRecentContent::GetInstance() {
-  return g_clipboard_recent_content;
+  return UniquePtrToInstance()->get();
 }
 
 // static
-void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) {
-  g_clipboard_recent_content = instance;
+void ClipboardRecentContent::SetInstance(
+    std::unique_ptr<ClipboardRecentContent> new_instance) {
+  (*UniquePtrToInstance()) = std::move(new_instance);
 }
 
 // static
@@ -48,3 +46,10 @@ bool ClipboardRecentContent::IsAppropriateSuggestion(const GURL& url) {
   // Not a scheme we're allowed to return.
   return false;
 }
+
+// static
+std::unique_ptr<ClipboardRecentContent>*
+ClipboardRecentContent::UniquePtrToInstance() {

[Peter Kasting, 2017/04/10 21:59:22]

Why do this as a static member function instead of a file-scoped unique_ptr? 
The latter seems more direct.

[Mark P, 2017/04/10 22:05:22]

jif@ wrote:
"I don't think we are allowed to have global static unique_ptr,"

Do you disagree?

[Peter Kasting, 2017/04/10 22:18:34]

On thinking about it more I think both methods are problematic.  The style guide
basically bans globals/statics that run destructors.  So no matter where we
declare it this is a problem.  We want the object to not be destroyed on exit.

We could do this with CR_DEFINE_STATIC_LOCAL here instead.  But this all feels
like overkill.  Why not leave the interface above as taking a scoped_ptr (to
indicate ownership transfer), but under the hood store this in a raw pointer
like before?

ClipboardRecentContent* g_clipboard_recent_content = nullptr;

void ClipboardRecentContent::SetInstance(
    std::unique_ptr<ClipboardRecentContent> new_instance) {
  delete g_clipboard_recent_content;
  g_clipboard_recent_content = new_instance.release();
}

[Mark P, 2017/04/10 23:10:14]

Okay.

This is also fine with me.  Done.

+  static std::unique_ptr<ClipboardRecentContent> instance;
+  return &instance;
+}