Cleanup GpuDataManagerImpl[Private] code.

1. BlockSwiftShader() and SwiftShaderAllowed() don't need to be publicly
   exposed.
2. UnlockedSession is just a copy of AutoUnlock at this point. The added
   functionality for |owner_| being null was removed. Replace usage with
   AutoUnlock.
3. GpuDataManagerPrivateImpl::Create() isn't needed. Delete it and use
   std::make_unique<> instead.
4. GpuDataManagerPrivateImpl initialize variables at definition instead
   of in constructor initializer list.
5. Remove UnlockedSession before doing something that might notify
   GpuDataManagerObservers. Observers are already in a
   ThreadSafeObserverList and are notified by PostTask.
6. GpuDataManagerImplPrivate::ProcessCrashed() doesn't need to run on
   browser UI thread. The observers are already notified by PostTask.
7. ScopedGpuDataManagerImpl[Private] doesn't need to hold a raw pointer.
8. Use base::NoDestructor<> instead of base::Singleton<>.

Bug: none
Change-Id: I8bd0829c9fb3b6c11c935872d81bca0971f31629
Reviewed-on: https://chromium-review.googlesource.com/1100974Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567432}

content/browser/gpu/gpu_data_manager_impl.cc

@@ -16,7 +16,8 @@ GpuDataManager* GpuDataManager::GetInstance() {
 
 // static
 GpuDataManagerImpl* GpuDataManagerImpl::GetInstance() {
-  return base::Singleton<GpuDataManagerImpl>::get();
+  static base::NoDestructor<GpuDataManagerImpl> instance;
+  return instance.get();
 }
 
 void GpuDataManagerImpl::BlacklistWebGLForTesting() {
@@ -79,16 +80,6 @@ void GpuDataManagerImpl::DisableHardwareAcceleration() {
   private_->DisableHardwareAcceleration();
 }
 
-void GpuDataManagerImpl::BlockSwiftShader() {
-  base::AutoLock auto_lock(lock_);
-  private_->BlockSwiftShader();
-}
-
-bool GpuDataManagerImpl::SwiftShaderAllowed() const {
-  base::AutoLock auto_lock(lock_);
-  return private_->SwiftShaderAllowed();
-}
-
 bool GpuDataManagerImpl::HardwareAccelerationEnabled() const {
   base::AutoLock auto_lock(lock_);
   return private_->HardwareAccelerationEnabled();
@@ -228,10 +219,8 @@ void GpuDataManagerImpl::SetApplicationVisible(bool is_visible) {
 }
 
 GpuDataManagerImpl::GpuDataManagerImpl()
-    : private_(GpuDataManagerImplPrivate::Create(this)) {
-}
+    : private_(std::make_unique<GpuDataManagerImplPrivate>(this)) {}
 
-GpuDataManagerImpl::~GpuDataManagerImpl() {
-}
+GpuDataManagerImpl::~GpuDataManagerImpl() = default;
 
 }  // namespace content

content/browser/gpu/gpu_data_manager_impl.h

@@ -14,7 +14,7 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/singleton.h"
+#include "base/no_destructor.h"
 #include "base/process/kill.h"
 #include "base/synchronization/lock.h"
 #include "base/time/time.h"
@@ -159,9 +159,6 @@ class CONTENT_EXPORT GpuDataManagerImpl : public GpuDataManager {
   // on Android and Chrome OS.
   void FallBackToNextGpuMode();
 
-  void BlockSwiftShader();
-  bool SwiftShaderAllowed() const;
-
   // Returns false if the latest GPUInfo gl_renderer is from SwiftShader or
   // Disabled (in the viz case).
   bool IsGpuProcessUsingHardwareGpu() const;
@@ -173,31 +170,7 @@ class CONTENT_EXPORT GpuDataManagerImpl : public GpuDataManager {
  private:
   friend class GpuDataManagerImplPrivate;
   friend class GpuDataManagerImplPrivateTest;
-  friend struct base::DefaultSingletonTraits<GpuDataManagerImpl>;
-
-  // It's similar to AutoUnlock, but we want to make it a no-op
-  // if the owner GpuDataManagerImpl is null.
-  // This should only be used by GpuDataManagerImplPrivate where
-  // callbacks are called, during which re-entering
-  // GpuDataManagerImpl is possible.
-  class UnlockedSession {
-   public:
-    explicit UnlockedSession(GpuDataManagerImpl* owner)
-        : owner_(owner) {
-      DCHECK(owner_);
-      owner_->lock_.AssertAcquired();
-      owner_->lock_.Release();
-    }
-
-    ~UnlockedSession() {
-      DCHECK(owner_);
-      owner_->lock_.Acquire();
-    }
-
-   private:
-    GpuDataManagerImpl* owner_;
-    DISALLOW_COPY_AND_ASSIGN(UnlockedSession);
-  };
+  friend class base::NoDestructor<GpuDataManagerImpl>;
 
   GpuDataManagerImpl();
   ~GpuDataManagerImpl() override;

content/browser/gpu/gpu_data_manager_impl_private.cc

@@ -266,6 +266,35 @@ void UpdateGpuInfoOnIO(const gpu::GPUInfo& gpu_info) {
 
 }  // anonymous namespace
 
+GpuDataManagerImplPrivate::GpuDataManagerImplPrivate(GpuDataManagerImpl* owner)
+    : owner_(owner),
+      observer_list_(base::MakeRefCounted<GpuDataManagerObserverList>()) {
+  DCHECK(owner_);
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kDisableGpu))
+    DisableHardwareAcceleration();
+
+  if (command_line->HasSwitch(switches::kSingleProcess) ||
+      command_line->HasSwitch(switches::kInProcessGPU)) {
+    in_process_gpu_ = true;
+    AppendGpuCommandLine(command_line);
+  }
+
+#if defined(OS_MACOSX)
+  CGDisplayRegisterReconfigurationCallback(DisplayReconfigCallback, owner_);
+#endif  // OS_MACOSX
+
+  // For testing only.
+  if (command_line->HasSwitch(switches::kDisableDomainBlockingFor3DAPIs))
+    domain_blocking_enabled_ = false;
+}
+
+GpuDataManagerImplPrivate::~GpuDataManagerImplPrivate() {
+#if defined(OS_MACOSX)
+  CGDisplayRemoveReconfigurationCallback(DisplayReconfigCallback, owner_);
+#endif
+}
+
 void GpuDataManagerImplPrivate::BlacklistWebGLForTesting() {
   // This function is for testing only, so disable histograms.
   update_histograms_ = false;
@@ -289,8 +318,7 @@ gpu::GPUInfo GpuDataManagerImplPrivate::GetGPUInfoForHardwareGpu() const {
   return gpu_info_for_hardware_gpu_;
 }
 
-bool GpuDataManagerImplPrivate::GpuAccessAllowed(
-    std::string* reason) const {
+bool GpuDataManagerImplPrivate::GpuAccessAllowed(std::string* reason) const {
   bool swiftshader_available = false;
 #if BUILDFLAG(ENABLE_SWIFTSHADER)
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ -398,13 +426,11 @@ void GpuDataManagerImplPrivate::RequestVideoMemoryUsageStatsUpdate(
 }
 
 void GpuDataManagerImplPrivate::AddObserver(GpuDataManagerObserver* observer) {
-  GpuDataManagerImpl::UnlockedSession session(owner_);
   observer_list_->AddObserver(observer);
 }
 
 void GpuDataManagerImplPrivate::RemoveObserver(
     GpuDataManagerObserver* observer) {
-  GpuDataManagerImpl::UnlockedSession session(owner_);
   observer_list_->RemoveObserver(observer);
 }
 
@@ -577,11 +603,6 @@ bool GpuDataManagerImplPrivate::HardwareAccelerationEnabled() const {
   return !card_disabled_;
 }
 
-void GpuDataManagerImplPrivate::BlockSwiftShader() {
-  swiftshader_blocked_ = true;
-  OnGpuBlocked();
-}
-
 bool GpuDataManagerImplPrivate::SwiftShaderAllowed() const {
 #if !BUILDFLAG(ENABLE_SWIFTSHADER)
   return false;
@@ -618,27 +639,15 @@ void GpuDataManagerImplPrivate::AddLogMessage(
 
 void GpuDataManagerImplPrivate::ProcessCrashed(
     base::TerminationStatus exit_code) {
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    // Unretained is ok, because it's posted to UI thread, the thread
-    // where the singleton GpuDataManagerImpl lives until the end.
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&GpuDataManagerImpl::ProcessCrashed,
-                       base::Unretained(owner_), exit_code));
-    return;
-  }
-  {
-    GpuDataManagerImpl::UnlockedSession session(owner_);
-    observer_list_->Notify(
-        FROM_HERE, &GpuDataManagerObserver::OnGpuProcessCrashed, exit_code);
-  }
+  observer_list_->Notify(
+      FROM_HERE, &GpuDataManagerObserver::OnGpuProcessCrashed, exit_code);
 }
 
 std::unique_ptr<base::ListValue> GpuDataManagerImplPrivate::GetLogMessages()
     const {
   auto value = std::make_unique<base::ListValue>();
   for (size_t ii = 0; ii < log_messages_.size(); ++ii) {
-    std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
+    auto dict = std::make_unique<base::DictionaryValue>();
     dict->SetInteger("level", log_messages_[ii].level);
     dict->SetString("header", log_messages_[ii].header);
     dict->SetString("message", log_messages_[ii].message);
@@ -648,7 +657,7 @@ std::unique_ptr<base::ListValue> GpuDataManagerImplPrivate::GetLogMessages()
 }
 
 void GpuDataManagerImplPrivate::HandleGpuSwitch() {
-  GpuDataManagerImpl::UnlockedSession session(owner_);
+  base::AutoUnlock unlock(owner_->lock_);
   // Notify observers in the browser process.
   ui::GpuSwitchingManager::GetInstance()->NotifyGpuSwitched();
   // Pass the notification to the GPU process to notify observers there.
@@ -706,48 +715,6 @@ void GpuDataManagerImplPrivate::DisableDomainBlockingFor3DAPIsForTesting() {
   domain_blocking_enabled_ = false;
 }
 
-// static
-GpuDataManagerImplPrivate* GpuDataManagerImplPrivate::Create(
-    GpuDataManagerImpl* owner) {
-  return new GpuDataManagerImplPrivate(owner);
-}
-
-GpuDataManagerImplPrivate::GpuDataManagerImplPrivate(GpuDataManagerImpl* owner)
-    : complete_gpu_info_already_requested_(false),
-      observer_list_(new GpuDataManagerObserverList),
-      card_disabled_(false),
-      swiftshader_blocked_(false),
-      update_histograms_(true),
-      domain_blocking_enabled_(true),
-      owner_(owner),
-      in_process_gpu_(false) {
-  DCHECK(owner_);
-  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDisableGpu))
-    DisableHardwareAcceleration();
-
-  if (command_line->HasSwitch(switches::kSingleProcess) ||
-      command_line->HasSwitch(switches::kInProcessGPU)) {
-    in_process_gpu_ = true;
-    AppendGpuCommandLine(command_line);
-  }
-
-#if defined(OS_MACOSX)
-  CGDisplayRegisterReconfigurationCallback(DisplayReconfigCallback, owner_);
-#endif  // OS_MACOSX
-
-  // For testing only.
-  if (command_line->HasSwitch(switches::kDisableDomainBlockingFor3DAPIs)) {
-    domain_blocking_enabled_ = false;
-  }
-}
-
-GpuDataManagerImplPrivate::~GpuDataManagerImplPrivate() {
-#if defined(OS_MACOSX)
-  CGDisplayRemoveReconfigurationCallback(DisplayReconfigCallback, owner_);
-#endif
-}
-
 void GpuDataManagerImplPrivate::NotifyGpuInfoUpdate() {
   observer_list_->Notify(FROM_HERE, &GpuDataManagerObserver::OnGpuInfoUpdate);
 }
@@ -765,8 +732,7 @@ void GpuDataManagerImplPrivate::SetApplicationVisible(bool is_visible) {
   application_is_visible_ = is_visible;
 }
 
-std::string GpuDataManagerImplPrivate::GetDomainFromURL(
-    const GURL& url) const {
+std::string GpuDataManagerImplPrivate::GetDomainFromURL(const GURL& url) const {
   // For the moment, we just use the host, or its IP address, as the
   // entry in the set, rather than trying to figure out the top-level
   // domain. This does mean that a.foo.com and b.foo.com will be
@@ -894,7 +860,8 @@ void GpuDataManagerImplPrivate::FallBackToNextGpuMode() {
   if (!card_disabled_) {
     DisableHardwareAcceleration();
   } else if (SwiftShaderAllowed()) {
-    BlockSwiftShader();
+    swiftshader_blocked_ = true;
+    OnGpuBlocked();
   } else if (base::FeatureList::IsEnabled(features::kVizDisplayCompositor)) {
     // The GPU process is frequently crashing with only the display compositor
     // running. This should never happen so something is wrong. Crash the

content/browser/gpu/gpu_data_manager_impl_private.h

@@ -37,7 +37,8 @@ namespace content {
 
 class CONTENT_EXPORT GpuDataManagerImplPrivate {
  public:
-  static GpuDataManagerImplPrivate* Create(GpuDataManagerImpl* owner);
+  explicit GpuDataManagerImplPrivate(GpuDataManagerImpl* owner);
+  virtual ~GpuDataManagerImplPrivate();
 
   void BlacklistWebGLForTesting();
   gpu::GPUInfo GetGPUInfo() const;
@@ -57,7 +58,6 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
   void UnblockDomainFrom3DAPIs(const GURL& url);
   void DisableHardwareAcceleration();
   bool HardwareAccelerationEnabled() const;
-  void BlockSwiftShader();
   bool SwiftShaderAllowed() const;
 
   void UpdateGpuInfo(
@@ -109,8 +109,6 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
 
   void SetApplicationVisible(bool is_visible);
 
-  virtual ~GpuDataManagerImplPrivate();
-
  private:
   friend class GpuDataManagerImplPrivateTest;
 
@@ -131,10 +129,10 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
     GpuDataManagerImpl::DomainGuilt last_guilt;
   };
 
-  typedef std::map<std::string, DomainBlockEntry> DomainBlockMap;
+  using DomainBlockMap = std::map<std::string, DomainBlockEntry>;
 
-  typedef base::ObserverListThreadSafe<GpuDataManagerObserver>
-      GpuDataManagerObserverList;
+  using GpuDataManagerObserverList =
+      base::ObserverListThreadSafe<GpuDataManagerObserver>;
 
   struct LogMessage {
     int level;
@@ -149,8 +147,6 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
           message(_message) { }
   };
 
-  explicit GpuDataManagerImplPrivate(GpuDataManagerImpl* owner);
-
   // Called when GPU access (hardware acceleration and swiftshader) becomes
   // blocked.
   void OnGpuBlocked();
@@ -173,7 +169,9 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
   //   3) all other platforms, this returns false.
   bool NeedsCompleteGpuInfoCollection() const;
 
-  bool complete_gpu_info_already_requested_;
+  GpuDataManagerImpl* const owner_;
+
+  bool complete_gpu_info_already_requested_ = false;
 
   gpu::GpuFeatureInfo gpu_feature_info_;
   gpu::GPUInfo gpu_info_;
@@ -190,25 +188,23 @@ class CONTENT_EXPORT GpuDataManagerImplPrivate {
 
   // Current card force-disabled due to GPU crashes, or disabled through
   // the --disable-gpu commandline switch.
-  bool card_disabled_;
+  bool card_disabled_ = false;
 
   // SwiftShader force-blocked due to GPU crashes using SwiftShader.
-  bool swiftshader_blocked_;
+  bool swiftshader_blocked_ = false;
 
   // We disable histogram stuff in testing, especially in unit tests because
   // they cause random failures.
-  bool update_histograms_;
+  bool update_histograms_ = true;
 
   DomainBlockMap blocked_domains_;
   mutable std::list<base::Time> timestamps_of_gpu_resets_;
-  bool domain_blocking_enabled_;
+  bool domain_blocking_enabled_ = true;
 
   bool application_is_visible_ = true;
 
-  GpuDataManagerImpl* owner_;
-
   // True if --single-process or --in-process-gpu is passed in.
-  bool in_process_gpu_;
+  bool in_process_gpu_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(GpuDataManagerImplPrivate);
 };

content/browser/gpu/gpu_data_manager_impl_private_unittest.cc

@@ -21,12 +21,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
-#if defined(OS_WIN)
-#include "base/win/windows_version.h"
-#endif
-
-#define LONG_STRING_CONST(...) #__VA_ARGS__
-
 namespace content {
 namespace {
 
@@ -72,18 +66,14 @@ class GpuDataManagerImplPrivateTest : public testing::Test {
   // so we can make a little helper class here.
   class ScopedGpuDataManagerImpl {
    public:
-    ScopedGpuDataManagerImpl() : impl_(new GpuDataManagerImpl()) {
-      EXPECT_TRUE(impl_);
-      EXPECT_TRUE(impl_->private_.get());
-    }
-    ~ScopedGpuDataManagerImpl() { delete impl_; }
-
-    GpuDataManagerImpl* get() const { return impl_; }
+    ScopedGpuDataManagerImpl() { EXPECT_TRUE(impl_.private_.get()); }
+    ~ScopedGpuDataManagerImpl() = default;
 
-    GpuDataManagerImpl* operator->() const { return impl_; }
+    GpuDataManagerImpl* get() { return &impl_; }
+    GpuDataManagerImpl* operator->() { return &impl_; }
 
    private:
-    GpuDataManagerImpl* impl_;
+    GpuDataManagerImpl impl_;
     DISALLOW_COPY_AND_ASSIGN(ScopedGpuDataManagerImpl);
   };
 
@@ -91,29 +81,17 @@ class GpuDataManagerImplPrivateTest : public testing::Test {
   // in the GpuDataManagerImpl constructor.
   class ScopedGpuDataManagerImplPrivate {
    public:
-    ScopedGpuDataManagerImplPrivate() : impl_(new GpuDataManagerImpl()) {
-      EXPECT_TRUE(impl_);
-      EXPECT_TRUE(impl_->private_.get());
-    }
-    ~ScopedGpuDataManagerImplPrivate() { delete impl_; }
+    ScopedGpuDataManagerImplPrivate() { EXPECT_TRUE(impl_.private_.get()); }
+    ~ScopedGpuDataManagerImplPrivate() = default;
 
-    GpuDataManagerImplPrivate* get() const {
-      return impl_->private_.get();
-    }
-
-    GpuDataManagerImplPrivate* operator->() const {
-      return impl_->private_.get();
-    }
+    GpuDataManagerImplPrivate* get() { return impl_.private_.get(); }
+    GpuDataManagerImplPrivate* operator->() { return impl_.private_.get(); }
 
    private:
-    GpuDataManagerImpl* impl_;
+    GpuDataManagerImpl impl_;
     DISALLOW_COPY_AND_ASSIGN(ScopedGpuDataManagerImplPrivate);
   };
 
-  void SetUp() override {}
-
-  void TearDown() override {}
-
   base::Time JustBeforeExpiration(const GpuDataManagerImplPrivate* manager);
   base::Time JustAfterExpiration(const GpuDataManagerImplPrivate* manager);
   void TestBlockingDomainFrom3DAPIs(