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..ca18421b44fb8d7c4f03fc9554d3bacfab8e19fc 100644
--- a/components/open_from_clipboard/clipboard_recent_content.cc
+++ b/components/open_from_clipboard/clipboard_recent_content.cc
@@ -24,7 +24,6 @@ const char* kAuthorizedSchemes[] = {
 ClipboardRecentContent::ClipboardRecentContent() {}
 
 ClipboardRecentContent::~ClipboardRecentContent() {
-  g_clipboard_recent_content = nullptr;

[Peter Kasting, 2017/04/05 23:23:51]

Why are you removing this line?  The only effect of doing so is that
GetInstance() will return pointer-to-garbage in cases where it was returning
pointer-to-null.

[Mark P, 2017/04/06 17:58:00]

No, that's not the only effect.

If someone creates a ClipboardRecentContent object locally, say, on a stack in
tests, then when the object gets deleted, we clear the global singleton.  That's
not right.  The global singleton should never be cleared or deleted, just
auto-cleaned when the OS shuts down the process.

[Peter Kasting, 2017/04/06 19:35:41]

OK.  I think my assumption mentally is that creating one of these locally is
wrong.  It only makes sense to access them through the static getter.  See other
comment's discussion thread.

[Mark P, 2017/04/06 23:25:27]

Acknowledged.

 }
 
 // static
@@ -34,6 +33,7 @@ ClipboardRecentContent* ClipboardRecentContent::GetInstance() {
 
 // static
 void ClipboardRecentContent::SetInstance(ClipboardRecentContent* instance) {

[Peter Kasting, 2017/04/05 23:23:51]

Is there an advantage of having this setter at all over just having the
constructor do this stuff?  This seems like it would be simpler (for callers)
and safer as well.

[Mark P, 2017/04/06 17:58:00]

Oh, do you mean have the constructor set the global singleton pointer to point
to the new object?  That seems like it would work.  But there are two reasons I
don't want to do:
1. That's a weird and unexpected side effect.  Constructing an object causes a
modification to global state.
2. Various tests will have to revised because then they wouldn't be able to
create their own recent content object.  They'd all have to use the global.  For
example, this means they cannot run simultaneously.  We'd also have to be more
careful to make them hermetic.  (I think I'm using that term right.)

[Peter Kasting, 2017/04/06 19:35:41]

True, but sort of makes sense if no one is ever expected to construct one of
these.  We use this technique with various other globals in views/chrome that
tests occasionally need to override.
Correct.
I don't think our test harness actually runs simultaneous tests within the same
process, does it?  I thought it sharded using multiple processes?

There are lots of tests that muck with global state while they're running, so I
can't imagine how things work today if we just use multiple threads.
I don't know, I'm not sure what you're saying :)

[Mark P, 2017/04/06 23:25:27]

Left as-is because the side effect of what you propose unnerves me.  Let's see
what the OWNER of this directory says.

On 2017/04/06 19:35:41, Peter Kasting wrote:

+  DCHECK(g_clipboard_recent_content == nullptr);
   g_clipboard_recent_content = instance;
 }