Don't post tasks from RulesRegistry c-tor

chrome/browser/extensions/api/declarative/rules_registry.cc

 // Copyright (c) 2012 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/extensions/api/declarative/rules_registry.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 68 matching lines @@
     content::BrowserThread::ID owner_thread,
     RulesCacheDelegate* cache_delegate,
     const WebViewKey& webview_key)
     : profile_(profile),
       owner_thread_(owner_thread),
       event_name_(event_name),
       webview_key_(webview_key),
       weak_ptr_factory_(profile ? this : NULL),
       last_generated_rule_identifier_id_(0) {
   if (cache_delegate) {
+    ready_.reset(new OneShotEvent);
     cache_delegate_ = cache_delegate->GetWeakPtr();
     cache_delegate->Init(this);
-  } else {
-    content::BrowserThread::PostTask(
-        owner_thread,
-        FROM_HERE,
-        base::Bind(&RulesRegistry::MarkReady, this, base::Time::Now()));

[Jeffrey Yasskin, 2013/12/17 22:55:05]

Would this CL be simpler if you just called ready_.Signal() right here, instead
of Posting a call to MarkReady()?
I'm not particularly happy about needing to check ready_ for NULL instead of
just posting tasks to it. Removing the double-check is why OneShotEvent accepts
Post()s even after it's signaled.

[vabr (Chromium), 2013/12/18 12:01:51]

I'm afraid calling ready_.Signal() here would lead to a DCHECK failing
CalledOnValidThread as soon as ready_.Post() is used. This is because the
constructor runs on UI thread, but the rest runs on owner_thread (IO for
WebRequestRulesRegistry).

Currently, OneShotEvent detaches from the thread it's constructed on just to
allow constructing it on another thread than it's used to.

What I can do is:
1) Allow to construct it as already signalled, or
2) provide a OneShotEvent method to detach on demand.

Does anything from the above sound good to you? Any other suggestions?

   }
+  // Note that if |cache_delegate| is NULL, the Rulesregistry never signals its

[Jeffrey Yasskin, 2013/12/17 22:55:05]

capitalization: RulesRegistry

[vabr (Chromium), 2013/12/18 12:01:51]

Done.

+  // |ready_| event. That's OK, because that event is only ever needed by the
+  // cache delegate.
 }
 
 std::string RulesRegistry::AddRulesNoFill(
     const std::string& extension_id,
     const std::vector<linked_ptr<Rule> >& rules) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   // Verify that all rule IDs are new.
   for (std::vector<linked_ptr<Rule> >::const_iterator i =
       rules.begin(); i != rules.end(); ++i) {
@@ skipping 158 matching lines @@
 }
 
 void RulesRegistry::MarkReady(base::Time storage_init_time) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   if (!storage_init_time.is_null()) {
     UMA_HISTOGRAM_TIMES("Extensions.DeclarativeRulesStorageInitialization",
                         base::Time::Now() - storage_init_time);
   }
 
-  ready_.Signal();
+  ready_->Signal();
 }
 
 void RulesRegistry::ProcessChangedRules(const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
   DCHECK(ContainsKey(process_changed_rules_requested_, extension_id));
   process_changed_rules_requested_[extension_id] = NOT_SCHEDULED_FOR_PROCESSING;
 
   std::vector<linked_ptr<Rule> > new_rules;
   GetAllRules(extension_id, &new_rules);
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
       base::Bind(&RulesCacheDelegate::WriteToStorage,
                  cache_delegate_,
                  extension_id,
                  base::Passed(RulesToValue(new_rules))));
 }
 
 void RulesRegistry::MaybeProcessChangedRules(const std::string& extension_id) {
   // Read and initialize |process_changed_rules_requested_[extension_id]| if
   // necessary. (Note that the insertion below will not overwrite
   // |process_changed_rules_requested_[extension_id]| if that already exists.
+  bool processing_needed = profile_ && ready_;
   std::pair<ProcessStateMap::iterator, bool> insertion =
       process_changed_rules_requested_.insert(std::make_pair(
           extension_id,
-          profile_ ? NOT_SCHEDULED_FOR_PROCESSING : NEVER_PROCESS));
+          processing_needed ? NOT_SCHEDULED_FOR_PROCESSING : NEVER_PROCESS));
   if (insertion.first->second != NOT_SCHEDULED_FOR_PROCESSING)
     return;
 
   process_changed_rules_requested_[extension_id] = SCHEDULED_FOR_PROCESSING;
-  ready_.Post(FROM_HERE,
-              base::Bind(&RulesRegistry::ProcessChangedRules,
-                         weak_ptr_factory_.GetWeakPtr(),
-                         extension_id));
+  ready_->Post(FROM_HERE,
+               base::Bind(&RulesRegistry::ProcessChangedRules,
+                          weak_ptr_factory_.GetWeakPtr(),
+                          extension_id));
 }
 
 bool RulesRegistry::IsUniqueId(const std::string& extension_id,
                                const std::string& rule_id) const {
   RuleIdentifiersMap::const_iterator identifiers =
       used_rule_identifiers_.find(extension_id);
   if (identifiers == used_rule_identifiers_.end())
     return true;
   return identifiers->second.find(rule_id) == identifiers->second.end();
 }
@@ skipping 55 matching lines @@
   for (i = identifiers.begin(); i != identifiers.end(); ++i)
     used_rule_identifiers_[extension_id].erase(*i);
 }
 
 void RulesRegistry::RemoveAllUsedRuleIdentifiers(
     const std::string& extension_id) {
   used_rule_identifiers_.erase(extension_id);
 }
 
 }  // namespace extensions