Bug 1287007 - Fix "onclick" in contextMenus, to child. r=billm

Main thing: Making contextMenus implementation webext-oop compatible.

Preparation:

- Add getParentEvent to ChildAPIManager to allow use of remote events.
- Introduce `addon_parent_only` to "allowedContexts" to only generate a
  schema API in the main process.
- Do not fill in `null` for missing keys if the schema declares a key as
  `"optional": "omit-key-if-missing"`. This is needed for the second
  point in the next list.

Drive-by fixes:

- Ensure that the "onclick" handler is erased when a context closes.
- Do not clear the "onclick" handler in `contextMenus.update` if the
  onclick key has been omitted (parity with Chrome).
- Remove some unnecessary `Promise.resolve()`
- Add extensive set of tests that check the behavior of the contextMenus
  APIs with regards to the onclick attribute in various scenarios.

MozReview-Commit-ID: A5f3AUQzU8T

--HG--
extra : rebase_source : 0464a1aa2387343a6f1d0fcd8fbabfdd1a68b1bb
This commit is contained in:
Rob Wu 2016-09-12 18:26:03 -07:00
Родитель fcb710d311
Коммит 0c66504d77
11 изменённых файлов: 477 добавлений и 19 удалений

Просмотреть файл

@ -0,0 +1,158 @@
/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set sts=2 sw=2 et tw=80: */
"use strict";
// If id is not specified for an item we use an integer.
// This ID need only be unique within a single addon. Since all addon code that
// can use this API runs in the same process, this local variable suffices.
var gNextMenuItemID = 0;
// Map[Extension -> Map[string or id, ContextMenusClickPropHandler]]
var gPropHandlers = new Map();
// The contextMenus API supports an "onclick" attribute in the create/update
// methods to register a callback. This class manages these onclick properties.
class ContextMenusClickPropHandler {
constructor(context) {
this.context = context;
// Map[string or integer -> callback]
this.onclickMap = new Map();
this.dispatchEvent = this.dispatchEvent.bind(this);
}
// A listener on contextMenus.onClicked that forwards the event to the only
// listener, if any.
dispatchEvent(info, tab) {
let onclick = this.onclickMap.get(info.menuItemId);
if (onclick) {
// No need for runSafe or anything because we are already being run inside
// an event handler -- the event is just being forwarded to the actual
// handler.
onclick(info, tab);
}
}
// Sets the `onclick` handler for the given menu item.
// The `onclick` function MUST be owned by `this.context`.
setListener(id, onclick) {
if (this.onclickMap.size === 0) {
this.context.childManager.getParentEvent("contextMenus.onClicked").addListener(this.dispatchEvent);
this.context.callOnClose(this);
}
this.onclickMap.set(id, onclick);
let propHandlerMap = gPropHandlers.get(this.context.extension);
if (!propHandlerMap) {
propHandlerMap = new Map();
} else {
// If the current callback was created in a different context, remove it
// from the other context.
let propHandler = propHandlerMap.get(id);
if (propHandler && propHandler !== this) {
propHandler.unsetListener(id);
}
}
propHandlerMap.set(id, this);
gPropHandlers.set(this.context.extension, propHandlerMap);
}
// Deletes the `onclick` handler for the given menu item.
// The `onclick` function MUST be owned by `this.context`.
unsetListener(id) {
if (!this.onclickMap.delete(id)) {
return;
}
if (this.onclickMap.size === 0) {
this.context.childManager.getParentEvent("contextMenus.onClicked").removeListener(this.dispatchEvent);
this.context.forgetOnClose(this);
}
let propHandlerMap = gPropHandlers.get(this.context.extension);
propHandlerMap.delete(id);
if (propHandlerMap.size === 0) {
gPropHandlers.delete(this.context.extension);
}
}
// Deletes the `onclick` handler for the given menu item, if any, regardless
// of the context where it was created.
unsetListenerFromAnyContext(id) {
let propHandlerMap = gPropHandlers.get(this.context.extension);
let propHandler = propHandlerMap && propHandlerMap.get(id);
if (propHandler) {
propHandler.unsetListener(id);
}
}
// Remove all `onclick` handlers of the extension.
deleteAllListenersFromExtension() {
let propHandlerMap = gPropHandlers.get(this.context.extension);
if (propHandlerMap) {
for (let [id, propHandler] of propHandlerMap) {
propHandler.unsetListener(id);
}
}
}
// Removes all `onclick` handlers from this context.
close() {
for (let id of this.onclickMap.keys()) {
this.unsetListener(id);
}
}
}
extensions.registerSchemaAPI("contextMenus", "addon_child", context => {
let onClickedProp = new ContextMenusClickPropHandler(context);
return {
contextMenus: {
create(createProperties, callback) {
if (createProperties.id === null) {
createProperties.id = ++gNextMenuItemID;
}
let {onclick} = createProperties;
delete createProperties.onclick;
context.childManager.callParentAsyncFunction("contextMenus.createInternal", [
createProperties,
]).then(() => {
if (onclick) {
onClickedProp.setListener(createProperties.id, onclick);
}
if (callback) {
callback();
}
});
return createProperties.id;
},
update(id, updateProperties) {
let {onclick} = updateProperties;
delete updateProperties.onclick;
return context.childManager.callParentAsyncFunction("contextMenus.update", [
id,
updateProperties,
]).then(() => {
if (onclick) {
onClickedProp.setListener(id, onclick);
} else if (onclick === null) {
onClickedProp.unsetListenerFromAnyContext(id);
}
// else onclick is not set so it should not be changed.
});
},
remove(id) {
onClickedProp.unsetListenerFromAnyContext(id);
return context.childManager.callParentAsyncFunction("contextMenus.remove", [
id,
]);
},
removeAll() {
onClickedProp.deleteAllListenersFromExtension();
return context.childManager.callParentAsyncFunction("contextMenus.removeAll", []);
},
},
};
});

