From e11c51eede1cb6a9707fcee08f791d767dce6655 Mon Sep 17 00:00:00 2001 From: wittman Date: Thu, 13 Aug 2015 17:57:07 -0700 Subject: [PATCH] ChromeContentsRulesRegistry: encapsulate predicate evaluation state/logic Moves all logic and state for predicate evaluation into the respective trackers, so that ChromeContentsRulesRegistry no longer needs to serve as a middleman for tracker-specific evaluation details. This is in support of step 6 in the associated bug. No functional change is intended. BUG=492946 Review URL: https://codereview.chromium.org/1287373002 Cr-Commit-Position: refs/heads/master@{#343311} --- .../chrome_content_rules_registry.cc | 18 +- .../declarative_content_css_condition_tracker.cc | 34 ++-- .../declarative_content_css_condition_tracker.h | 18 +- ...ative_content_css_condition_tracker_unittest.cc | 47 ++--- ...tive_content_is_bookmarked_condition_tracker.cc | 10 +- ...ative_content_is_bookmarked_condition_tracker.h | 11 +- ...ent_is_bookmarked_condition_tracker_unittest.cc | 195 ++++++++++++--------- ...clarative_content_page_url_condition_tracker.cc | 17 +- ...eclarative_content_page_url_condition_tracker.h | 13 +- ..._content_page_url_condition_tracker_unittest.cc | 33 ++-- 10 files changed, 207 insertions(+), 189 deletions(-) diff --git a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc index e1e4f5b25fa7..46c9d6bd5309 100644 --- a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc +++ b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.cc @@ -307,12 +307,6 @@ bool ChromeContentRulesRegistry::ManagingRulesForBrowserContext( std::set ChromeContentRulesRegistry::GetMatchingRules(content::WebContents* tab) const { - std::set page_url_matches; - base::hash_set css_selectors; - page_url_condition_tracker_.GetMatches(tab, &page_url_matches); - css_condition_tracker_.GetMatchingCssSelectors(tab, &css_selectors); - const bool is_bookmarked = - is_bookmarked_condition_tracker_.IsUrlBookmarked(tab); const bool is_incognito_tab = tab->GetBrowserContext()->IsOffTheRecord(); std::set matching_rules; for (const RulesMap::value_type& rule_id_rule_pair : content_rules_) { @@ -323,18 +317,24 @@ ChromeContentRulesRegistry::GetMatchingRules(content::WebContents* tab) const { for (const ContentCondition* condition : rule->conditions) { if (condition->page_url_predicate && - !condition->page_url_predicate->Evaluate(page_url_matches)) { + !page_url_condition_tracker_.EvaluatePredicate( + condition->page_url_predicate.get(), + tab)) { continue; } if (condition->css_predicate && - !condition->css_predicate->Evaluate(css_selectors)) { + !css_condition_tracker_.EvaluatePredicate( + condition->css_predicate.get(), + tab)) { continue; } if (condition->is_bookmarked_predicate && !condition->is_bookmarked_predicate->IsIgnored() && - !condition->is_bookmarked_predicate->Evaluate(is_bookmarked)) { + !is_bookmarked_condition_tracker_.EvaluatePredicate( + condition->is_bookmarked_predicate.get(), + tab)) { continue; } diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc index 943b75c817ce..733b8c148dc1 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.cc @@ -36,17 +36,6 @@ const char kInvalidTypeOfParameter[] = "Attribute '%s' has an invalid type"; DeclarativeContentCssPredicate::~DeclarativeContentCssPredicate() { } -bool DeclarativeContentCssPredicate::Evaluate( - const base::hash_set& matched_css_selectors) const { - // All attributes must be fulfilled for a fulfilled condition. - for (const std::string& css_selector : css_selectors_) { - if (!ContainsKey(matched_css_selectors, css_selector)) - return false; - } - - return true; -} - // static scoped_ptr DeclarativeContentCssPredicate::Create( @@ -128,7 +117,9 @@ OnWebContentsNavigation(const content::LoadCommittedDetails& details, void DeclarativeContentCssConditionTracker::PerWebContentsTracker:: UpdateMatchingCssSelectorsForTesting( const std::vector& matching_css_selectors) { - matching_css_selectors_ = matching_css_selectors; + matching_css_selectors_.clear(); + matching_css_selectors_.insert(matching_css_selectors.begin(), + matching_css_selectors.end()); request_evaluation_.Run(web_contents()); } @@ -154,7 +145,8 @@ void DeclarativeContentCssConditionTracker::PerWebContentsTracker:: OnWatchedPageChange( const std::vector& css_selectors) { - matching_css_selectors_ = css_selectors; + matching_css_selectors_.clear(); + matching_css_selectors_.insert(css_selectors.begin(), css_selectors.end()); request_evaluation_.Run(web_contents()); } @@ -219,15 +211,19 @@ void DeclarativeContentCssConditionTracker::OnWebContentsNavigation( per_web_contents_tracker_[contents]->OnWebContentsNavigation(details, params); } -void DeclarativeContentCssConditionTracker::GetMatchingCssSelectors( - content::WebContents* contents, - base::hash_set* css_selectors) const { +bool DeclarativeContentCssConditionTracker::EvaluatePredicate( + const DeclarativeContentCssPredicate* predicate, + content::WebContents* contents) const { auto loc = per_web_contents_tracker_.find(contents); DCHECK(loc != per_web_contents_tracker_.end()); - const std::vector& matching_css_selectors = + const base::hash_set& matching_css_selectors = loc->second->matching_css_selectors(); - css_selectors->insert(matching_css_selectors.begin(), - matching_css_selectors.end()); + for (const std::string& predicate_css_selector : predicate->css_selectors()) { + if (!ContainsKey(matching_css_selectors, predicate_css_selector)) + return false; + } + + return true; } void DeclarativeContentCssConditionTracker:: diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h index 3bba9522a117..599f8b1bdc2e 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker.h @@ -43,10 +43,6 @@ class DeclarativeContentCssPredicate { return css_selectors_; } - // Evaluate for all watched CSS selectors that match on frames with the same - // origin as the page's main frame. - bool Evaluate(const base::hash_set& matched_css_selectors) const; - static scoped_ptr Create( const base::Value& value, std::string* error); @@ -96,11 +92,10 @@ class DeclarativeContentCssConditionTracker const content::LoadCommittedDetails& details, const content::FrameNavigateParams& params); - // Inserts the currently-matching CSS selectors into |css_selectors|. We use - // hash_set for maximally efficient lookup. - void GetMatchingCssSelectors( - content::WebContents* contents, - base::hash_set* css_selectors) const; + // Returns the result of evaluating |predicate| on the per-tab state + // associated with |contents|. + bool EvaluatePredicate(const DeclarativeContentCssPredicate* predicate, + content::WebContents* contents) const; // TODO(wittman): Remove once DeclarativeChromeContentRulesRegistry no longer // depends on concrete condition implementations. At that point @@ -133,7 +128,7 @@ class DeclarativeContentCssConditionTracker void UpdateMatchingCssSelectorsForTesting( const std::vector& matching_css_selectors); - const std::vector& matching_css_selectors() const { + const base::hash_set& matching_css_selectors() const { return matching_css_selectors_; } @@ -147,7 +142,8 @@ class DeclarativeContentCssConditionTracker const RequestEvaluationCallback request_evaluation_; const WebContentsDestroyedCallback web_contents_destroyed_; - std::vector matching_css_selectors_; + // We use a hash_set for maximally efficient lookup. + base::hash_set matching_css_selectors_; DISALLOW_COPY_AND_ASSIGN(PerWebContentsTracker); }; diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc index d9446a444ef1..8fe8ac0c4abb 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_css_condition_tracker_unittest.cc @@ -16,6 +16,7 @@ namespace extensions { using testing::HasSubstr; +using testing::UnorderedElementsAre; using testing::UnorderedElementsAreArray; class DeclarativeContentCssConditionTrackerTest @@ -92,19 +93,12 @@ TEST(DeclarativeContentCssPredicateTest, CssPredicate) { std::string error; scoped_ptr predicate = DeclarativeContentCssPredicate::Create( - *base::test::ParseJson("[\"input\"]"), + *base::test::ParseJson("[\"input\", \"a\"]"), &error); EXPECT_EQ("", error); ASSERT_TRUE(predicate); - base::hash_set matched_css_selectors; - matched_css_selectors.insert("input"); - - EXPECT_TRUE(predicate->Evaluate(matched_css_selectors)); - - matched_css_selectors.clear(); - matched_css_selectors.insert("body"); - EXPECT_FALSE(predicate->Evaluate(matched_css_selectors)); + EXPECT_THAT(predicate->css_selectors(), UnorderedElementsAre("input", "a")); } // Tests the basic flow of operations on the @@ -128,18 +122,30 @@ TEST_F(DeclarativeContentCssConditionTrackerTest, Basic) { }); EXPECT_EQ(expected_evaluation_requests, delegate_.evaluation_requests()); + std::string error; + scoped_ptr div_predicate = + DeclarativeContentCssPredicate::Create( + *base::test::ParseJson("[\"div\"]"), + &error); + EXPECT_EQ("", error); + ASSERT_TRUE(div_predicate); + + scoped_ptr a_predicate = + DeclarativeContentCssPredicate::Create( + *base::test::ParseJson("[\"a\"]"), + &error); + EXPECT_EQ("", error); + ASSERT_TRUE(a_predicate); + // Check that receiving an OnWatchedPageChange message from the tab results in // a request for condition evaluation. const std::vector matched_selectors(1, "div"); SendOnWatchedPageChangeMessage(tab.get(), matched_selectors); EXPECT_EQ(++expected_evaluation_requests, delegate_.evaluation_requests()); - // Check that GetMatchingCssSelectors produces the same matched selectors as - // were sent by the OnWatchedPageChange message. - base::hash_set matching_selectors; - tracker.GetMatchingCssSelectors(tab.get(), &matching_selectors); - EXPECT_THAT(matching_selectors, - UnorderedElementsAreArray(matched_selectors)); + // Check that only the div predicate matches. + EXPECT_TRUE(tracker.EvaluatePredicate(div_predicate.get(), tab.get())); + EXPECT_FALSE(tracker.EvaluatePredicate(a_predicate.get(), tab.get())); EXPECT_EQ(expected_evaluation_requests, delegate_.evaluation_requests()); // Check that an in-page navigation has no effect on the matching selectors. @@ -148,10 +154,8 @@ TEST_F(DeclarativeContentCssConditionTrackerTest, Basic) { details.is_in_page = true; content::FrameNavigateParams params; tracker.OnWebContentsNavigation(tab.get(), details, params); - matching_selectors.clear(); - tracker.GetMatchingCssSelectors(tab.get(), &matching_selectors); - EXPECT_THAT(matching_selectors, - UnorderedElementsAreArray(matched_selectors)); + EXPECT_TRUE(tracker.EvaluatePredicate(div_predicate.get(), tab.get())); + EXPECT_FALSE(tracker.EvaluatePredicate(a_predicate.get(), tab.get())); EXPECT_EQ(expected_evaluation_requests, delegate_.evaluation_requests()); } @@ -162,9 +166,8 @@ TEST_F(DeclarativeContentCssConditionTrackerTest, Basic) { details.is_in_page = false; content::FrameNavigateParams params; tracker.OnWebContentsNavigation(tab.get(), details, params); - matching_selectors.clear(); - tracker.GetMatchingCssSelectors(tab.get(), &matching_selectors); - EXPECT_TRUE(matching_selectors.empty()); + EXPECT_FALSE(tracker.EvaluatePredicate(div_predicate.get(), tab.get())); + EXPECT_FALSE(tracker.EvaluatePredicate(a_predicate.get(), tab.get())); EXPECT_EQ(++expected_evaluation_requests, delegate_.evaluation_requests()); } diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.cc index 8640da440734..e57f5b979a77 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.cc @@ -42,11 +42,6 @@ bool DeclarativeContentIsBookmarkedPredicate::IsIgnored() const { return !HasBookmarkAPIPermission(extension_.get()); } -bool DeclarativeContentIsBookmarkedPredicate::Evaluate( - bool url_is_bookmarked) const { - return url_is_bookmarked == is_bookmarked_; -} - // static scoped_ptr DeclarativeContentIsBookmarkedPredicate::Create( @@ -191,11 +186,12 @@ void DeclarativeContentIsBookmarkedConditionTracker::OnWebContentsNavigation( per_web_contents_tracker_[contents]->UpdateState(true); } -bool DeclarativeContentIsBookmarkedConditionTracker::IsUrlBookmarked( +bool DeclarativeContentIsBookmarkedConditionTracker::EvaluatePredicate( + const DeclarativeContentIsBookmarkedPredicate* predicate, content::WebContents* contents) const { auto loc = per_web_contents_tracker_.find(contents); DCHECK(loc != per_web_contents_tracker_.end()); - return loc->second->is_url_bookmarked(); + return loc->second->is_url_bookmarked() == predicate->is_bookmarked(); } void DeclarativeContentIsBookmarkedConditionTracker::BookmarkModelChanged() {} diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.h b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.h index 11f6653b264c..c5cc7f1494a7 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.h +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker.h @@ -36,8 +36,8 @@ class DeclarativeContentIsBookmarkedPredicate { ~DeclarativeContentIsBookmarkedPredicate(); bool IsIgnored() const; - // Evaluate for URL bookmarked state. - bool Evaluate(bool url_is_bookmarked) const; + + bool is_bookmarked() const { return is_bookmarked_; } static scoped_ptr Create( const Extension* extension, @@ -86,8 +86,11 @@ class DeclarativeContentIsBookmarkedConditionTracker const content::LoadCommittedDetails& details, const content::FrameNavigateParams& params); - // Returns true if |contents| current URL is bookmarked. - bool IsUrlBookmarked(content::WebContents* contents) const; + // Returns the result of evaluating |predicate| on the per-tab state + // associated with |contents|. + bool EvaluatePredicate( + const DeclarativeContentIsBookmarkedPredicate* predicate, + content::WebContents* contents) const; private: class PerWebContentsTracker : public content::WebContentsObserver { diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker_unittest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker_unittest.cc index 13f795c573da..6fa3f538ac4c 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker_unittest.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_is_bookmarked_condition_tracker_unittest.cc @@ -8,6 +8,7 @@ #include #include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" #include "base/stl_util.h" #include "base/strings/utf_string_conversions.h" @@ -44,6 +45,21 @@ scoped_refptr CreateExtensionWithBookmarksPermission( .Build(); } +scoped_ptr CreatePredicate( + const Extension* extension, + bool is_bookmarked) { + std::string error; + scoped_ptr predicate = + DeclarativeContentIsBookmarkedPredicate::Create( + extension, + *base::test::ParseJson(is_bookmarked ? "true" : "false"), + &error); + EXPECT_EQ("", error); + EXPECT_TRUE(predicate); + EXPECT_EQ(is_bookmarked, predicate->is_bookmarked()); + return predicate; +} + } // namespace using testing::HasSubstr; @@ -83,6 +99,12 @@ class DeclarativeContentIsBookmarkedConditionTrackerTest bookmarks::test::WaitForBookmarkModelToLoad( BookmarkModelFactory::GetForProfile(profile())); bookmark_model_ = BookmarkModelFactory::GetForProfile(profile()); + tracker_.reset(new DeclarativeContentIsBookmarkedConditionTracker( + profile(), + &delegate_)); + extension_ = CreateExtensionWithBookmarksPermission(true); + is_bookmarked_predicate_ = CreatePredicate(extension_.get(), true); + is_not_bookmarked_predicate_ = CreatePredicate(extension_.get(), false); } void LoadURL(content::WebContents* tab, const GURL& url) { @@ -90,8 +112,43 @@ class DeclarativeContentIsBookmarkedConditionTrackerTest ui::PAGE_TRANSITION_LINK, std::string()); } + testing::AssertionResult CheckPredicates(content::WebContents* tab, + bool page_is_bookmarked) { + const bool is_bookmarked_predicate_success = + page_is_bookmarked == + tracker_->EvaluatePredicate(is_bookmarked_predicate_.get(), tab); + const bool is_not_bookmarked_predicate_success = + page_is_bookmarked != + tracker_->EvaluatePredicate(is_not_bookmarked_predicate_.get(), tab); + + if (is_bookmarked_predicate_success && is_not_bookmarked_predicate_success) + return testing::AssertionSuccess(); + + testing::AssertionResult result = testing::AssertionFailure(); + if (!is_bookmarked_predicate_success) { + result << "IsBookmarkedPredicate(true): expected " + << (page_is_bookmarked ? "true" : "false") << " got " + << (page_is_bookmarked ? "false" : "true"); + } + + if (!is_not_bookmarked_predicate_success) { + if (!is_bookmarked_predicate_success) + result << "; "; + result << "IsBookmarkedPredicate(false): expected " + << (page_is_bookmarked ? "false" : "true") << " got " + << (page_is_bookmarked ? "true" : "false"); + } + + return result; + } + Delegate delegate_; bookmarks::BookmarkModel* bookmark_model_; + scoped_ptr tracker_; + scoped_refptr extension_; + scoped_ptr is_bookmarked_predicate_; + scoped_ptr + is_not_bookmarked_predicate_; private: DISALLOW_COPY_AND_ASSIGN(DeclarativeContentIsBookmarkedConditionTrackerTest); @@ -128,46 +185,26 @@ TEST(DeclarativeContentIsBookmarkedPredicateTest, EXPECT_FALSE(predicate); } -// Tests isBookmark: true. +// Tests isBookmark: true. Predicate state is checked in CreatePredicate(). TEST(DeclarativeContentIsBookmarkedPredicateTest, IsBookmarkedPredicateTrue) { scoped_refptr extension = CreateExtensionWithBookmarksPermission(true); - std::string error; scoped_ptr predicate = - DeclarativeContentIsBookmarkedPredicate::Create( - extension.get(), - *base::test::ParseJson("true"), - &error); - EXPECT_EQ("", error); - ASSERT_TRUE(predicate); - - EXPECT_TRUE(predicate->Evaluate(true /* url_is_bookmarked */)); - EXPECT_FALSE(predicate->Evaluate(false /* url_is_bookmarked */)); + CreatePredicate(extension.get(), true); } -// Tests isBookmark: false. +// Tests isBookmark: false. Predicate state is checked in CreatePredicate(). TEST(DeclarativeContentIsBookmarkedPredicateTest, IsBookmarkedPredicateFalse) { scoped_refptr extension = CreateExtensionWithBookmarksPermission(true); - std::string error; scoped_ptr predicate = - DeclarativeContentIsBookmarkedPredicate::Create( - extension.get(), - *base::test::ParseJson("false"), - &error); - EXPECT_EQ("", error); - ASSERT_TRUE(predicate); - - EXPECT_FALSE(predicate->Evaluate(true /* url_is_bookmarked */)); - EXPECT_TRUE(predicate->Evaluate(false /* url_is_bookmarked */)); + CreatePredicate(extension.get(), false); } // Tests that starting tracking for a WebContents that has a bookmarked URL // results in the proper IsUrlBookmarked state. TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, BookmarkedAtStartOfTracking) { - DeclarativeContentIsBookmarkedConditionTracker tracker(profile(), &delegate_); - scoped_ptr tab = MakeTab(); LoadURL(tab.get(), GURL("http://bookmarked/")); EXPECT_TRUE(delegate_.evaluation_requests().empty()); @@ -176,36 +213,34 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, base::ASCIIToUTF16("title"), GURL("http://bookmarked/")); - tracker.TrackForWebContents(tab.get()); + tracker_->TrackForWebContents(tab.get()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tab.get())); - EXPECT_TRUE(tracker.IsUrlBookmarked(tab.get())); + EXPECT_TRUE(CheckPredicates(tab.get(), true)); } // Tests that adding and removing bookmarks triggers evaluation requests for the // matching WebContents. TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, AddAndRemoveBookmark) { - DeclarativeContentIsBookmarkedConditionTracker tracker(profile(), &delegate_); - // Create two tabs. ScopedVector tabs; for (int i = 0; i < 2; ++i) { tabs.push_back(MakeTab()); delegate_.evaluation_requests().clear(); - tracker.TrackForWebContents(tabs.back()); + tracker_->TrackForWebContents(tabs.back()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs.back())); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs.back())); + EXPECT_TRUE(CheckPredicates(tabs.back(), false)); } // Navigate the first tab to a URL that we will bookmark. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://bookmarked/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Bookmark the first tab's URL. delegate_.evaluation_requests().clear(); @@ -214,41 +249,39 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, base::ASCIIToUTF16("title"), GURL("http://bookmarked/")); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Remove the bookmark. delegate_.evaluation_requests().clear(); bookmark_model_->Remove(node); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } // Tests that adding and removing bookmarks triggers evaluation requests for the // matching WebContents. TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, ExtensiveChanges) { - DeclarativeContentIsBookmarkedConditionTracker tracker(profile(), &delegate_); - // Create two tabs. ScopedVector tabs; for (int i = 0; i < 2; ++i) { tabs.push_back(MakeTab()); delegate_.evaluation_requests().clear(); - tracker.TrackForWebContents(tabs.back()); + tracker_->TrackForWebContents(tabs.back()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs.back())); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs.back())); + EXPECT_TRUE(CheckPredicates(tabs.back(), false)); } // Navigate the first tab to a URL that we will bookmark. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://bookmarked/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); { // Check that evaluation requests occur outside ExtensiveBookmarkChanges for @@ -260,12 +293,12 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, ExtensiveChanges) { base::ASCIIToUTF16("title"), GURL("http://bookmarked/")); EXPECT_TRUE(delegate_.evaluation_requests().empty()); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); bookmark_model_->EndExtensiveChanges(); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Check that evaluation requests occur outside ExtensiveBookmarkChanges for // removed nodes. @@ -273,12 +306,12 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, ExtensiveChanges) { bookmark_model_->BeginExtensiveChanges(); bookmark_model_->Remove(node); EXPECT_TRUE(delegate_.evaluation_requests().empty()); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); bookmark_model_->EndExtensiveChanges(); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } { @@ -292,12 +325,12 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, ExtensiveChanges) { base::ASCIIToUTF16("title"), GURL("http://bookmarked/")); EXPECT_TRUE(delegate_.evaluation_requests().empty()); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Check that evaluation requests occur outside ScopedGroupBookmarkActions // for removed nodes. @@ -306,20 +339,18 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, ExtensiveChanges) { bookmarks::ScopedGroupBookmarkActions scoped_group(bookmark_model_); bookmark_model_->Remove(node); EXPECT_TRUE(delegate_.evaluation_requests().empty()); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } } // Tests that navigation to bookmarked and non-bookmarked URLs triggers // evaluation requests for the relevant WebContents. TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, Navigation) { - DeclarativeContentIsBookmarkedConditionTracker tracker(profile(), &delegate_); - // Bookmark two URLs. delegate_.evaluation_requests().clear(); bookmark_model_->AddURL(bookmark_model_->other_node(), 0, @@ -334,51 +365,51 @@ TEST_F(DeclarativeContentIsBookmarkedConditionTrackerTest, Navigation) { for (int i = 0; i < 2; ++i) { tabs.push_back(MakeTab()); delegate_.evaluation_requests().clear(); - tracker.TrackForWebContents(tabs.back()); + tracker_->TrackForWebContents(tabs.back()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs.back())); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs.back())); + EXPECT_TRUE(CheckPredicates(tabs.back(), false)); } // Navigate the first tab to one bookmarked URL. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://bookmarked1/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Navigate the first tab to another bookmarked URL. The contents have // changed, so we should receive a new evaluation request even though the // bookmarked state hasn't. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://bookmarked2/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_TRUE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], true)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Navigate the first tab to a non-bookmarked URL. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://not-bookmarked1/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); // Navigate the first tab to another non-bookmarked URL. The contents have // changed, so we should receive a new evaluation request even though the // bookmarked state hasn't. delegate_.evaluation_requests().clear(); LoadURL(tabs[0], GURL("http://not-bookmarked2/")); - tracker.OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), - content::FrameNavigateParams()); + tracker_->OnWebContentsNavigation(tabs[0], content::LoadCommittedDetails(), + content::FrameNavigateParams()); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[0])); - EXPECT_FALSE(tracker.IsUrlBookmarked(tabs[1])); + EXPECT_TRUE(CheckPredicates(tabs[0], false)); + EXPECT_TRUE(CheckPredicates(tabs[1], false)); } } // namespace extensions diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.cc index 555fb60cca12..dd94d74a363b 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.cc @@ -29,12 +29,6 @@ static url_matcher::URLMatcherConditionSet::ID g_next_id = 0; DeclarativeContentPageUrlPredicate::~DeclarativeContentPageUrlPredicate() { } -bool DeclarativeContentPageUrlPredicate::Evaluate( - const std::set& - page_url_matches) const { - return ContainsKey(page_url_matches, url_matcher_condition_set_->id()); -} - // static scoped_ptr DeclarativeContentPageUrlPredicate::Create( @@ -166,12 +160,15 @@ void DeclarativeContentPageUrlConditionTracker::OnWebContentsNavigation( per_web_contents_tracker_[contents]->UpdateMatchesForCurrentUrl(true); } -void DeclarativeContentPageUrlConditionTracker::GetMatches( - content::WebContents* contents, - std::set* matches) const { +bool DeclarativeContentPageUrlConditionTracker::EvaluatePredicate( + const DeclarativeContentPageUrlPredicate* predicate, + content::WebContents* contents) const { auto loc = per_web_contents_tracker_.find(contents); DCHECK(loc != per_web_contents_tracker_.end()); - *matches = loc->second->matches(); + const std::set& + web_contents_id_matches = loc->second->matches(); + return ContainsKey(web_contents_id_matches, + predicate->url_matcher_condition_set()->id()); } bool DeclarativeContentPageUrlConditionTracker::IsEmpty() const { diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.h b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.h index 6390d1bd743b..f143e93cc26d 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.h +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker.h @@ -40,11 +40,6 @@ class DeclarativeContentPageUrlPredicate { return url_matcher_condition_set_.get(); } - // Evaluate for match IDs for the URL of the top-level page of the renderer. - bool Evaluate( - const std::set& - page_url_matches) const; - static scoped_ptr Create( url_matcher::URLMatcherConditionFactory* url_matcher_condition_factory, const base::Value& value, @@ -115,10 +110,10 @@ class DeclarativeContentPageUrlConditionTracker { const content::LoadCommittedDetails& details, const content::FrameNavigateParams& params); - // Gets the maching IDs for the last navigation on WebContents. - void GetMatches( - content::WebContents* contents, - std::set* matches) const; + // Returns the result of evaluating |predicate| on the per-tab state + // associated with |contents|. + bool EvaluatePredicate(const DeclarativeContentPageUrlPredicate* predicate, + content::WebContents* contents) const; // Returns true if this object retains no allocated data. Only for debugging. bool IsEmpty() const; diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker_unittest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker_unittest.cc index 5964570d4aff..92287e7e74c6 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker_unittest.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_page_url_condition_tracker_unittest.cc @@ -101,8 +101,6 @@ TEST(DeclarativeContentPageUrlPredicateTest, PageUrlPredicate) { EXPECT_THAT( page_url_matches, ElementsAre(predicate->url_matcher_condition_set()->id())); - - EXPECT_TRUE(predicate->Evaluate(page_url_matches)); } // Tests that adding and removing condition sets trigger evaluation requests for @@ -125,26 +123,29 @@ TEST_F(DeclarativeContentPageUrlConditionTrackerTest, // add. LoadURL(tabs[0], GURL("http://test1/")); - const int condition_set_id = 100; - std::set conditions; - conditions.insert( - tracker.condition_factory()->CreateHostPrefixCondition("test1")); - std::vector> - condition_sets(1, - new url_matcher::URLMatcherConditionSet( - condition_set_id, conditions)); + std::string error; + scoped_ptr predicate = + DeclarativeContentPageUrlPredicate::Create( + tracker.condition_factory(), + *base::test::ParseJson("{\"hostPrefix\": \"test1\"}"), + &error); + EXPECT_EQ("", error); + ASSERT_TRUE(predicate); + delegate_.evaluation_requests().clear(); + url_matcher::URLMatcherConditionSet::Vector condition_sets( + 1, + predicate->url_matcher_condition_set()); tracker.AddConditionSets(condition_sets); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); - std::set match_ids; - tracker.GetMatches(tabs[0], &match_ids); - EXPECT_THAT(match_ids, UnorderedElementsAre(condition_set_id)); - tracker.GetMatches(tabs[1], &match_ids); - EXPECT_TRUE(match_ids.empty()); + EXPECT_TRUE(tracker.EvaluatePredicate(predicate.get(), tabs[0])); + EXPECT_FALSE(tracker.EvaluatePredicate(predicate.get(), tabs[1])); delegate_.evaluation_requests().clear(); - tracker.RemoveConditionSets(std::vector(1, condition_set_id)); + tracker.RemoveConditionSets(std::vector( + 1, + predicate->url_matcher_condition_set()->id())); EXPECT_THAT(delegate_.evaluation_requests(), UnorderedElementsAre(tabs[0])); } -- 2.11.4.GIT