Specifying "targetUrlPatterns" should imply link context.

When creating a context menu item from an extension, specifying one or more 
patterns in the "targetUrlPatterns" should imply that your item only appears
on items with a matching link, and not in contexts that have no link (which
does not match, sort of by definition :) ).


BUG=49739
TEST=Follow steps in bug report.

Review URL: http://codereview.chromium.org/3420009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59986 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 77 insertions, 25 deletions

diff --git a/chrome/browser/extensions/extension_context_menu_browsertest.cc b/chrome/browser/extensions/extension_context_menu_browsertest.cc
index b176c57..427a7fc 100644
--- a/chrome/browser/extensions/extension_context_menu_browsertest.cc
+++ b/chrome/browser/extensions/extension_context_menu_browsertest.cc
@@ -122,12 +122,13 @@ class ExtensionContextMenuBrowserTest : public ExtensionBrowserTest {
     return LoadExtension(extension_dir);
   }
 
-  // This creates and returns a test menu for a page with |url|.
-  TestRenderViewContextMenu* CreateMenuForURL(const GURL& url) {
+  TestRenderViewContextMenu* CreateMenu(const GURL& page_url,
+                                        const GURL& link_url) {
     TabContents* tab_contents = browser()->GetSelectedTabContents();
     WebContextMenuData data;
     ContextMenuParams params(data);
-    params.page_url = url;
+    params.page_url = page_url;
+    params.link_url = link_url;
     TestRenderViewContextMenu* menu =
         new TestRenderViewContextMenu(tab_contents, params);
     menu->Init();
@@ -166,10 +167,13 @@ class ExtensionContextMenuBrowserTest : public ExtensionBrowserTest {
     return result;
   }
 
-  // This creates a test menu for a page with |url|, looks for an extension item
-  // with the given |label|, and returns true if the item was found.
-  bool MenuHasItemWithLabel(const GURL& url, const std::string& label) {
-    scoped_ptr<TestRenderViewContextMenu> menu(CreateMenuForURL(url));
+  // This creates a test menu for a page with |page_url| and |link_url|, looks
+  // for an extension item with the given |label|, and returns true if the item
+  // was found.
+  bool MenuHasItemWithLabel(const GURL& page_url,
+                            const GURL& link_url,
+                            const std::string& label) {
+    scoped_ptr<TestRenderViewContextMenu> menu(CreateMenu(page_url, link_url));
     return menu->HasExtensionItemWithLabel(label);
   }
 };
@@ -186,7 +190,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Simple) {
   GURL page_url("http://www.google.com");
 
   // Create and build our test context menu.
-  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenuForURL(page_url));
+  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenu(page_url, GURL()));
 
   // Look for the extension item in the menu, and execute it.
   int command_id = IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST;
@@ -209,13 +213,21 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Patterns) {
 
   // Check that a document url that should match the items' patterns appears.
   GURL google_url("http://www.google.com");
-  ASSERT_TRUE(MenuHasItemWithLabel(google_url, std::string("test_item1")));
-  ASSERT_TRUE(MenuHasItemWithLabel(google_url, std::string("test_item2")));
+  ASSERT_TRUE(MenuHasItemWithLabel(google_url,
+                                   GURL(),
+                                   std::string("test_item1")));
+  ASSERT_TRUE(MenuHasItemWithLabel(google_url,
+                                   GURL(),
+                                   std::string("test_item2")));
 
   // Now check with a non-matching url.
   GURL test_url("http://www.test.com");
-  ASSERT_FALSE(MenuHasItemWithLabel(test_url, std::string("test_item1")));
-  ASSERT_FALSE(MenuHasItemWithLabel(test_url, std::string("test_item2")));
+  ASSERT_FALSE(MenuHasItemWithLabel(test_url,
+                                    GURL(),
+                                    std::string("test_item1")));
+  ASSERT_FALSE(MenuHasItemWithLabel(test_url,
+                                    GURL(),
+                                    std::string("test_item2")));
 }
 
 // Tests registering an item with a very long title that should get truncated in
@@ -237,7 +249,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, LongTitle) {
   // Create a context menu, then find the item's label. It should be properly
   // truncated.
   GURL url("http://foo.com/");
-  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenuForURL(url));
+  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenu(url, GURL()));
 
   string16 label;
   ASSERT_TRUE(menu->GetItemLabel(item->id(), &label));
@@ -301,7 +313,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Separators) {
   listener1.WaitUntilSatisfied();
 
   GURL url("http://www.google.com/");
-  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenuForURL(url));
+  scoped_ptr<TestRenderViewContextMenu> menu(CreateMenu(url, GURL()));
 
   // The top-level item should be an "automagic parent" with the extension's
   // name.
@@ -324,7 +336,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Separators) {
   ui_test_utils::NavigateToURL(browser(),
                                GURL(extension->GetResourceURL("test2.html")));
   listener2.WaitUntilSatisfied();
-  menu.reset(CreateMenuForURL(url));
+  menu.reset(CreateMenu(url, GURL()));
   ASSERT_TRUE(menu->GetMenuModelAndItemIndex(
       IDC_EXTENSIONS_CONTEXT_CUSTOM_FIRST, &model, &index));
   EXPECT_EQ(UTF8ToUTF16("parent"), model->GetLabelAt(index));
@@ -332,3 +344,27 @@ IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, Separators) {
   ASSERT_TRUE(submenu != NULL);
   VerifyMenuForSeparatorsTest(*submenu);
 }