Просмотреть файл

@ -10,7 +10,6 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm");
var {
EventManager,
IconDetails,
runSafe,
} = ExtensionUtils;
// Map[Extension -> Map[ID -> MenuItem]]
@ -193,9 +192,6 @@ var gMenuBuilder = {
let tab = item.tabManager.convert(contextData.tab);
let info = item.getClickInfo(contextData, wasChecked);
item.extension.emit("webext-contextmenu-menuitem-click", info, tab);
if (item.onclick) {
runSafe(item.extContext, item.onclick, info, tab);
}
});
return element;
@ -259,9 +255,8 @@ function getContexts(contextData) {
return contexts;
}
function MenuItem(extension, extContext, createProperties, isRoot = false) {
function MenuItem(extension, createProperties, isRoot = false) {
this.extension = extension;
this.extContext = extContext;
this.children = [];
this.parent = null;
this.tabManager = TabManager.for(extension);
@ -375,7 +370,7 @@ MenuItem.prototype = {
get root() {
let extension = this.extension;
if (!gRootItems.has(extension)) {
let root = new MenuItem(extension, this.context,
let root = new MenuItem(extension,
{title: extension.name},
/* isRoot = */ true);
gRootItems.set(extension, root);
@ -495,13 +490,12 @@ extensions.registerSchemaAPI("contextMenus", "addon_parent", context => {
let {extension} = context;
return {
contextMenus: {
create: function(createProperties, callback) {
let menuItem = new MenuItem(extension, context, createProperties);
createInternal: function(createProperties) {
// Note that the id is required by the schema. If the addon did not set
// it, the implementation of contextMenus.create in the child should
// have added it.
let menuItem = new MenuItem(extension, createProperties);
gContextMenuMap.get(extension).set(menuItem.id, menuItem);
if (callback) {
runSafe(context, callback);
}
return menuItem.id;
},
update: function(id, updateProperties) {
@ -509,7 +503,6 @@ extensions.registerSchemaAPI("contextMenus", "addon_parent", context => {
if (menuItem) {
menuItem.setProps(updateProperties);
}
return Promise.resolve();
},
remove: function(id) {
@ -517,7 +510,6 @@ extensions.registerSchemaAPI("contextMenus", "addon_parent", context => {
if (menuItem) {
menuItem.remove();
}
return Promise.resolve();
},
removeAll: function() {
@ -525,7 +517,6 @@ extensions.registerSchemaAPI("contextMenus", "addon_parent", context => {
if (root) {
root.remove();
}
return Promise.resolve();
},
onClicked: new EventManager(context, "contextMenus.onClicked", fire => {

Просмотреть файл

@ -12,6 +12,7 @@ category webextension-scripts windows chrome://browser/content/ext-windows.js
# scripts that must run in the same process as addon code.
category webextension-scripts-addon browserAction chrome://browser/content/ext-c-browserAction.js
category webextension-scripts-addon contextMenus chrome://browser/content/ext-c-contextMenus.js
category webextension-scripts-addon pageAction chrome://browser/content/ext-c-pageAction.js
category webextension-scripts-addon tabs chrome://browser/content/ext-c-tabs.js

Просмотреть файл

@ -23,5 +23,6 @@ browser.jar:
content/browser/ext-utils.js
content/browser/ext-windows.js
content/browser/ext-c-browserAction.js
content/browser/ext-c-contextMenus.js
content/browser/ext-c-pageAction.js
content/browser/ext-c-tabs.js

Просмотреть файл

@ -206,6 +206,74 @@
}
]
},
{
"name": "createInternal",
"type": "function",
"allowedContexts": ["addon_parent_only"],
"async": "callback",
"description": "Identical to contextMenus.create, except: the 'id' field is required and allows an integer, 'onclick' is not allowed, and the method is async (and the return value is not a menu item ID).",
"parameters": [
{
"type": "object",
"name": "createProperties",
"properties": {
"type": {
"$ref": "ItemType",
"optional": true
},
"id": {
"choices": [
{ "type": "integer" },
{ "type": "string" }
]
},
"title": {
"type": "string",
"optional": true
},
"checked": {
"type": "boolean",
"optional": true
},
"contexts": {
"type": "array",
"items": {
"$ref": "ContextType"
},
"minItems": 1,
"optional": true
},
"parentId": {
"choices": [
{ "type": "integer" },
{ "type": "string" }
],
"optional": true
},
"documentUrlPatterns": {
"type": "array",
"items": {"type": "string"},
"optional": true
},
"targetUrlPatterns": {
"type": "array",
"items": {"type": "string"},
"optional": true
},
"enabled": {
"type": "boolean",
"optional": true
}
}
},
{
"type": "function",
"name": "callback",
"optional": true,
"parameters": []
}
]
},
{
"name": "update",
"type": "function",
@ -247,7 +315,7 @@
},
"onclick": {
"type": "function",
"optional": true,
"optional": "omit-key-if-missing",
"parameters": [
{
"name": "info",

Просмотреть файл

@ -35,6 +35,7 @@ tags = webextensions
[browser_ext_contextMenus.js]
[browser_ext_contextMenus_checkboxes.js]
[browser_ext_contextMenus_icons.js]
[browser_ext_contextMenus_onclick.js]
[browser_ext_contextMenus_radioGroups.js]
[browser_ext_contextMenus_uninstall.js]
[browser_ext_contextMenus_urlPatterns.js]

Просмотреть файл

@ -34,6 +34,9 @@ add_task(function* () {
title: "child",
});
browser.test.onMessage.addListener(() => {
browser.test.sendMessage("pong");
});
browser.test.notifyPass("contextmenus-icons");
},
});
@ -55,6 +58,10 @@ add_task(function* () {
let childToDelete = extensionMenu.getElementsByAttribute("label", "child-to-delete")[0];
yield closeExtensionContextMenu(childToDelete);
// Now perform a roundtrip to the extension process to make sure that the
// click event has had a chance to fire.
extension.sendMessage("ping");
yield extension.awaitMessage("pong");
yield openExtensionContextMenu();

Просмотреть файл

@ -0,0 +1,196 @@
/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set sts=2 sw=2 et tw=80: */
"use strict";
// Loaded both as a background script and a tab page.
function testScript() {
let page = location.pathname.includes("tab.html") ? "tab" : "background";
let clickCounts = {
old: 0,
new: 0,
};
browser.contextMenus.onClicked.addListener(() => {
// Async to give other onclick handlers a chance to fire.
setTimeout(() => {
browser.test.sendMessage("onClicked-fired", page);
});
});
browser.test.onMessage.addListener((toPage, msg) => {
if (toPage !== page) {
return;
}
browser.test.log(`Received ${msg} for ${toPage}`);
if (msg == "get-click-counts") {
browser.test.sendMessage("click-counts", clickCounts);
} else if (msg == "clear-click-counts") {
clickCounts.old = clickCounts.new = 0;
browser.test.sendMessage("next");
} else if (msg == "create-with-onclick") {
browser.contextMenus.create({
id: "iden",
title: "tifier",
onclick() {
++clickCounts.old;
browser.test.log(`onclick fired for original onclick property in ${page}`);
},
}, () => browser.test.sendMessage("next"));
} else if (msg == "create-without-onclick") {
browser.contextMenus.create({
id: "iden",
title: "tifier",
}, () => browser.test.sendMessage("next"));
} else if (msg == "update-without-onclick") {
browser.contextMenus.update("iden", {
enabled: true, // Already enabled, so this does nothing.
}, () => browser.test.sendMessage("next"));
} else if (msg == "update-with-onclick") {
browser.contextMenus.update("iden", {
onclick() {
++clickCounts.new;
browser.test.log(`onclick fired for updated onclick property in ${page}`);
},
}, () => browser.test.sendMessage("next"));
} else if (msg == "remove") {
browser.contextMenus.remove("iden", () => browser.test.sendMessage("next"));
} else if (msg == "removeAll") {
browser.contextMenus.removeAll(() => browser.test.sendMessage("next"));
}
});
if (page == "background") {
browser.test.log("Opening tab.html");
browser.tabs.create({
url: "tab.html",
active: false, // To not interfere with the context menu tests.
});
} else {
// Sanity check - the pages must be in the same process.
let pages = browser.extension.getViews();
browser.test.assertTrue(pages.includes(window),
"Expected this tab to be an extension view");
pages = pages.filter(w => w !== window);
browser.test.assertEq(pages[0], browser.extension.getBackgroundPage(),
"Expected the other page to be a background page");
browser.test.sendMessage("tab.html ready");
}
}
add_task(function* () {
let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
"http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
gBrowser.selectedTab = tab1;
let extension = ExtensionTestUtils.loadExtension({
manifest: {
"permissions": ["contextMenus"],
},
background: testScript,
files: {
"tab.html": `<!DOCTYPE html><meta charset="utf-8"><script src="tab.js"></script>`,
"tab.js": testScript,
},
});
yield extension.startup();
yield extension.awaitMessage("tab.html ready");
function* clickContextMenu() {
// Using openContextMenu instead of openExtensionContextMenu because the
// test extension has only one context menu item.
let extensionMenuRoot = yield openContextMenu();
let items = extensionMenuRoot.getElementsByAttribute("label", "tifier");
is(items.length, 1, "Expected one context menu item");
yield closeExtensionContextMenu(items[0]);
// One of them is "tab", the other is "background".
info(`onClicked from: ${yield extension.awaitMessage("onClicked-fired")}`);
info(`onClicked from: ${yield extension.awaitMessage("onClicked-fired")}`);
}
function* getCounts(page) {
extension.sendMessage(page, "get-click-counts");
return yield extension.awaitMessage("click-counts");
}
function* resetCounts() {
extension.sendMessage("tab", "clear-click-counts");
extension.sendMessage("background", "clear-click-counts");
yield extension.awaitMessage("next");
yield extension.awaitMessage("next");
}
// During this test, at most one "onclick" attribute is expected at any time.
for (let pageOne of ["background", "tab"]) {
for (let pageTwo of ["background", "tab"]) {
info(`Testing with menu created by ${pageOne} and updated by ${pageTwo}`);
extension.sendMessage(pageOne, "create-with-onclick");
yield extension.awaitMessage("next");
// Test that update without onclick attribute does not clear the existing
// onclick handler.
extension.sendMessage(pageTwo, "update-without-onclick");
yield extension.awaitMessage("next");
yield clickContextMenu();
let clickCounts = yield getCounts(pageOne);
is(clickCounts.old, 1, `Original onclick should still be present in ${pageOne}`);
is(clickCounts.new, 0, `Not expecting any new handlers in ${pageOne}`);
if (pageOne !== pageTwo) {
clickCounts = yield getCounts(pageTwo);
is(clickCounts.old, 0, `Not expecting any handlers in ${pageTwo}`);
is(clickCounts.new, 0, `Not expecting any new handlers in ${pageTwo}`);
}
yield resetCounts();
// Test that update with onclick handler in a different page clears the
// existing handler and activates the new onclick handler.
extension.sendMessage(pageTwo, "update-with-onclick");
yield extension.awaitMessage("next");
yield clickContextMenu();
clickCounts = yield getCounts(pageOne);
is(clickCounts.old, 0, `Original onclick should be gone from ${pageOne}`);
if (pageOne !== pageTwo) {
is(clickCounts.new, 0, `Still not expecting new handlers in ${pageOne}`);
}
clickCounts = yield getCounts(pageTwo);
if (pageOne !== pageTwo) {
is(clickCounts.old, 0, `Not expecting an old onclick in ${pageTwo}`);
}
is(clickCounts.new, 1, `New onclick should be triggered in ${pageTwo}`);
yield resetCounts();
// Test that updating the handler (different again from the last `update`
// call, but the same as the `create` call) clears the existing handler
// and activates the new onclick handler.
extension.sendMessage(pageOne, "update-with-onclick");
yield extension.awaitMessage("next");
yield clickContextMenu();
clickCounts = yield getCounts(pageOne);
is(clickCounts.new, 1, `onclick should be triggered in ${pageOne}`);
if (pageOne !== pageTwo) {
clickCounts = yield getCounts(pageTwo);
is(clickCounts.new, 0, `onclick should be gone from ${pageTwo}`);
}
yield resetCounts();
// Test that removing the context menu and recreating it with the same ID
// (in a different context) does not leave behind any onclick handlers.
extension.sendMessage(pageTwo, "remove");
yield extension.awaitMessage("next");
extension.sendMessage(pageTwo, "create-without-onclick");
yield extension.awaitMessage("next");
yield clickContextMenu();
clickCounts = yield getCounts(pageOne);
is(clickCounts.new, 0, `Did not expect any click handlers in ${pageOne}`);
if (pageOne !== pageTwo) {
clickCounts = yield getCounts(pageTwo);
is(clickCounts.new, 0, `Did not expect any click handlers in ${pageTwo}`);
}
yield resetCounts();
// Remove context menu for the next iteration of the test. And just to get
// more coverage, let's use removeAll instead of remove.
extension.sendMessage(pageOne, "removeAll");
yield extension.awaitMessage("next");
}
}
yield extension.unload();
yield BrowserTestUtils.removeTab(tab1);
});

Просмотреть файл

@ -740,6 +740,10 @@ GlobalManager = {
if (context.envType === "content_parent" && !allowedContexts.includes("content")) {
return false;
}
if (context.envType !== "addon_parent" &&
allowedContexts.includes("addon_parent_only")) {
return false;
}
return findPathInObject(apis, namespace, false) !== null;
},

Просмотреть файл

@ -1856,6 +1856,30 @@ class ChildAPIManager {
return this.context.wrapPromise(deferred.promise, callback);
}
/**
* Create a proxy for an event in the parent process. The returned event
* object shares its internal state with other instances. For instance, if
* `removeListener` is used on a listener that was added on another object
* through `addListener`, then the event is unregistered.
*
* @param {string} path The full name of the event, e.g. "tabs.onCreated".
* @returns {object} An object with the addListener, removeListener and
* hasListener methods. See SchemaAPIInterface for documentation.
*/
getParentEvent(path) {
let parsed = /^(.+)\.(on[A-Z][^.]+)$/.exec(path);
if (!parsed) {
throw new Error("getParentEvent: Invalid event name: " + path);
}
let [, namespace, name] = parsed;
let impl = new ProxyAPIImplementation(namespace, name, this);
return {
addListener: (listener, ...args) => impl.addListener(listener, args),
removeListener: (listener) => impl.removeListener(listener),
hasListener: (listener) => impl.hasListener(listener),
};
}
close() {
this.messageManager.sendAsyncMessage("API:CloseProxyContext", {childId: this.id});
}
@ -1870,7 +1894,14 @@ class ChildAPIManager {
shouldInject(namespace, name, allowedContexts) {
// Do not generate content script APIs, unless explicitly allowed.
return this.context.envType !== "content_child" || allowedContexts.includes("content");
if (this.context.envType === "content_child" &&
!allowedContexts.includes("content")) {
return false;
}
if (allowedContexts.includes("addon_parent_only")) {
return false;
}
return true;
}
getImplementation(namespace, name) {

Просмотреть файл

@ -1035,7 +1035,7 @@ class ObjectType extends Type {
} else if (!optional) {
error = context.error(`Property "${prop}" is required`,
`contain the required "${prop}" property`);
} else {
} else if (optional !== "omit-key-if-missing") {
result[prop] = null;
}