Omnibox - ClipboardRecentContent - Make Proper Singleton

components/open_from_clipboard/clipboard_recent_content.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 "components/open_from_clipboard/clipboard_recent_content.h"
 
 #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,
     // TODO(mpearson): add support for chrome:// URLs.  Right now the scheme
     // for that lives in content and is accessible via
     // GetEmbedderRepresentationOfAboutScheme() or content::kChromeUIScheme
     // TODO(mpearson): when adding desktop support, add kFileScheme, kFtpScheme,
     // and kGopherScheme.
 };
 
 }  // namespace
 
 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
 ClipboardRecentContent* ClipboardRecentContent::GetInstance() {
   return g_clipboard_recent_content;
 }
 
 // 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;
 }
 
 // static
 bool ClipboardRecentContent::IsAppropriateSuggestion(const GURL& url) {
   // Check to make sure it's a scheme we're willing to suggest.
   for (const auto* authorized_scheme : kAuthorizedSchemes) {
     if (url.SchemeIs(authorized_scheme))
       return true;
   }
 
   // Not a scheme we're allowed to return.
   return false;
 }