Clean up threading model of external extension providers

chrome/browser/extensions/external_extension_loader.h

+// Copyright (c) 2011 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_LOADER_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_LOADER_H_
+#pragma once
+
+#include "base/lock.h"
+#include "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+
+class DictionaryValue;
+class ExternalExtensionProviderImpl;
+
+// Base class for gathering a list of external extensions. Subclasses
+// implement loading from registry, JSON file, policy.
+// Instances are owned by ExternalExtensionProviderImpl objects.
+// Instances are created on the UI thread and expect public method calls from
+// the UI thread. Some subclasses introduce new methods that are executed on the
+// FILE thread.
+// The sequence of loading the extension list:
+// 1.) StartLoading() - checks if a loading task is already running
+// 2.) Load() - implemented in subclasses
+// 3.) FinishLoading()
+// 4.) owner_->SetPrefs()
+class ExternalExtensionLoader
+    : public base::RefCountedThreadSafe<ExternalExtensionLoader> {
+ public:
+  explicit ExternalExtensionLoader() : running_(false) {}
+
+  // Specifies the provider that owns this object.
+  void Init(ExternalExtensionProviderImpl* owner);
+
+  // Called by the owner before it gets deleted.
+  void OwnerShutdown();
+
+  // Initiates the possibly asynchronous loading of extension list.
+  void StartLoading();
+
+ protected:
+  virtual ~ExternalExtensionLoader() {}
+
+  // Notifies the provider that the list of extensions has been loaded.
+  void LoadFinished();
+
+  // The external-source specific logic that loads the list of extensions.
+  virtual void Load() = 0;
+
+  // Protects |prefs_|.
+  Lock lock_;

[Aaron Boodman, 2011/01/06 18:54:40]

It is rare to use Locks directly in Chromium code and usually indicates
something is wrong with the design.

Do you actually access this concurrently from multiple threads? It looks like
maybe this member is only used as a way to pass something from the file thread
to the ui thread. If so, I don't think the lock is necessary.

[gfeher, 2011/01/07 00:10:01]

Done.

+
+  // Used for passing the list of extensions from the method that loads them
+  // to |LoadFinished|. Depending on the implementation of |Load|, this
+  // may be accessed from multiple threads.
+  scoped_ptr<DictionaryValue> prefs_;
+
+ private:
+  friend class base::RefCountedThreadSafe<ExternalExtensionLoader>;
+
+  ExternalExtensionProviderImpl* owner_;  // weak
+
+  // Set to true if loading the extensions is already running. New requests
+  // are ignored while this is set true.
+  bool running_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExternalExtensionLoader);
+};
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTERNAL_EXTENSION_LOADER_H_