+
+// Tests that targetUrlPattern keeps items from appearing when there is no
+// target url.
+IN_PROC_BROWSER_TEST_F(ExtensionContextMenuBrowserTest, TargetURLs) {
+  ExtensionTestMessageListener listener("created items");
+  ASSERT_TRUE(LoadContextMenuExtension("target_urls"));
+  ASSERT_TRUE(listener.WaitUntilSatisfied());
+
+  GURL google_url("http://www.google.com");
+  GURL non_google_url("http://www.foo.com");
+
+  // No target url - the item should not appear.
+  ASSERT_FALSE(MenuHasItemWithLabel(google_url, GURL(), std::string("item1")));
+
+  // A matching target url - the item should appear.
+  ASSERT_TRUE(MenuHasItemWithLabel(google_url,
+                                   google_url,
+                                   std::string("item1")));
+
+  // A non-matching target url - the item should not appear.
+  ASSERT_FALSE(MenuHasItemWithLabel(google_url,
+                                    non_google_url,
+                                    std::string("item1")));
+}
diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc
index b79996e..707ca68 100644
--- a/chrome/browser/tab_contents/render_view_context_menu.cc
+++ b/chrome/browser/tab_contents/render_view_context_menu.cc
@@ -166,8 +166,7 @@ static ExtensionMenuItem::List GetRelevantExtensionItems(
 
     const GURL& target_url =
         params.src_url.is_empty() ? params.link_url : params.src_url;
-    if (!target_url.is_empty() &&
-        !ExtensionPatternMatch(item->target_url_patterns(), target_url))
+    if (!ExtensionPatternMatch(item->target_url_patterns(), target_url))
       continue;
 
     result.push_back(*i);
diff --git a/chrome/test/data/extensions/context_menus/patterns/test.js b/chrome/test/data/extensions/context_menus/patterns/test.js
index 4777a86..ef9bd45 100644
--- a/chrome/test/data/extensions/context_menus/patterns/test.js
+++ b/chrome/test/data/extensions/context_menus/patterns/test.js
@@ -11,23 +11,20 @@ var make_browsertest_proceed = function() {
 var patterns = ["http://*.google.com/*", "https://*.google.com/*"];
 
 window.onload = function() {
-  // Create one item that does have a documentUrlPattern and targetUrlPattern.
+  // Create one item that does have a documentUrlPattern.
   var properties1 = {
-    "title": "test_item1", "documentUrlPatterns": patterns,
-    "targetUrlPatterns": patterns
+    "title": "test_item1", "documentUrlPatterns": patterns
   };
   chrome.contextMenus.create(properties1);
 
-  // Create an item that initially doesn't have a documentUrlPattern and
-  // targetUrlPattern, then update it, and trigger the rest of the c++ code in
-  // the browser test by navigating the tab.
+  // Create an item that initially doesn't have a documentUrlPattern, then
+  // update it, and then proceed with the c++ code in the browser test.
   var properties2 = { "title": "test_item2" };
 
   var id2;
   id2 = chrome.contextMenus.create(properties2,
                                                 function() {
-    var update_properties = { "documentUrlPatterns": patterns,
-                              "targetUrlPatterns": patterns };
+    var update_properties = { "documentUrlPatterns": patterns };
     chrome.contextMenus.update(id2, update_properties,
                                make_browsertest_proceed);
   });
diff --git a/chrome/test/data/extensions/context_menus/target_urls/background.html b/chrome/test/data/extensions/context_menus/target_urls/background.html
new file mode 100644
index 0000000..46f4d74
--- /dev/null
+++ b/chrome/test/data/extensions/context_menus/target_urls/background.html
@@ -0,0 +1 @@
+<script src="test.js"></script>
diff --git a/chrome/test/data/extensions/context_menus/target_urls/manifest.json b/chrome/test/data/extensions/context_menus/target_urls/manifest.json
new file mode 100644
index 0000000..8061f29
--- /dev/null
+++ b/chrome/test/data/extensions/context_menus/target_urls/manifest.json
@@ -0,0 +1,6 @@
+{
+  "name" : "targetUrlPatterns test",
+  "version" : "0.1",
+  "permissions": [ "contextMenus", "tabs" ],
+  "background_page": "background.html"
+}
diff --git a/chrome/test/data/extensions/context_menus/target_urls/test.js b/chrome/test/data/extensions/context_menus/target_urls/test.js
new file mode 100644
index 0000000..e32099f
--- /dev/null
+++ b/chrome/test/data/extensions/context_menus/target_urls/test.js
@@ -0,0 +1,13 @@
+// Copyright (c) 2010 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.
+
+window.onload = function() {
+  var patterns = [ "http://*.google.com/*" ];
+  chrome.contextMenus.create({"title":"item1", "contexts": ["all"],
+                              "targetUrlPatterns": patterns}, function() {
+    if (!chrome.extension.lastError) {
+      chrome.test.sendMessage("created items");
+    }
+  });
+};