From 64b78871a088973eb7dc4c36b19509a8526b5a71 Mon Sep 17 00:00:00 2001 From: thakis Date: Sat, 9 May 2015 13:16:42 -0700 Subject: [PATCH] Revert of Evaluate declarativeContent API rules on add/remove (patchset #3 id:40001 of https://codereview.chromium.org/1128323002/) Reason for revert: DeclarativeContentApiTest.Overview started to reliably fail on the Mac OS X 10.6 bot after this went in: http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/2062 http://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests/builds/2063 ../../chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:134: Failure Value of: page_action->GetIsVisible(tab_id) Actual: true Expected: false Original issue's description: > Evaluate declarativeContent API rules on add/remove > > Fixes bug where rules were not being evaluated until tabs' WebContents > were navigated. > > BUG=485172 > > Committed: https://crrev.com/b6780cb55862276ac886d6acfc6193f885d93f2e > Cr-Commit-Position: refs/heads/master@{#329048} TBR=kalman@chromium.org,wittman@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=485172 Review URL: https://codereview.chromium.org/1130793004 Cr-Commit-Position: refs/heads/master@{#329053} --- .../chrome_content_rules_registry.cc | 124 ++++++++---------- .../chrome_content_rules_registry.h | 20 +-- .../chrome_content_rules_registry_unittest.cc | 3 +- .../declarative_content_apitest.cc | 110 ++++------------ 4 files changed, 83 insertions(+), 174 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 12b0a7b1f4a8b4..aca2cd6ea71b3a 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 @@ -8,10 +8,7 @@ #include "chrome/browser/extensions/api/declarative_content/content_action.h" #include "chrome/browser/extensions/api/declarative_content/content_condition.h" #include "chrome/browser/extensions/api/declarative_content/content_constants.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/ui/browser.h" -#include "chrome/browser/ui/browser_iterator.h" -#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/browser/extensions/extension_tab_util.h" #include "content/public/browser/navigation_details.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_source.h" @@ -59,10 +56,10 @@ void ChromeContentRulesRegistry::Observe( case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: { content::WebContents* tab = content::Source(source).ptr(); - // Note that neither non-tab WebContents nor tabs from other browser - // contexts will be in the map. - active_rules_.erase(tab); - matching_css_selectors_.erase(tab); + // GetTabId() returns -1 for non-tab WebContents, which won't be + // in the map. Similarly, tabs from other browser_contexts won't be in + // the map. + active_rules_.erase(ExtensionTabUtil::GetTabId(tab)); break; } } @@ -71,9 +68,41 @@ void ChromeContentRulesRegistry::Observe( void ChromeContentRulesRegistry::Apply( content::WebContents* contents, const std::vector& matching_css_selectors) { - matching_css_selectors_[contents] = matching_css_selectors; + const int tab_id = ExtensionTabUtil::GetTabId(contents); + RendererContentMatchData renderer_data; + renderer_data.page_url_matches = url_matcher_.MatchURL(contents->GetURL()); + renderer_data.css_selectors.insert(matching_css_selectors.begin(), + matching_css_selectors.end()); + std::set matching_rules = GetMatches(renderer_data); + if (matching_rules.empty() && !ContainsKey(active_rules_, tab_id)) + return; - EvaluateConditionsForTab(contents); + std::set& prev_matching_rules = active_rules_[tab_id]; + ContentAction::ApplyInfo apply_info = {browser_context(), contents}; + for (std::set::const_iterator it = matching_rules.begin(); + it != matching_rules.end(); + ++it) { + apply_info.priority = (*it)->priority(); + if (!ContainsKey(prev_matching_rules, *it)) { + (*it)->actions().Apply((*it)->extension_id(), base::Time(), &apply_info); + } else { + (*it)->actions().Reapply( + (*it)->extension_id(), base::Time(), &apply_info); + } + } + for (std::set::const_iterator it = prev_matching_rules.begin(); + it != prev_matching_rules.end(); + ++it) { + if (!ContainsKey(matching_rules, *it)) { + apply_info.priority = (*it)->priority(); + (*it)->actions().Revert((*it)->extension_id(), base::Time(), &apply_info); + } + } + + if (matching_rules.empty()) + active_rules_.erase(tab_id); + else + swap(matching_rules, prev_matching_rules); } void ChromeContentRulesRegistry::DidNavigateMainFrame( @@ -91,8 +120,8 @@ void ChromeContentRulesRegistry::DidNavigateMainFrame( // document's empty, so no CSS rules match. The renderer will send // an ExtensionHostMsg_OnWatchedPageChange later if any CSS rules // match. - matching_css_selectors_[contents].clear(); - EvaluateConditionsForTab(contents); + std::vector no_css_selectors; + Apply(contents, no_css_selectors); } std::set ChromeContentRulesRegistry::GetMatches( @@ -179,7 +208,6 @@ std::string ChromeContentRulesRegistry::AddRulesImpl( url_matcher_.AddConditionSets(all_new_condition_sets); UpdateConditionCache(); - EvaluateConditionsForAllTabs(); return std::string(); } @@ -212,12 +240,21 @@ std::string ChromeContentRulesRegistry::RemoveRulesImpl( } // Remove the ContentRule from active_rules_. - for (auto& tab_rules_pair : active_rules_) { - if (ContainsKey(tab_rules_pair.second, rule)) { - ContentAction::ApplyInfo apply_info = - {browser_context(), tab_rules_pair.first}; + for (std::map >::iterator it = + active_rules_.begin(); + it != active_rules_.end(); + ++it) { + if (ContainsKey(it->second, rule)) { + content::WebContents* tab; + if (!ExtensionTabUtil::GetTabById( + it->first, browser_context(), true, NULL, NULL, &tab, NULL)) { + LOG(DFATAL) << "Tab id " << it->first + << " still in active_rules_, but tab has been destroyed"; + continue; + } + ContentAction::ApplyInfo apply_info = {browser_context(), tab}; rule->actions().Revert(rule->extension_id(), base::Time(), &apply_info); - tab_rules_pair.second.erase(rule); + it->second.erase(rule); } } @@ -286,57 +323,6 @@ void ChromeContentRulesRegistry::InstructRenderProcess( process->Send(new ExtensionMsg_WatchPages(watched_css_selectors_)); } -void ChromeContentRulesRegistry::EvaluateConditionsForTab( - content::WebContents* tab) { - extensions::RendererContentMatchData renderer_data; - renderer_data.page_url_matches = url_matcher_.MatchURL(tab->GetURL()); - renderer_data.css_selectors.insert(matching_css_selectors_[tab].begin(), - matching_css_selectors_[tab].end()); - std::set matching_rules = GetMatches(renderer_data); - if (matching_rules.empty() && !ContainsKey(active_rules_, tab)) - return; - - std::set& prev_matching_rules = active_rules_[tab]; - ContentAction::ApplyInfo apply_info = {browser_context(), tab}; - for (std::set::const_iterator it = matching_rules.begin(); - it != matching_rules.end(); - ++it) { - apply_info.priority = (*it)->priority(); - if (!ContainsKey(prev_matching_rules, *it)) { - (*it)->actions().Apply((*it)->extension_id(), base::Time(), &apply_info); - } else { - (*it)->actions().Reapply( - (*it)->extension_id(), base::Time(), &apply_info); - } - } - for (std::set::const_iterator it = prev_matching_rules.begin(); - it != prev_matching_rules.end(); - ++it) { - if (!ContainsKey(matching_rules, *it)) { - apply_info.priority = (*it)->priority(); - (*it)->actions().Revert((*it)->extension_id(), base::Time(), &apply_info); - } - } - - if (matching_rules.empty()) - active_rules_.erase(tab); - else - swap(matching_rules, prev_matching_rules); -} - -void ChromeContentRulesRegistry::EvaluateConditionsForAllTabs() { - for (chrome::BrowserIterator it; !it.done(); it.Next()) { - Browser* browser = *it; - if (browser->profile() != Profile::FromBrowserContext(browser_context())) - continue; - - for (int i = 0, tab_count = browser->tab_strip_model()->count(); - i < tab_count; - ++i) - EvaluateConditionsForTab(browser->tab_strip_model()->GetWebContentsAt(i)); - } -} - bool ChromeContentRulesRegistry::IsEmpty() const { return match_id_to_rule_.empty() && content_rules_.empty() && url_matcher_.IsEmpty(); diff --git a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h index 039718a88eaff2..e35c396b007540 100644 --- a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h +++ b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h @@ -118,19 +118,10 @@ class ChromeContentRulesRegistry : public ContentRulesRegistry, // ExtensionMsg_WatchPages. void InstructRenderProcess(content::RenderProcessHost* process); - // Evaluates the conditions for |tab| based on the tab state and matching CSS - // selectors. - void EvaluateConditionsForTab(content::WebContents* tab); - - // Evaluates the conditions for tabs in each browser window. - void EvaluateConditionsForAllTabs(); - typedef std::map URLMatcherIdToRule; - typedef std::map> + typedef std::map > RulesMap; - typedef std::map> - CssSelectors; // Map that tells us which ContentRules may match under the condition that // the URLMatcherConditionSet::ID was returned by the |url_matcher_|. @@ -138,9 +129,9 @@ class ChromeContentRulesRegistry : public ContentRulesRegistry, RulesMap content_rules_; - // Maps a WebContents to the set of rules that match on that WebContents. - // This lets us call Revert as appropriate. - std::map> active_rules_; + // Maps tab_id to the set of rules that match on that tab. This + // lets us call Revert as appropriate. + std::map > active_rules_; // Matches URLs for the page_url condition. url_matcher::URLMatcher url_matcher_; @@ -153,9 +144,6 @@ class ChromeContentRulesRegistry : public ContentRulesRegistry, scoped_refptr extension_info_map_; - // Maps tab_id to the matching CSS selectors for the tab. - CssSelectors matching_css_selectors_; - DISALLOW_COPY_AND_ASSIGN(ChromeContentRulesRegistry); }; diff --git a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry_unittest.cc b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry_unittest.cc index 8b27e8a3a8690c..b468cc60663f77 100644 --- a/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry_unittest.cc +++ b/chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry_unittest.cc @@ -26,8 +26,7 @@ using content::WebContents; // ContentRulesRegistry. class DeclarativeChromeContentRulesRegistryTest : public testing::Test { protected: - using RulesMap = std::map>; - static const RulesMap& active_rules( + static const std::map >& active_rules( const ChromeContentRulesRegistry& registry) { return registry.active_rules_; } diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc index 488cbfec146b56..4c727062ed816e 100644 --- a/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc +++ b/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc @@ -11,6 +11,7 @@ #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_window.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/common/extensions/features/feature_channel.h" #include "content/public/test/browser_test_utils.h" #include "extensions/test/extension_test_message_listener.h" #include "testing/gmock/include/gmock/gmock.h" @@ -38,30 +39,32 @@ const char kBackgroundHelpers[] = "var PageStateMatcher = chrome.declarativeContent.PageStateMatcher;\n" "var ShowPageAction = chrome.declarativeContent.ShowPageAction;\n" "var onPageChanged = chrome.declarativeContent.onPageChanged;\n" - "var reply = window.domAutomationController.send.bind(\n" + "var Reply = window.domAutomationController.send.bind(\n" " window.domAutomationController);\n" "\n" "function setRules(rules, responseString) {\n" " onPageChanged.removeRules(undefined, function() {\n" " onPageChanged.addRules(rules, function() {\n" " if (chrome.runtime.lastError) {\n" - " reply(chrome.runtime.lastError.message);\n" + " Reply(chrome.runtime.lastError.message);\n" " return;\n" " }\n" - " reply(responseString);\n" + " Reply(responseString);\n" " });\n" " });\n" "};\n"; class DeclarativeContentApiTest : public ExtensionApiTest { public: - DeclarativeContentApiTest() {} - - protected: + DeclarativeContentApiTest() + // Set the channel to "trunk" since declarativeContent is restricted + // to trunk. + : current_channel_(chrome::VersionInfo::CHANNEL_UNKNOWN) { + } + ~DeclarativeContentApiTest() override {} + + extensions::ScopedCurrentChannel current_channel_; TestExtensionDir ext_dir_; - - private: - DISALLOW_COPY_AND_ASSIGN(DeclarativeContentApiTest); }; IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) { @@ -119,7 +122,10 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) { "document.body.offsetTop;")); // Give the style match a chance to run and send back the matching-selector - // update. + // update. This takes one time through the Blink message loop to apply the + // style to the new element, and a second to dedupe updates. + // FIXME: Remove this after https://codereview.chromium.org/145663012/ + ASSERT_TRUE(content::ExecuteScript(tab, std::string())); ASSERT_TRUE(content::ExecuteScript(tab, std::string())); EXPECT_TRUE(page_action->GetIsVisible(tab_id)) @@ -131,87 +137,17 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) { tab, "document.body.innerHTML = 'Hello world';" "document.body.offsetTop;")); + // Give the style match a chance to run and send back the matching-selector + // update. This takes one time through the Blink message loop to apply the + // style to the new element, and a second to dedupe updates. + // FIXME: Remove this after https://codereview.chromium.org/145663012/ + ASSERT_TRUE(content::ExecuteScript(tab, std::string())); + ASSERT_TRUE(content::ExecuteScript(tab, std::string())); + EXPECT_FALSE(page_action->GetIsVisible(tab_id)) << "Removing the matching element should hide the page action again."; } -// Tests that the rules are evaluated at the time they are added or removed. -IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, RulesEvaluatedOnAddRemove) { - ext_dir_.WriteManifest(kDeclarativeContentManifest); - ext_dir_.WriteFile( - FILE_PATH_LITERAL("background.js"), - "var PageStateMatcher = chrome.declarativeContent.PageStateMatcher;\n" - "var ShowPageAction = chrome.declarativeContent.ShowPageAction;\n" - "var onPageChanged = chrome.declarativeContent.onPageChanged;\n" - "var reply = window.domAutomationController.send.bind(\n" - " window.domAutomationController);\n" - "\n" - "function addRules(rules, responseString) {\n" - " onPageChanged.addRules(rules, function() {\n" - " if (chrome.runtime.lastError) {\n" - " reply(chrome.runtime.lastError.message);\n" - " return;\n" - " }\n" - " reply(responseString);\n" - " });\n" - "};\n" - "\n" - "function removeRule(id, responseString) {\n" - " onPageChanged.removeRules([id], function() {\n" - " if (chrome.runtime.lastError) {\n" - " reply(chrome.runtime.lastError.message);\n" - " return;\n" - " }\n" - " reply(responseString);\n" - " });\n" - "};\n"); - const Extension* extension = LoadExtension(ext_dir_.unpacked_path()); - ASSERT_TRUE(extension); - const ExtensionAction* page_action = - ExtensionActionManager::Get(browser()->profile())-> - GetPageAction(*extension); - ASSERT_TRUE(page_action); - - content::WebContents* const tab = - browser()->tab_strip_model()->GetWebContentsAt(0); - const int tab_id = ExtensionTabUtil::GetTabId(tab); - - NavigateInRenderer(tab, GURL("http://test1/")); - - const std::string kAddTestRules = - "addRules([{\n" - " id: '1',\n" - " conditions: [new PageStateMatcher({\n" - " pageUrl: {hostPrefix: \"test1\"}})],\n" - " actions: [new ShowPageAction()]\n" - "}, {\n" - " id: '2',\n" - " conditions: [new PageStateMatcher({\n" - " pageUrl: {hostPrefix: \"test2\"}})],\n" - " actions: [new ShowPageAction()]\n" - "}], 'add_rules');\n"; - EXPECT_EQ("add_rules", - ExecuteScriptInBackgroundPage(extension->id(), kAddTestRules)); - - EXPECT_TRUE(page_action->GetIsVisible(tab_id)); - - const std::string kRemoveTestRule1 = "removeRule('1', 'remove_rule1');\n"; - EXPECT_EQ("remove_rule1", - ExecuteScriptInBackgroundPage(extension->id(), kRemoveTestRule1)); - - EXPECT_FALSE(page_action->GetIsVisible(tab_id)); - - NavigateInRenderer(tab, GURL("http://test2/")); - - EXPECT_TRUE(page_action->GetIsVisible(tab_id)); - - const std::string kRemoveTestRule2 = "removeRule('2', 'remove_rule2');\n"; - EXPECT_EQ("remove_rule2", - ExecuteScriptInBackgroundPage(extension->id(), kRemoveTestRule2)); - - EXPECT_FALSE(page_action->GetIsVisible(tab_id)); -} - // http://crbug.com/304373 IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, UninstallWhileActivePageAction) {