From 04fba9a91d7f46a517f67e1f3f10625ba589b8cc Mon Sep 17 00:00:00 2001 From: "evanm@google.com" Date: Tue, 28 Oct 2008 17:25:25 +0000 Subject: - Make user script loading asynchronous on the file thread. - Automatically reload scripts when the directory changes. - Add a unit test for the GreasemonkeyMaster. Review URL: http://codereview.chromium.org/7472 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@4069 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/greasemonkey_master.cc | 189 ++++++++++++++++++++++--- chrome/browser/greasemonkey_master.h | 59 ++++++-- chrome/browser/greasemonkey_master_unittest.cc | 118 +++++++++++++++ chrome/browser/profile.cc | 19 +++ chrome/browser/profile.h | 8 ++ chrome/browser/render_process_host.cc | 9 +- chrome/common/notification_types.h | 6 + chrome/test/testing_profile.h | 3 + chrome/test/unit/unittests.vcproj | 18 ++- 9 files changed, 385 insertions(+), 44 deletions(-) create mode 100644 chrome/browser/greasemonkey_master_unittest.cc (limited to 'chrome') diff --git a/chrome/browser/greasemonkey_master.cc b/chrome/browser/greasemonkey_master.cc index a8e40c9..2648fa0 100644 --- a/chrome/browser/greasemonkey_master.cc +++ b/chrome/browser/greasemonkey_master.cc @@ -6,21 +6,113 @@ #include +#include "base/file_path.h" #include "base/file_util.h" #include "base/logging.h" +#include "base/message_loop.h" #include "base/path_service.h" #include "base/pickle.h" #include "base/string_util.h" -#include "chrome/common/chrome_paths.h" +#include "chrome/common/notification_service.h" #include "googleurl/src/gurl.h" #include "net/base/net_util.h" -bool GreasemonkeyMaster::UpdateScripts() { +// We reload user scripts on the file thread to prevent blocking the UI. +// ScriptReloader lives on the file thread and does the reload +// work, and then sends a message back to its master with a new SharedMemory*. + +// ScriptReloader is the worker that manages running the script scan +// on the file thread. +// It must be created on, and its public API must only be called from, +// the master's thread. +class GreasemonkeyMaster::ScriptReloader + : public base::RefCounted { + public: + ScriptReloader(GreasemonkeyMaster* master) + : master_(master), master_message_loop_(MessageLoop::current()) {} + + // Start a scan for scripts. + // Will always send a message to the master upon completion. + void StartScan(MessageLoop* work_loop, const FilePath& script_dir); + + // The master is going away; don't call it back. + void DisownMaster() { + master_ = NULL; + } + + private: + // Where functions are run: + // master file + // StartScan -> RunScan + // GetNewScripts() + // NotifyMaster <- RunScan + + // Runs on the master thread. + // Notify the master that new scripts are available. + void NotifyMaster(SharedMemory* memory); + + // Runs on the File thread. + // Scan the script directory for scripts, calling NotifyMaster when done. + // The path is intentionally passed by value so its lifetime isn't tied + // to the caller. + void RunScan(const FilePath script_dir); + + // Runs on the File thread. + // Scan the script directory for scripts, returning either a new SharedMemory + // or NULL on error. + SharedMemory* GetNewScripts(const FilePath& script_dir); + + // A pointer back to our master. + // May be NULL if DisownMaster() is called. + GreasemonkeyMaster* master_; + + // The message loop to call our master back on. + // Expected to always outlive us. + MessageLoop* master_message_loop_; + + DISALLOW_COPY_AND_ASSIGN(ScriptReloader); +}; + +void GreasemonkeyMaster::ScriptReloader::StartScan( + MessageLoop* work_loop, + const FilePath& script_dir) { + // Add a reference to ourselves to keep ourselves alive while we're running. + // Balanced by NotifyMaster(). + AddRef(); + work_loop->PostTask(FROM_HERE, + NewRunnableMethod(this, + &GreasemonkeyMaster::ScriptReloader::RunScan, + script_dir)); +} + +void GreasemonkeyMaster::ScriptReloader::NotifyMaster(SharedMemory* memory) { + if (!master_) { + // The master went away, so these new scripts aren't useful anymore. + delete memory; + } else { + master_->NewScriptsAvailable(memory); + } + + // Drop our self-reference. + // Balances StartScan(). + Release(); +} + +void GreasemonkeyMaster::ScriptReloader::RunScan(const FilePath script_dir) { + SharedMemory* shared_memory = GetNewScripts(script_dir); + + // Post the new scripts back to the master's message loop. + master_message_loop_->PostTask(FROM_HERE, + NewRunnableMethod(this, + &GreasemonkeyMaster::ScriptReloader::NotifyMaster, + shared_memory)); +} + +SharedMemory* GreasemonkeyMaster::ScriptReloader::GetNewScripts( + const FilePath& script_dir) { std::vector scripts; - std::wstring path; - PathService::Get(chrome::DIR_USER_SCRIPTS, &path); - file_util::FileEnumerator enumerator(path, false, + file_util::FileEnumerator enumerator(script_dir.value(), false, file_util::FileEnumerator::FILES, L"*.user.js"); for (std::wstring file = enumerator.Next(); !file.empty(); @@ -28,6 +120,9 @@ bool GreasemonkeyMaster::UpdateScripts() { scripts.push_back(file); } + if (scripts.empty()) + return NULL; + // Pickle scripts data. Pickle pickle; pickle.WriteSize(scripts.size()); @@ -45,29 +140,42 @@ bool GreasemonkeyMaster::UpdateScripts() { } // Create the shared memory object. - scoped_ptr temp_shared_memory(new SharedMemory()); - if (!temp_shared_memory.get()) { - return false; - } + scoped_ptr shared_memory(new SharedMemory()); - shared_memory_serial_++; - if (!temp_shared_memory->Create(std::wstring(), // anonymous - false, // read-only - false, // open existing - pickle.size())) { - return false; + if (!shared_memory->Create(std::wstring(), // anonymous + false, // read-only + false, // open existing + pickle.size())) { + return NULL; } // Map into our process. - if (!temp_shared_memory->Map(pickle.size())) { - return false; - } + if (!shared_memory->Map(pickle.size())) + return NULL; // Copy the pickle to shared memory. - memcpy(temp_shared_memory->memory(), pickle.data(), pickle.size()); + memcpy(shared_memory->memory(), pickle.data(), pickle.size()); - shared_memory_.reset(temp_shared_memory.release()); - return true; + return shared_memory.release(); +} + + +GreasemonkeyMaster::GreasemonkeyMaster(MessageLoop* worker_loop, + const FilePath& script_dir) + : user_script_dir_(new FilePath(script_dir)), + dir_watcher_(new DirectoryWatcher), + worker_loop_(worker_loop) { + // Watch our scripts directory for modifications. + bool ok = dir_watcher_->Watch(script_dir, this); + DCHECK(ok); + + // (Asynchronously) scan for our initial set of scripts. + StartScan(); +} + +GreasemonkeyMaster::~GreasemonkeyMaster() { + if (script_reloader_) + script_reloader_->DisownMaster(); } bool GreasemonkeyMaster::ShareToProcess(ProcessHandle process, @@ -78,3 +186,42 @@ bool GreasemonkeyMaster::ShareToProcess(ProcessHandle process, NOTREACHED(); return false; } + +void GreasemonkeyMaster::NewScriptsAvailable(SharedMemory* handle) { + // Ensure handle is deleted or released. + scoped_ptr handle_deleter(handle); + + if (pending_scan_) { + // While we were scanning, there were further changes. Don't bother + // notifying about these scripts and instead just immediately rescan. + pending_scan_ = false; + StartScan(); + } else { + // We're no longer scanning. + script_reloader_ = NULL; + // We've got scripts ready to go. + shared_memory_.swap(handle_deleter); + + NotificationService::current()->Notify(NOTIFY_NEW_USER_SCRIPTS, + NotificationService::AllSources(), + Details(handle)); + } +} + +void GreasemonkeyMaster::OnDirectoryChanged(const FilePath& path) { + if (script_reloader_.get()) { + // We're already scanning for scripts. We note that we should rescan when + // we get the chance. + pending_scan_ = true; + return; + } + + StartScan(); +} + +void GreasemonkeyMaster::StartScan() { + if (!script_reloader_) + script_reloader_ = new ScriptReloader(this); + + script_reloader_->StartScan(worker_loop_, *user_script_dir_); +} diff --git a/chrome/browser/greasemonkey_master.h b/chrome/browser/greasemonkey_master.h index 9e3a789..51d7139 100644 --- a/chrome/browser/greasemonkey_master.h +++ b/chrome/browser/greasemonkey_master.h @@ -5,20 +5,23 @@ #ifndef CHROME_BROWSER_GREASEMONKEY_MASTER_H__ #define CHROME_BROWSER_GREASEMONKEY_MASTER_H__ +#include "base/directory_watcher.h" #include "base/process.h" #include "base/scoped_ptr.h" #include "base/shared_memory.h" +class MessageLoop; + // Manages a segment of shared memory that contains the Greasemonkey scripts the -// user has installed. -class GreasemonkeyMaster { +// user has installed. Lives on the UI thread. +class GreasemonkeyMaster : public base::RefCounted, + public DirectoryWatcher::Delegate { public: - GreasemonkeyMaster() - : shared_memory_serial_(0) {} - - // Reloads scripts from disk into a new chunk of shared memory and notifies - // renderers. - bool UpdateScripts(); + // For testability, the constructor takes the MessageLoop to run the + // script-reloading worker on as well as the path the scripts live in. + // These are normally the file thread and DIR_USER_SCRIPTS. + GreasemonkeyMaster(MessageLoop* worker, const FilePath& script_dir); + ~GreasemonkeyMaster(); // Creates a handle to the shared memory that can be used in the specified // process. @@ -29,15 +32,43 @@ class GreasemonkeyMaster { return shared_memory_.get(); } + // Called by the script reloader when new scripts have been loaded. + void NewScriptsAvailable(SharedMemory* handle); + + // Return true if we have any scripts ready. + bool ScriptsReady() const { return shared_memory_.get() != NULL; } + private: - // Contains the scripts that were found the last time UpdateScripts() was - // called. + class ScriptReloader; + + // DirectoryWatcher::Delegate implementation. + virtual void OnDirectoryChanged(const FilePath& path); + + // Kicks off a process on the file thread to reload scripts from disk + // into a new chunk of shared memory and notify renderers. + void StartScan(); + + // The directory containing user scripts. + scoped_ptr user_script_dir_; + + // The watcher watches the profile's user scripts directory for new scripts. + scoped_ptr dir_watcher_; + + // The MessageLoop that the scanner worker runs on. + // Typically the file thread; configurable for testing. + MessageLoop* worker_loop_; + + // ScriptReloader (in another thread) reloads script off disk. + // We hang on to our pointer to know if we've already got one running. + scoped_refptr script_reloader_; + + // Contains the scripts that were found the last time scripts were updated. scoped_ptr shared_memory_; - // A counter that is incremented each time a new shared memory segment is - // created. This is used to uniquely identify segments created at different - // times by this class. - int shared_memory_serial_; + // If the script directory is modified while we're rescanning it, we note + // that we're currently mid-scan and then start over again once the scan + // finishes. This boolean tracks whether another scan is pending. + bool pending_scan_; DISALLOW_COPY_AND_ASSIGN(GreasemonkeyMaster); }; diff --git a/chrome/browser/greasemonkey_master_unittest.cc b/chrome/browser/greasemonkey_master_unittest.cc new file mode 100644 index 0000000..b3ce12f --- /dev/null +++ b/chrome/browser/greasemonkey_master_unittest.cc @@ -0,0 +1,118 @@ +// Copyright (c) 2008 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 "chrome/browser/greasemonkey_master.h" + +#include + +#include "base/file_path.h" +#include "base/file_util.h" +#include "base/message_loop.h" +#include "base/path_service.h" +#include "base/string_util.h" +#include "chrome/common/notification_service.h" +#include "testing/gtest/include/gtest/gtest.h" + +// Test bringing up a master on a specific directory, putting a script in there, etc. + +class GreasemonkeyMasterTest : public testing::Test, + public NotificationObserver { + public: + GreasemonkeyMasterTest() : shared_memory_(NULL) {} + + virtual void SetUp() { + // Name a subdirectory of the temp directory. + std::wstring path_str; + ASSERT_TRUE(PathService::Get(base::DIR_TEMP, &path_str)); + script_dir_ = FilePath(path_str).Append( + FILE_PATH_LITERAL("GreasemonkeyTest")); + + // Create a fresh, empty copy of this directory. + file_util::Delete(script_dir_.value(), true); + file_util::CreateDirectory(script_dir_.value()); + + // Register for all user script notifications. + NotificationService::current()->AddObserver(this, + NOTIFY_NEW_USER_SCRIPTS, + NotificationService::AllSources()); + } + + virtual void TearDown() { + NotificationService::current()->RemoveObserver(this, + NOTIFY_NEW_USER_SCRIPTS, + NotificationService::AllSources()); + + // Clean up test directory. + ASSERT_TRUE(file_util::Delete(script_dir_.value(), true)); + ASSERT_FALSE(file_util::PathExists(script_dir_.value())); + } + + virtual void Observe(NotificationType type, + const NotificationSource& source, + const NotificationDetails& details) { + DCHECK(type == NOTIFY_NEW_USER_SCRIPTS); + + shared_memory_ = Details(details).ptr(); + if (MessageLoop::current() == &message_loop_) + MessageLoop::current()->Quit(); + } + + // MessageLoop used in tests. + MessageLoop message_loop_; + + // Directory containing user scripts. + FilePath script_dir_; + + // Updated to the script shared memory when we get notified. + SharedMemory* shared_memory_; +}; + +// Test that we *don't* get spurious notifications. +TEST_F(GreasemonkeyMasterTest, NoScripts) { + // Set shared_memory_ to something non-NULL, so we can check it became NULL. + shared_memory_ = reinterpret_cast(1); + + scoped_refptr master( + new GreasemonkeyMaster(MessageLoop::current(), script_dir_)); + message_loop_.PostTask(FROM_HERE, new MessageLoop::QuitTask); + message_loop_.Run(); + + // There were no scripts in the script dir, so we shouldn't have gotten + // a notification. + ASSERT_EQ(NULL, shared_memory_); +} + +// Test that we get notified about new scripts after they're added. +TEST_F(GreasemonkeyMasterTest, NewScripts) { + scoped_refptr master( + new GreasemonkeyMaster(MessageLoop::current(), script_dir_)); + + FilePath path = script_dir_.Append(FILE_PATH_LITERAL("script.user.js")); + + std::ofstream file; + file.open(WideToUTF8(path.value()).c_str()); + file << "some content"; + file.close(); + + message_loop_.Run(); + + ASSERT_TRUE(shared_memory_ != NULL); +} + +// Test that we get notified about scripts if they're already in the test dir. +TEST_F(GreasemonkeyMasterTest, ExistingScripts) { + FilePath path = script_dir_.Append(FILE_PATH_LITERAL("script.user.js")); + std::ofstream file; + file.open(WideToUTF8(path.value()).c_str()); + file << "some content"; + file.close(); + + scoped_refptr master( + new GreasemonkeyMaster(MessageLoop::current(), script_dir_)); + + message_loop_.PostTask(FROM_HERE, new MessageLoop::QuitTask); + message_loop_.Run(); + + ASSERT_TRUE(shared_memory_ != NULL); +} \ No newline at end of file diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc index 6023a2c..a81cdd8 100644 --- a/chrome/browser/profile.cc +++ b/chrome/browser/profile.cc @@ -5,6 +5,7 @@ #include "chrome/browser/profile.h" #include "base/command_line.h" +#include "base/file_path.h" #include "base/file_util.h" #include "base/lock.h" #include "base/path_service.h" @@ -15,6 +16,7 @@ #include "chrome/browser/browser_list.h" #include "chrome/browser/browser_process.h" #include "chrome/browser/download/download_manager.h" +#include "chrome/browser/greasemonkey_master.h" #include "chrome/browser/history/history.h" #include "chrome/browser/navigation_controller.h" #include "chrome/browser/profile_manager.h" @@ -378,6 +380,10 @@ class OffTheRecordProfileImpl : public Profile, return profile_->GetVisitedLinkMaster(); } + virtual GreasemonkeyMaster* GetGreasemonkeyMaster() { + return profile_->GetGreasemonkeyMaster(); + } + virtual HistoryService* GetHistoryService(ServiceAccessType sat) { if (sat == EXPLICIT_ACCESS) { return profile_->GetHistoryService(sat); @@ -670,6 +676,19 @@ VisitedLinkMaster* ProfileImpl::GetVisitedLinkMaster() { return visited_link_master_.get(); } +GreasemonkeyMaster* ProfileImpl::GetGreasemonkeyMaster() { + if (!greasemonkey_master_.get()) { + std::wstring script_dir_str; + PathService::Get(chrome::DIR_USER_SCRIPTS, &script_dir_str); + FilePath script_dir(script_dir_str); + greasemonkey_master_ = + new GreasemonkeyMaster(g_browser_process->file_thread()->message_loop(), + script_dir); + } + + return greasemonkey_master_.get(); +} + PrefService* ProfileImpl::GetPrefs() { if (!prefs_.get()) { prefs_.reset(new PrefService(GetPrefFilePath())); diff --git a/chrome/browser/profile.h b/chrome/browser/profile.h index 27e5fcf..2dbc055 100644 --- a/chrome/browser/profile.h +++ b/chrome/browser/profile.h @@ -21,6 +21,7 @@ class BookmarkModel; class DownloadManager; +class GreasemonkeyMaster; class HistoryService; class NavigationController; class PrefService; @@ -98,6 +99,11 @@ class Profile { // that this method is called. virtual VisitedLinkMaster* GetVisitedLinkMaster() = 0; + // Retrieves a pointer to the GreasemonkeyMaster associated with this + // profile. The GreasemonkeyMaster is lazily created the first time + // that this method is called. + virtual GreasemonkeyMaster* GetGreasemonkeyMaster() = 0; + // Retrieves a pointer to the HistoryService associated with this // profile. The HistoryService is lazily created the first time // that this method is called. @@ -235,6 +241,7 @@ class ProfileImpl : public Profile { virtual Profile* GetOffTheRecordProfile(); virtual Profile* GetOriginalProfile(); virtual VisitedLinkMaster* GetVisitedLinkMaster(); + virtual GreasemonkeyMaster* GetGreasemonkeyMaster(); virtual HistoryService* GetHistoryService(ServiceAccessType sat); virtual WebDataService* GetWebDataService(ServiceAccessType sat); virtual PrefService* GetPrefs(); @@ -282,6 +289,7 @@ class ProfileImpl : public Profile { std::wstring path_; bool off_the_record_; scoped_ptr visited_link_master_; + scoped_refptr greasemonkey_master_; scoped_ptr prefs_; scoped_ptr template_url_fetcher_; scoped_ptr template_url_model_; diff --git a/chrome/browser/render_process_host.cc b/chrome/browser/render_process_host.cc index 62a73ee..22ab752 100644 --- a/chrome/browser/render_process_host.cc +++ b/chrome/browser/render_process_host.cc @@ -469,14 +469,15 @@ void RenderProcessHost::InitGreasemonkeyScripts(HANDLE target_process) { // - File IO should be asynchronous (see VisitedLinkMaster), but how do we // get scripts to the first renderer without blocking startup? Should we // cache some information across restarts? - GreasemonkeyMaster* greasemonkey_master = - Singleton::get(); + GreasemonkeyMaster* greasemonkey_master = profile_->GetGreasemonkeyMaster(); if (!greasemonkey_master) { return; } - // TODO(aa): This does blocking IO. Move to background thread. - greasemonkey_master->UpdateScripts(); + if (!greasemonkey_master->ScriptsReady()) { + // No scripts ready. :( + return; + } SharedMemoryHandle handle_for_process = NULL; greasemonkey_master->ShareToProcess(target_process, &handle_for_process); diff --git a/chrome/common/notification_types.h b/chrome/common/notification_types.h index 29ba6e1..05160bb 100644 --- a/chrome/common/notification_types.h +++ b/chrome/common/notification_types.h @@ -477,6 +477,12 @@ enum NotificationType { // Personalization ----------------------------------------------------------- NOTIFY_PERSONALIZATION, + // Greasemonkey user scripts ------------------------------------------------- + + // Sent when there are new user scripts available. + // The details are a pointer to SharedMemory containing the new scripts. + NOTIFY_NEW_USER_SCRIPTS, + // Count (must be last) ------------------------------------------------------ // Used to determine the number of notification types. Not valid as // a type parameter when registering for or posting notifications. diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h index 9eff18d..a93708f 100644 --- a/chrome/test/testing_profile.h +++ b/chrome/test/testing_profile.h @@ -66,6 +66,9 @@ class TestingProfile : public Profile { virtual VisitedLinkMaster* GetVisitedLinkMaster() { return NULL; } + virtual GreasemonkeyMaster* GetGreasemonkeyMaster() { + return NULL; + } virtual HistoryService* GetHistoryService(ServiceAccessType access) { return history_service_.get(); } diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj index 12b44e7..3673ffc 100644 --- a/chrome/test/unit/unittests.vcproj +++ b/chrome/test/unit/unittests.vcproj @@ -186,23 +186,23 @@ > + + + + -- cgit v1.1