From 1dc0b2885dea98321d9d75fad7466ef7ea3153ba Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Wed, 21 Nov 2018 06:51:56 +0000 Subject: [PATCH 01/21] Bug 1506056 - Rename runtimeDetails.client to clientWrapper;r=daisuke Note that I fixed the worker bug at the same time. We can keep your other bug to add a test. Differential Revision: https://phabricator.services.mozilla.com/D12007 --HG-- extra : moz-landing-system : lando --- .../src/actions/debug-targets.js | 32 +++++++++-------- .../src/actions/runtimes.js | 31 ++++++++++------- .../src/middleware/debug-target-listener.js | 34 ++++++++++--------- .../src/modules/runtime-client-factory.js | 4 +-- .../src/modules/runtimes-state-helper.js | 2 +- .../client/aboutdebugging-new/src/types.js | 2 +- .../test/browser/mocks/head-usb-mocks.js | 2 +- 7 files changed, 58 insertions(+), 49 deletions(-) diff --git a/devtools/client/aboutdebugging-new/src/actions/debug-targets.js b/devtools/client/aboutdebugging-new/src/actions/debug-targets.js index 30e483f879be..a8c1bd9e4de3 100644 --- a/devtools/client/aboutdebugging-new/src/actions/debug-targets.js +++ b/devtools/client/aboutdebugging-new/src/actions/debug-targets.js @@ -54,9 +54,7 @@ function inspectDebugTarget({ type, id, front }) { } case DEBUG_TARGETS.EXTENSION: { if (runtimeType === RUNTIMES.NETWORK || runtimeType === RUNTIMES.USB) { - // runtimeDetails.client is a ClientWrapper instance, here we need to go back - // to the actual DevTools client. Confusion should be reduce after Bug 1506056. - const devtoolsClient = runtimeDetails.client.client; + const devtoolsClient = runtimeDetails.clientWrapper.client; await debugRemoteAddon(id, devtoolsClient); } else if (runtimeType === RUNTIMES.THIS_FIREFOX) { debugLocalAddon(id); @@ -91,10 +89,10 @@ function installTemporaryExtension() { function pushServiceWorker(actor) { return async (_, getState) => { - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - await client.request({ to: actor, type: "push" }); + await clientWrapper.request({ to: actor, type: "push" }); } catch (e) { console.error(e); } @@ -103,10 +101,10 @@ function pushServiceWorker(actor) { function reloadTemporaryExtension(actor) { return async (_, getState) => { - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - await client.request({ to: actor, type: "reload" }); + await clientWrapper.request({ to: actor, type: "reload" }); } catch (e) { console.error(e); } @@ -127,10 +125,10 @@ function requestTabs() { return async (dispatch, getState) => { dispatch({ type: REQUEST_TABS_START }); - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - const { tabs } = await client.listTabs({ favicons: true }); + const { tabs } = await clientWrapper.listTabs({ favicons: true }); dispatch({ type: REQUEST_TABS_SUCCESS, tabs }); } catch (e) { @@ -144,10 +142,10 @@ function requestExtensions() { dispatch({ type: REQUEST_EXTENSIONS_START }); const runtime = getCurrentRuntime(getState().runtimes); - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - const { addons } = await client.listAddons(); + const { addons } = await clientWrapper.listAddons(); const extensions = addons.filter(a => a.debuggable); if (runtime.type !== RUNTIMES.THIS_FIREFOX) { // manifestURL can only be used when debugging local addons, remove this @@ -174,10 +172,14 @@ function requestWorkers() { return async (dispatch, getState) => { dispatch({ type: REQUEST_WORKERS_START }); - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - const { otherWorkers, serviceWorkers, sharedWorkers } = await client.listWorkers(); + const { + otherWorkers, + serviceWorkers, + sharedWorkers, + } = await clientWrapper.listWorkers(); dispatch({ type: REQUEST_WORKERS_SUCCESS, @@ -193,10 +195,10 @@ function requestWorkers() { function startServiceWorker(actor) { return async (_, getState) => { - const client = getCurrentClient(getState().runtimes); + const clientWrapper = getCurrentClient(getState().runtimes); try { - await client.request({ to: actor, type: "start" }); + await clientWrapper.request({ to: actor, type: "start" }); } catch (e) { console.error(e); } diff --git a/devtools/client/aboutdebugging-new/src/actions/runtimes.js b/devtools/client/aboutdebugging-new/src/actions/runtimes.js index bbc88e439622..52312c195a81 100644 --- a/devtools/client/aboutdebugging-new/src/actions/runtimes.js +++ b/devtools/client/aboutdebugging-new/src/actions/runtimes.js @@ -38,10 +38,10 @@ const { WATCH_RUNTIME_SUCCESS, } = require("../constants"); -async function getRuntimeInfo(runtime, client) { +async function getRuntimeInfo(runtime, clientWrapper) { const { type } = runtime; const { brandName: name, channel, deviceName, version } = - await client.getDeviceDescription(); + await clientWrapper.getDeviceDescription(); const icon = (channel === "release" || channel === "beta" || channel === "aurora") ? `chrome://devtools/skin/images/aboutdebugging-firefox-${ channel }.svg` @@ -68,17 +68,22 @@ function connectRuntime(id) { dispatch({ type: CONNECT_RUNTIME_START }); try { const runtime = findRuntimeById(id, getState().runtimes); - const { client, transportDetails } = await createClientForRuntime(runtime); - const info = await getRuntimeInfo(runtime, client); + const { clientWrapper, transportDetails } = await createClientForRuntime(runtime); + const info = await getRuntimeInfo(runtime, clientWrapper); const promptPrefName = RUNTIME_PREFERENCE.CONNECTION_PROMPT; - const connectionPromptEnabled = await client.getPreference(promptPrefName); - const runtimeDetails = { connectionPromptEnabled, client, info, transportDetails }; + const connectionPromptEnabled = await clientWrapper.getPreference(promptPrefName); + const runtimeDetails = { + clientWrapper, + connectionPromptEnabled, + info, + transportDetails, + }; if (runtime.type === RUNTIMES.USB) { // `closed` event will be emitted when disabling remote debugging // on the connected USB runtime. - client.addOneTimeListener("closed", onUSBDebuggerClientClosed); + clientWrapper.addOneTimeListener("closed", onUSBDebuggerClientClosed); } dispatch({ @@ -100,13 +105,13 @@ function disconnectRuntime(id) { dispatch({ type: DISCONNECT_RUNTIME_START }); try { const runtime = findRuntimeById(id, getState().runtimes); - const client = runtime.runtimeDetails.client; + const { clientWrapper } = runtime.runtimeDetails; if (runtime.type === RUNTIMES.USB) { - client.removeListener("closed", onUSBDebuggerClientClosed); + clientWrapper.removeListener("closed", onUSBDebuggerClientClosed); } - await client.close(); + await clientWrapper.close(); if (runtime.type === RUNTIMES.THIS_FIREFOX) { DebuggerServer.destroy(); @@ -130,11 +135,11 @@ function updateConnectionPromptSetting(connectionPromptEnabled) { dispatch({ type: UPDATE_CONNECTION_PROMPT_SETTING_START }); try { const runtime = getCurrentRuntime(getState().runtimes); - const client = runtime.runtimeDetails.client; + const { clientWrapper } = runtime.runtimeDetails; const promptPrefName = RUNTIME_PREFERENCE.CONNECTION_PROMPT; - await client.setPreference(promptPrefName, connectionPromptEnabled); + await clientWrapper.setPreference(promptPrefName, connectionPromptEnabled); // Re-get actual value from the runtime. - connectionPromptEnabled = await client.getPreference(promptPrefName); + connectionPromptEnabled = await clientWrapper.getPreference(promptPrefName); dispatch({ type: UPDATE_CONNECTION_PROMPT_SETTING_SUCCESS, runtime, connectionPromptEnabled }); diff --git a/devtools/client/aboutdebugging-new/src/middleware/debug-target-listener.js b/devtools/client/aboutdebugging-new/src/middleware/debug-target-listener.js index d9af295f0383..da80dc2315d0 100644 --- a/devtools/client/aboutdebugging-new/src/middleware/debug-target-listener.js +++ b/devtools/client/aboutdebugging-new/src/middleware/debug-target-listener.js @@ -29,43 +29,45 @@ function debugTargetListenerMiddleware(store) { switch (action.type) { case WATCH_RUNTIME_SUCCESS: { const { runtime } = action; - const { client } = runtime.runtimeDetails; + const { clientWrapper } = runtime.runtimeDetails; if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.TAB)) { - client.addListener("tabListChanged", onTabsUpdated); + clientWrapper.addListener("tabListChanged", onTabsUpdated); } if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.EXTENSION)) { - client.addListener("addonListChanged", onExtensionsUpdated); + clientWrapper.addListener("addonListChanged", onExtensionsUpdated); } if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.WORKER)) { - client.addListener("workerListChanged", onWorkersUpdated); - client.addListener("serviceWorkerRegistrationListChanged", onWorkersUpdated); - client.addListener("processListChanged", onWorkersUpdated); - client.addListener("registration-changed", onWorkersUpdated); - client.addListener("push-subscription-modified", onWorkersUpdated); + clientWrapper.addListener("workerListChanged", onWorkersUpdated); + clientWrapper.addListener("serviceWorkerRegistrationListChanged", + onWorkersUpdated); + clientWrapper.addListener("processListChanged", onWorkersUpdated); + clientWrapper.addListener("registration-changed", onWorkersUpdated); + clientWrapper.addListener("push-subscription-modified", onWorkersUpdated); } break; } case UNWATCH_RUNTIME_START: { const { runtime } = action; - const { client } = runtime.runtimeDetails; + const { clientWrapper } = runtime.runtimeDetails; if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.TAB)) { - client.removeListener("tabListChanged", onTabsUpdated); + clientWrapper.removeListener("tabListChanged", onTabsUpdated); } if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.EXTENSION)) { - client.removeListener("addonListChanged", onExtensionsUpdated); + clientWrapper.removeListener("addonListChanged", onExtensionsUpdated); } if (isSupportedDebugTarget(runtime.type, DEBUG_TARGETS.WORKER)) { - client.removeListener("workerListChanged", onWorkersUpdated); - client.removeListener("serviceWorkerRegistrationListChanged", onWorkersUpdated); - client.removeListener("processListChanged", onWorkersUpdated); - client.removeListener("registration-changed", onWorkersUpdated); - client.removeListener("push-subscription-modified", onWorkersUpdated); + clientWrapper.removeListener("workerListChanged", onWorkersUpdated); + clientWrapper.removeListener("serviceWorkerRegistrationListChanged", + onWorkersUpdated); + clientWrapper.removeListener("processListChanged", onWorkersUpdated); + clientWrapper.removeListener("registration-changed", onWorkersUpdated); + clientWrapper.removeListener("push-subscription-modified", onWorkersUpdated); } break; } diff --git a/devtools/client/aboutdebugging-new/src/modules/runtime-client-factory.js b/devtools/client/aboutdebugging-new/src/modules/runtime-client-factory.js index 290aa87a20b3..a5b08c5a9226 100644 --- a/devtools/client/aboutdebugging-new/src/modules/runtime-client-factory.js +++ b/devtools/client/aboutdebugging-new/src/modules/runtime-client-factory.js @@ -16,7 +16,7 @@ async function createLocalClient() { DebuggerServer.registerAllActors(); const client = new DebuggerClient(DebuggerServer.connectPipe()); await client.connect(); - return { client: new ClientWrapper(client) }; + return { clientWrapper: new ClientWrapper(client) }; } async function createNetworkClient(host, port) { @@ -24,7 +24,7 @@ async function createNetworkClient(host, port) { const transport = await DebuggerClient.socketConnect(transportDetails); const client = new DebuggerClient(transport); await client.connect(); - return { client: new ClientWrapper(client), transportDetails }; + return { clientWrapper: new ClientWrapper(client), transportDetails }; } async function createUSBClient(socketPath) { diff --git a/devtools/client/aboutdebugging-new/src/modules/runtimes-state-helper.js b/devtools/client/aboutdebugging-new/src/modules/runtimes-state-helper.js index b2e49143fe6e..ebecffa3103f 100644 --- a/devtools/client/aboutdebugging-new/src/modules/runtimes-state-helper.js +++ b/devtools/client/aboutdebugging-new/src/modules/runtimes-state-helper.js @@ -12,7 +12,7 @@ exports.getCurrentRuntime = getCurrentRuntime; function getCurrentClient(runtimesState) { const runtimeDetails = getCurrentRuntimeDetails(runtimesState); - return runtimeDetails ? runtimeDetails.client : null; + return runtimeDetails ? runtimeDetails.clientWrapper : null; } exports.getCurrentClient = getCurrentClient; diff --git a/devtools/client/aboutdebugging-new/src/types.js b/devtools/client/aboutdebugging-new/src/types.js index e015d1b4d301..866086b90ed1 100644 --- a/devtools/client/aboutdebugging-new/src/types.js +++ b/devtools/client/aboutdebugging-new/src/types.js @@ -32,7 +32,7 @@ const runtimeTransportDetails = { const runtimeDetails = { // ClientWrapper built using a DebuggerClient for the runtime - client: PropTypes.instanceOf(ClientWrapper).isRequired, + clientWrapper: PropTypes.instanceOf(ClientWrapper).isRequired, // reflect devtools.debugger.prompt-connection preference of this runtime connectionPromptEnabled: PropTypes.bool.isRequired, diff --git a/devtools/client/aboutdebugging-new/test/browser/mocks/head-usb-mocks.js b/devtools/client/aboutdebugging-new/test/browser/mocks/head-usb-mocks.js index 4c44b554f032..8872e47f24fa 100644 --- a/devtools/client/aboutdebugging-new/test/browser/mocks/head-usb-mocks.js +++ b/devtools/client/aboutdebugging-new/test/browser/mocks/head-usb-mocks.js @@ -35,7 +35,7 @@ class UsbMocks { this.runtimeClientFactoryMock = createRuntimeClientFactoryMock(); this._clients = {}; this.runtimeClientFactoryMock.createClientForRuntime = runtime => { - return { client: this._clients[runtime.id] }; + return { clientWrapper: this._clients[runtime.id] }; }; // Add a client for THIS_FIREFOX, since about:debugging will start on the This Firefox From 07bc63a79a30f327c41d9b2d5f2ba01d169199e5 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Wed, 21 Nov 2018 07:15:07 +0000 Subject: [PATCH 02/21] Bug 1500005 - Add dedicated types folder for aboutdebugging types;r=ladybenko Differential Revision: https://phabricator.services.mozilla.com/D11780 --HG-- rename : devtools/client/aboutdebugging-new/src/types.js => devtools/client/aboutdebugging-new/src/types/runtime.js extra : moz-landing-system : lando --- .../client/aboutdebugging-new/src/components/App.js | 2 +- .../src/components/sidebar/Sidebar.js | 2 +- devtools/client/aboutdebugging-new/src/moz.build | 2 +- devtools/client/aboutdebugging-new/src/types/index.js | 11 +++++++++++ .../client/aboutdebugging-new/src/types/moz.build | 8 ++++++++ .../src/{types.js => types/runtime.js} | 2 +- 6 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 devtools/client/aboutdebugging-new/src/types/index.js create mode 100644 devtools/client/aboutdebugging-new/src/types/moz.build rename devtools/client/aboutdebugging-new/src/{types.js => types/runtime.js} (97%) diff --git a/devtools/client/aboutdebugging-new/src/components/App.js b/devtools/client/aboutdebugging-new/src/components/App.js index 19dbf00f77b4..861f56ebbbde 100644 --- a/devtools/client/aboutdebugging-new/src/components/App.js +++ b/devtools/client/aboutdebugging-new/src/components/App.js @@ -13,7 +13,7 @@ const FluentReact = require("devtools/client/shared/vendor/fluent-react"); const LocalizationProvider = createFactory(FluentReact.LocalizationProvider); const { PAGES } = require("../constants"); -const Types = require("../types"); +const Types = require("../types/index"); const ConnectPage = createFactory(require("./connect/ConnectPage")); const RuntimePage = createFactory(require("./RuntimePage")); diff --git a/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js b/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js index 1b084391d532..6e801ec567db 100644 --- a/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js +++ b/devtools/client/aboutdebugging-new/src/components/sidebar/Sidebar.js @@ -12,7 +12,7 @@ const FluentReact = require("devtools/client/shared/vendor/fluent-react"); const Localized = createFactory(FluentReact.Localized); const { PAGES, RUNTIMES } = require("../../constants"); -const Types = require("../../types"); +const Types = require("../../types/index"); loader.lazyRequireGetter(this, "ADB_ADDON_STATES", "devtools/shared/adb/adb-addon", true); const SidebarItem = createFactory(require("./SidebarItem")); diff --git a/devtools/client/aboutdebugging-new/src/moz.build b/devtools/client/aboutdebugging-new/src/moz.build index 5bf11cc46ed4..e4ccec91bb07 100644 --- a/devtools/client/aboutdebugging-new/src/moz.build +++ b/devtools/client/aboutdebugging-new/src/moz.build @@ -8,11 +8,11 @@ DIRS += [ 'middleware', 'modules', 'reducers', + 'types', ] DevToolsModules( 'base.css', 'constants.js', 'create-store.js', - 'types.js', ) diff --git a/devtools/client/aboutdebugging-new/src/types/index.js b/devtools/client/aboutdebugging-new/src/types/index.js new file mode 100644 index 000000000000..a0a7d54e18e0 --- /dev/null +++ b/devtools/client/aboutdebugging-new/src/types/index.js @@ -0,0 +1,11 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +"use strict"; + +const { runtime } = require("./runtime"); + +module.exports = Object.assign({}, { + runtime, +}); diff --git a/devtools/client/aboutdebugging-new/src/types/moz.build b/devtools/client/aboutdebugging-new/src/types/moz.build new file mode 100644 index 000000000000..6b6cba858d1b --- /dev/null +++ b/devtools/client/aboutdebugging-new/src/types/moz.build @@ -0,0 +1,8 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +DevToolsModules( + 'index.js', + 'runtime.js', +) \ No newline at end of file diff --git a/devtools/client/aboutdebugging-new/src/types.js b/devtools/client/aboutdebugging-new/src/types/runtime.js similarity index 97% rename from devtools/client/aboutdebugging-new/src/types.js rename to devtools/client/aboutdebugging-new/src/types/runtime.js index 866086b90ed1..41fdebce5b75 100644 --- a/devtools/client/aboutdebugging-new/src/types.js +++ b/devtools/client/aboutdebugging-new/src/types/runtime.js @@ -5,7 +5,7 @@ "use strict"; const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); -const { ClientWrapper } = require("./modules/client-wrapper"); +const { ClientWrapper } = require("../modules/client-wrapper"); const runtimeInfo = { // device name which is running the runtime, From 59ebb01f92c59d26c7352682b43898c1b4309c16 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Wed, 21 Nov 2018 07:20:28 +0000 Subject: [PATCH 03/21] Bug 1500005 - Stop storing worker front in aboutdebugging state;r=daisuke Depends on D11780 Differential Revision: https://phabricator.services.mozilla.com/D12512 --HG-- extra : moz-landing-system : lando --- .../src/actions/debug-targets.js | 3 ++- .../src/components/debugtarget/InspectAction.js | 2 +- .../src/middleware/worker-component-data.js | 17 +++++++++++------ 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/devtools/client/aboutdebugging-new/src/actions/debug-targets.js b/devtools/client/aboutdebugging-new/src/actions/debug-targets.js index a8c1bd9e4de3..9dcd43825da9 100644 --- a/devtools/client/aboutdebugging-new/src/actions/debug-targets.js +++ b/devtools/client/aboutdebugging-new/src/actions/debug-targets.js @@ -35,7 +35,7 @@ const { RUNTIMES, } = require("../constants"); -function inspectDebugTarget({ type, id, front }) { +function inspectDebugTarget(type, id) { return async (_, getState) => { const runtime = getCurrentRuntime(getState().runtimes); const { runtimeDetails, type: runtimeType } = runtime; @@ -63,6 +63,7 @@ function inspectDebugTarget({ type, id, front }) { } case DEBUG_TARGETS.WORKER: { // Open worker toolbox in new window. + const front = runtimeDetails.client.client.getActor(id); gDevToolsBrowser.openWorkerToolbox(front); break; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js index 3fe7cd7bb41c..6ab3c49649cb 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js @@ -26,7 +26,7 @@ class InspectAction extends PureComponent { inspect() { const { dispatch, target } = this.props; - dispatch(Actions.inspectDebugTarget(target)); + dispatch(Actions.inspectDebugTarget(target.type, target.id)); } render() { diff --git a/devtools/client/aboutdebugging-new/src/middleware/worker-component-data.js b/devtools/client/aboutdebugging-new/src/middleware/worker-component-data.js index ee5994fbcc14..2573a2c1b817 100644 --- a/devtools/client/aboutdebugging-new/src/middleware/worker-component-data.js +++ b/devtools/client/aboutdebugging-new/src/middleware/worker-component-data.js @@ -44,9 +44,14 @@ function toComponentData(workers, isServiceWorker) { return workers.map(worker => { // Here `worker` is the worker object created by RootFront.listAllWorkers const type = DEBUG_TARGETS.WORKER; - const front = worker.workerTargetFront; const icon = "chrome://devtools/skin/images/debugging-workers.svg"; - let { fetch, name, registrationActor, scope } = worker; + let { fetch, name, registrationActor, scope, workerTargetFront } = worker; + + // For registering service workers, workerTargetFront will not be available. + // The only valid identifier we can use at that point is the actorID for the + // service worker registration. + const id = workerTargetFront ? workerTargetFront.actorID : registrationActor; + let isActive = false; let isRunning = false; let status = null; @@ -60,10 +65,6 @@ function toComponentData(workers, isServiceWorker) { } return { - name, - icon, - front, - type, details: { fetch, isActive, @@ -72,6 +73,10 @@ function toComponentData(workers, isServiceWorker) { scope, status, }, + icon, + id, + name, + type, }; }); } From 8ffb5d613a5a13a5567a2e4f831ab2666cbc7b68 Mon Sep 17 00:00:00 2001 From: Julian Descottes Date: Wed, 21 Nov 2018 07:24:00 +0000 Subject: [PATCH 04/21] Bug 1500005 - Add types file for aboutdebugging debug-target;r=ladybenko,daisuke Depends on D11780 Differential Revision: https://phabricator.services.mozilla.com/D11781 --HG-- extra : moz-landing-system : lando --- .../components/debugtarget/DebugTargetItem.js | 4 +- .../components/debugtarget/DebugTargetList.js | 4 +- .../components/debugtarget/DebugTargetPane.js | 3 +- .../components/debugtarget/ExtensionDetail.js | 4 +- .../components/debugtarget/InspectAction.js | 3 +- .../debugtarget/ServiceWorkerAction.js | 3 +- .../src/components/debugtarget/TabDetail.js | 4 +- .../debugtarget/TemporaryExtensionAction.js | 3 +- .../components/debugtarget/WorkerDetail.js | 4 +- .../src/types/debug-target.js | 60 +++++++++++++++++++ .../aboutdebugging-new/src/types/index.js | 2 + .../aboutdebugging-new/src/types/moz.build | 1 + 12 files changed, 85 insertions(+), 10 deletions(-) create mode 100644 devtools/client/aboutdebugging-new/src/types/debug-target.js diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.js index 24e62950691d..b92bcf3e2132 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetItem.js @@ -8,6 +8,8 @@ const { PureComponent } = require("devtools/client/shared/vendor/react"); const dom = require("devtools/client/shared/vendor/react-dom-factories"); const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); +const Types = require("../../types/index"); + /** * This component displays debug target. */ @@ -17,7 +19,7 @@ class DebugTargetItem extends PureComponent { actionComponent: PropTypes.any.isRequired, detailComponent: PropTypes.any.isRequired, dispatch: PropTypes.func.isRequired, - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js index b6553098a40b..1c8a706e9700 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetList.js @@ -14,6 +14,8 @@ const Localized = createFactory(FluentReact.Localized); const DebugTargetItem = createFactory(require("./DebugTargetItem")); +const Types = require("../../types/index"); + /** * This component displays list of debug target. */ @@ -24,7 +26,7 @@ class DebugTargetList extends PureComponent { detailComponent: PropTypes.any.isRequired, dispatch: PropTypes.func.isRequired, isCollapsed: PropTypes.bool.isRequired, - targets: PropTypes.arrayOf(PropTypes.object).isRequired, + targets: PropTypes.arrayOf(Types.debugTarget).isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetPane.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetPane.js index 691e96ff5d0b..ca46cfa22dc1 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetPane.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/DebugTargetPane.js @@ -11,6 +11,7 @@ const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); const DebugTargetList = createFactory(require("./DebugTargetList")); const Actions = require("../../actions/index"); +const Types = require("../../types/index"); /** * This component provides list for debug target and name area. @@ -24,7 +25,7 @@ class DebugTargetPane extends PureComponent { dispatch: PropTypes.func.isRequired, isCollapsed: PropTypes.bool.isRequired, name: PropTypes.string.isRequired, - targets: PropTypes.arrayOf(PropTypes.object).isRequired, + targets: PropTypes.arrayOf(Types.debugTarget).isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionDetail.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionDetail.js index 443882196c5f..520a5c489d24 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionDetail.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/ExtensionDetail.js @@ -13,6 +13,8 @@ const Localized = createFactory(FluentReact.Localized); const FieldPair = createFactory(require("./FieldPair")); +const Types = require("../../types/index"); + /** * This component displays detail information for extension. */ @@ -21,7 +23,7 @@ class ExtensionDetail extends PureComponent { return { // Provided by wrapping the component with FluentReact.withLocalization. getString: PropTypes.func.isRequired, - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js index 6ab3c49649cb..aea25a245211 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/InspectAction.js @@ -12,6 +12,7 @@ const FluentReact = require("devtools/client/shared/vendor/fluent-react"); const Localized = createFactory(FluentReact.Localized); const Actions = require("../../actions/index"); +const Types = require("../../types/index"); /** * This component provides inspect button. @@ -20,7 +21,7 @@ class InspectAction extends PureComponent { static get propTypes() { return { dispatch: PropTypes.func.isRequired, - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/ServiceWorkerAction.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/ServiceWorkerAction.js index 70fd7704423e..f0747dd9f462 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/ServiceWorkerAction.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/ServiceWorkerAction.js @@ -13,6 +13,7 @@ const FluentReact = require("devtools/client/shared/vendor/fluent-react"); const InspectAction = createFactory(require("./InspectAction")); const Actions = require("../../actions/index"); +const Types = require("../../types/index"); /** * This component displays buttons for service worker. @@ -23,7 +24,7 @@ class ServiceWorkerAction extends PureComponent { dispatch: PropTypes.func.isRequired, // Provided by wrapping the component with FluentReact.withLocalization. getString: PropTypes.func.isRequired, - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/TabDetail.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/TabDetail.js index 552235682d5b..ec66a9dd0097 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/TabDetail.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/TabDetail.js @@ -6,7 +6,7 @@ const { PureComponent } = require("devtools/client/shared/vendor/react"); const dom = require("devtools/client/shared/vendor/react-dom-factories"); -const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); +const Types = require("../../types/index"); /** * This component displays detail information for tab. @@ -14,7 +14,7 @@ const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); class TabDetail extends PureComponent { static get propTypes() { return { - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js index ea2b57d839a8..4e86827ff094 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js @@ -14,6 +14,7 @@ const Localized = createFactory(FluentReact.Localized); const InspectAction = createFactory(require("./InspectAction")); const Actions = require("../../actions/index"); +const Types = require("../../types/index"); /** * This component provides components that inspect/reload/remove temporary extension. @@ -22,7 +23,7 @@ class TemporaryExtensionAction extends PureComponent { static get propTypes() { return { dispatch: PropTypes.func.isRequired, - target: PropTypes.object.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/components/debugtarget/WorkerDetail.js b/devtools/client/aboutdebugging-new/src/components/debugtarget/WorkerDetail.js index 14fc4de2c5ca..47482eb97c7a 100644 --- a/devtools/client/aboutdebugging-new/src/components/debugtarget/WorkerDetail.js +++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/WorkerDetail.js @@ -17,15 +17,17 @@ const { const FieldPair = createFactory(require("./FieldPair")); +const Types = require("../../types/index"); + /** * This component displays detail information for worker. */ class WorkerDetail extends PureComponent { static get propTypes() { return { - target: PropTypes.object.isRequired, // Provided by wrapping the component with FluentReact.withLocalization. getString: PropTypes.func.isRequired, + target: Types.debugTarget.isRequired, }; } diff --git a/devtools/client/aboutdebugging-new/src/types/debug-target.js b/devtools/client/aboutdebugging-new/src/types/debug-target.js new file mode 100644 index 000000000000..492750d74e0e --- /dev/null +++ b/devtools/client/aboutdebugging-new/src/types/debug-target.js @@ -0,0 +1,60 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +"use strict"; + +const PropTypes = require("devtools/client/shared/vendor/react-prop-types"); + +const extensionTargetDetails = { + // actor ID for this extention. + actor: PropTypes.string.isRequired, + location: PropTypes.string.isRequired, + // manifestURL points to the manifest.json file. This URL is only valid when debugging + // local extensions so it might be null. + manifestURL: PropTypes.string, + // unique extension id. + uuid: PropTypes.string.isRequired, +}; + +const tabTargetDetails = { + // the url of the tab. + url: PropTypes.string.isRequired, +}; + +const workerTargetDetails = { + // (service worker specific) one of "LISTENING", "NOT_LISTENING". undefined otherwise. + fetch: PropTypes.string, + // (service worker specific) true if they reached the activated state. + isActive: PropTypes.bool, + // (service worker specific) true if they are currently running. + isRunning: PropTypes.bool, + // actor id for the ServiceWorkerRegistration related to this service worker. + registrationActor: PropTypes.string, + // (service worker specific) scope of the service worker registration. + scope: PropTypes.string, + // (service worker specific) one of "RUNNING", "REGISTERING", "STOPPED". + status: PropTypes.string, +}; + +const debugTarget = { + // details property will contain a type-specific object. + details: PropTypes.oneOfType([ + PropTypes.shape(extensionTargetDetails), + PropTypes.shape(tabTargetDetails), + PropTypes.shape(workerTargetDetails), + ]).isRequired, + // icon to display for the debug target. + icon: PropTypes.string.isRequired, + // unique id for the target (unique in the scope of the application lifecycle). + // - extensions: {String} extension id (for instance "someextension@mozilla.org") + // - tabs: {Number} outerWindowID + // - workers: {String} id for the WorkerTargetActor corresponding to the worker + id: PropTypes.oneOfType([PropTypes.number, PropTypes.string]).isRequired, + // display name for the debug target. + name: PropTypes.string.isRequired, + // one of "EXTENSION", "TAB", "WORKER". + type: PropTypes.string.isRequired, +}; + +exports.debugTarget = PropTypes.shape(debugTarget); diff --git a/devtools/client/aboutdebugging-new/src/types/index.js b/devtools/client/aboutdebugging-new/src/types/index.js index a0a7d54e18e0..fbee6347431d 100644 --- a/devtools/client/aboutdebugging-new/src/types/index.js +++ b/devtools/client/aboutdebugging-new/src/types/index.js @@ -4,8 +4,10 @@ "use strict"; +const { debugTarget } = require("./debug-target"); const { runtime } = require("./runtime"); module.exports = Object.assign({}, { + debugTarget, runtime, }); diff --git a/devtools/client/aboutdebugging-new/src/types/moz.build b/devtools/client/aboutdebugging-new/src/types/moz.build index 6b6cba858d1b..461c3db4b5e5 100644 --- a/devtools/client/aboutdebugging-new/src/types/moz.build +++ b/devtools/client/aboutdebugging-new/src/types/moz.build @@ -3,6 +3,7 @@ # file, You can obtain one at http://mozilla.org/MPL/2.0/. DevToolsModules( + 'debug-target.js', 'index.js', 'runtime.js', ) \ No newline at end of file From da75669fb68740c6e9c778299d8eef21caf2d5ed Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 19 Nov 2018 13:00:53 +0000 Subject: [PATCH 05/21] Bug 1498166 - Use fully-qualified name for histogram storage r=chutten This is a preparation for the following introduction of a new wrapper type by that name, making it more explicit which class we're handling Differential Revision: https://phabricator.services.mozilla.com/D11904 --HG-- extra : moz-landing-system : lando --- .../telemetry/core/TelemetryHistogram.cpp | 105 +++++++++--------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/toolkit/components/telemetry/core/TelemetryHistogram.cpp b/toolkit/components/telemetry/core/TelemetryHistogram.cpp index 75f4606e1244..5eec069ffb76 100644 --- a/toolkit/components/telemetry/core/TelemetryHistogram.cpp +++ b/toolkit/components/telemetry/core/TelemetryHistogram.cpp @@ -30,7 +30,6 @@ #include "TelemetryHistogramNameMap.h" #include "TelemetryScalar.h" -using base::Histogram; using base::BooleanHistogram; using base::CountHistogram; using base::FlagHistogram; @@ -159,9 +158,9 @@ struct HistogramInfo { // Structs used to keep information about the histograms for which a // snapshot should be created. struct HistogramSnapshotData { - nsTArray mBucketRanges; - nsTArray mBucketCounts; - int64_t mSampleSum; // Same type as Histogram::SampleSet::sum_ + nsTArray mBucketRanges; + nsTArray mBucketCounts; + int64_t mSampleSum; // Same type as base::Histogram::SampleSet::sum_ }; struct HistogramSnapshotInfo { @@ -187,8 +186,8 @@ class KeyedHistogram { public: KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired); ~KeyedHistogram(); - nsresult GetHistogram(const nsCString& name, Histogram** histogram); - Histogram* GetHistogram(const nsCString& name); + nsresult GetHistogram(const nsCString& name, base::Histogram** histogram); + base::Histogram* GetHistogram(const nsCString& name); uint32_t GetHistogramType() const { return mHistogramInfo.histogramType; } nsresult GetKeys(const StaticMutexAutoLock& aLock, nsTArray& aKeys); // Note: unlike other methods, GetJSSnapshot is thread safe. @@ -209,7 +208,7 @@ public: size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf); private: - typedef nsBaseHashtableET KeyedHistogramEntry; + typedef nsBaseHashtableET KeyedHistogramEntry; typedef AutoHashtable KeyedHistogramMapType; KeyedHistogramMapType mHistogramMap; @@ -238,12 +237,12 @@ bool gCanRecordExtended = false; // The storage for actual Histogram instances. // We use separate ones for plain and keyed histograms. -Histogram** gHistogramStorage; +base::Histogram** gHistogramStorage; // Keyed histograms internally map string keys to individual Histogram instances. KeyedHistogram** gKeyedHistogramStorage; // To simplify logic below we use a single histogram instance for all expired histograms. -Histogram* gExpiredHistogram = nullptr; +base::Histogram* gExpiredHistogram = nullptr; // The single placeholder for expired keyed histograms. KeyedHistogram* gExpiredKeyedHistogram = nullptr; @@ -303,9 +302,9 @@ size_t internal_HistogramStorageIndex(const StaticMutexAutoLock& aLock, return aHistogramId * size_t(ProcessID::Count) + size_t(aProcessId); } -Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLock, - HistogramID aHistogramId, - ProcessID aProcessId) +base::Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLock, + HistogramID aHistogramId, + ProcessID aProcessId) { size_t index = internal_HistogramStorageIndex(aLock, aHistogramId, aProcessId); return gHistogramStorage[index]; @@ -314,7 +313,7 @@ Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLock, void internal_SetHistogramInStorage(const StaticMutexAutoLock& aLock, HistogramID aHistogramId, ProcessID aProcessId, - Histogram* aHistogram) + base::Histogram* aHistogram) { MOZ_ASSERT(XRE_IsParentProcess(), "Histograms are stored only in the parent process."); @@ -346,7 +345,7 @@ void internal_SetKeyedHistogramInStorage(HistogramID aHistogramId, } // Factory function for histogram instances. -Histogram* +base::Histogram* internal_CreateHistogramInstance(const HistogramInfo& info, int bucketsOffset); bool @@ -357,7 +356,7 @@ internal_IsHistogramEnumId(HistogramID aID) } // Look up a plain histogram by id. -Histogram* +base::Histogram* internal_GetHistogramById(const StaticMutexAutoLock& aLock, HistogramID histogramId, ProcessID processId, @@ -367,7 +366,7 @@ internal_GetHistogramById(const StaticMutexAutoLock& aLock, MOZ_ASSERT(!gHistogramInfos[histogramId].keyed); MOZ_ASSERT(processId < ProcessID::Count); - Histogram* h = internal_GetHistogramFromStorage(aLock, histogramId, processId); + base::Histogram* h = internal_GetHistogramFromStorage(aLock, histogramId, processId); if (h || !instantiate) { return h; } @@ -482,13 +481,13 @@ internal_AttemptedGPUProcess() { // Note: this is completely unrelated to mozilla::IsEmpty. bool -internal_IsEmpty(const StaticMutexAutoLock& aLock, const Histogram *h) +internal_IsEmpty(const StaticMutexAutoLock& aLock, const base::Histogram *h) { return h->is_empty(); } bool -internal_IsExpired(const StaticMutexAutoLock& aLock, Histogram* h) +internal_IsExpired(const StaticMutexAutoLock& aLock, base::Histogram* h) { return h == gExpiredHistogram; } @@ -605,7 +604,7 @@ internal_CheckHistogramArguments(const HistogramInfo& info) return NS_OK; } -Histogram* +base::Histogram* internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOffset) { if (NS_FAILED(internal_CheckHistogramArguments(passedInfo))) { @@ -633,11 +632,11 @@ internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOff info.histogramType = nsITelemetry::HISTOGRAM_LINEAR; } - Histogram::Flags flags = Histogram::kNoFlags; - Histogram* h = nullptr; + base::Histogram::Flags flags = base::Histogram::kNoFlags; + base::Histogram* h = nullptr; switch (info.histogramType) { case nsITelemetry::HISTOGRAM_EXPONENTIAL: - h = Histogram::FactoryGet(info.min, info.max, info.bucketCount, flags, buckets); + h = base::Histogram::FactoryGet(info.min, info.max, info.bucketCount, flags, buckets); break; case nsITelemetry::HISTOGRAM_LINEAR: case nsITelemetry::HISTOGRAM_CATEGORICAL: @@ -666,7 +665,7 @@ internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOff nsresult internal_HistogramAdd(const StaticMutexAutoLock& aLock, - Histogram& histogram, + base::Histogram& histogram, const HistogramID id, uint32_t value, ProcessID aProcessType) @@ -721,7 +720,7 @@ namespace { */ nsresult internal_GetHistogramAndSamples(const StaticMutexAutoLock& aLock, - const Histogram *h, + const base::Histogram *h, HistogramSnapshotData& aSnapshot) { MOZ_ASSERT(h); @@ -735,7 +734,7 @@ internal_GetHistogramAndSamples(const StaticMutexAutoLock& aLock, } // Get a snapshot of the samples. - Histogram::SampleSet ss; + base::Histogram::SampleSet ss; h->SnapshotSample(&ss); // Get the number of samples in each bucket. @@ -840,7 +839,7 @@ internal_ReflectHistogramAndSamples(JSContext *cx, } bool -internal_ShouldReflectHistogram(const StaticMutexAutoLock& aLock, Histogram* h, HistogramID id) +internal_ShouldReflectHistogram(const StaticMutexAutoLock& aLock, base::Histogram* h, HistogramID id) { // Only flag histograms are serialized when they are empty. // This has historical reasons, changing this will require downstream changes. @@ -905,7 +904,7 @@ internal_GetHistogramsSnapshot(const StaticMutexAutoLock& aLock, bool shouldInstantiate = info.histogramType == nsITelemetry::HISTOGRAM_FLAG; - Histogram* h = internal_GetHistogramById(aLock, id, ProcessID(process), + base::Histogram* h = internal_GetHistogramById(aLock, id, ProcessID(process), shouldInstantiate); if (!h || internal_IsExpired(aLock, h) || !internal_ShouldReflectHistogram(aLock, h, id)) { continue; @@ -986,7 +985,7 @@ KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info, bool e KeyedHistogram::~KeyedHistogram() { for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { - Histogram* h = iter.Get()->mData; + base::Histogram* h = iter.Get()->mData; if (h == gExpiredHistogram) { continue; } @@ -996,7 +995,7 @@ KeyedHistogram::~KeyedHistogram() } nsresult -KeyedHistogram::GetHistogram(const nsCString& key, Histogram** histogram) +KeyedHistogram::GetHistogram(const nsCString& key, base::Histogram** histogram) { KeyedHistogramEntry* entry = mHistogramMap.GetEntry(key); if (entry) { @@ -1005,12 +1004,12 @@ KeyedHistogram::GetHistogram(const nsCString& key, Histogram** histogram) } int bucketsOffset = gHistogramBucketLowerBoundIndex[mId]; - Histogram* h = internal_CreateHistogramInstance(mHistogramInfo, bucketsOffset); + base::Histogram* h = internal_CreateHistogramInstance(mHistogramInfo, bucketsOffset); if (!h) { return NS_ERROR_FAILURE; } - h->ClearFlags(Histogram::kUmaTargetedHistogramFlag); + h->ClearFlags(base::Histogram::kUmaTargetedHistogramFlag); *histogram = h; entry = mHistogramMap.PutEntry(key); @@ -1022,10 +1021,10 @@ KeyedHistogram::GetHistogram(const nsCString& key, Histogram** histogram) return NS_OK; } -Histogram* +base::Histogram* KeyedHistogram::GetHistogram(const nsCString& key) { - Histogram* h = nullptr; + base::Histogram* h = nullptr; if (NS_FAILED(GetHistogram(key, &h))) { return nullptr; } @@ -1056,7 +1055,7 @@ KeyedHistogram::Add(const nsCString& key, uint32_t sample, return NS_OK; } - Histogram* histogram = GetHistogram(key); + base::Histogram* histogram = GetHistogram(key); MOZ_ASSERT(histogram); if (!histogram) { return NS_ERROR_FAILURE; @@ -1085,7 +1084,7 @@ KeyedHistogram::Clear() } for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { - Histogram* h = iter.Get()->mData; + base::Histogram* h = iter.Get()->mData; if (h == gExpiredHistogram) { continue; } @@ -1145,7 +1144,7 @@ KeyedHistogram::GetSnapshot(const StaticMutexAutoLock& aLock, { // Snapshot every key. for (auto iter = mHistogramMap.ConstIter(); !iter.Done(); iter.Next()) { - Histogram* keyData = iter.Get()->mData; + base::Histogram* keyData = iter.Get()->mData; if (!keyData) { return NS_ERROR_FAILURE; } @@ -1290,7 +1289,7 @@ void internal_Accumulate(const StaticMutexAutoLock& aLock, HistogramID aId, uint return; } - Histogram *h = internal_GetHistogramById(aLock, aId, ProcessID::Parent); + base::Histogram *h = internal_GetHistogramById(aLock, aId, ProcessID::Parent); MOZ_ASSERT(h); internal_HistogramAdd(aLock, *h, aId, aSample, ProcessID::Parent); } @@ -1319,7 +1318,7 @@ internal_AccumulateChild(const StaticMutexAutoLock& aLock, return; } - if (Histogram* h = internal_GetHistogramById(aLock, aId, aProcessType)) { + if (base::Histogram* h = internal_GetHistogramById(aLock, aId, aProcessType)) { internal_HistogramAdd(aLock, *h, aId, aSample, aProcessType); } else { NS_WARNING("Failed GetHistogramById for CHILD"); @@ -1589,7 +1588,7 @@ internal_JSHistogram_Snapshot(JSContext *cx, unsigned argc, JS::Value *vp) // This is not good standard behavior given that we have histogram instances // covering multiple processes. // However, changing this requires some broader changes to callers. - Histogram* h = internal_GetHistogramById(locker, id, ProcessID::Parent); + base::Histogram* h = internal_GetHistogramById(locker, id, ProcessID::Parent); // Take a snapshot of the data here, protected by the lock, and then, // outside of the lock protection, mirror it to a JS structure if (NS_FAILED(internal_GetHistogramAndSamples(locker, h, dataSnapshot))) { @@ -1786,7 +1785,7 @@ internal_KeyedHistogram_SnapshotImpl(JSContext *cx, unsigned argc, StaticMutexAutoLock locker(gTelemetryHistogramMutex); // Get data for the key we're looking for. - Histogram* h = nullptr; + base::Histogram* h = nullptr; nsresult rv = keyed->GetHistogram(NS_ConvertUTF16toUTF8(key), &h); if (NS_FAILED(rv)) { return false; @@ -2052,7 +2051,7 @@ void TelemetryHistogram::InitializeGlobalState(bool canRecordBase, if (XRE_IsParentProcess()) { gHistogramStorage = - new Histogram*[HistogramCount * size_t(ProcessID::Count)] {}; + new base::Histogram*[HistogramCount * size_t(ProcessID::Count)] {}; gKeyedHistogramStorage = new KeyedHistogram*[HistogramCount * size_t(ProcessID::Count)] {}; } @@ -2644,7 +2643,7 @@ TelemetryHistogram::GetHistogramSizesOfIncludingThis(mozilla::MallocSizeOf // If we allocated the array, let's count the number of pointers in there. if (gHistogramStorage) { - n += HistogramCount * size_t(ProcessID::Count) * sizeof(Histogram*); + n += HistogramCount * size_t(ProcessID::Count) * sizeof(base::Histogram*); for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) { if (gHistogramStorage[i] && gHistogramStorage[i] != gExpiredHistogram) { n += gHistogramStorage[i]->SizeOfIncludingThis(aMallocSizeOf); @@ -2670,14 +2669,14 @@ TelemetryHistogram::GetHistogramSizesOfIncludingThis(mozilla::MallocSizeOf // PRIVATE: GeckoView specific helpers namespace base { -class PersistedSampleSet : public Histogram::SampleSet +class PersistedSampleSet : public base::Histogram::SampleSet { public: - explicit PersistedSampleSet(const nsTArray& aCounts, + explicit PersistedSampleSet(const nsTArray& aCounts, int64_t aSampleSum); }; -PersistedSampleSet::PersistedSampleSet(const nsTArray& aCounts, +PersistedSampleSet::PersistedSampleSet(const nsTArray& aCounts, int64_t aSampleSum) { // Initialize the data in the base class. See Histogram::SampleSet @@ -2746,7 +2745,7 @@ internal_CanRecordHistogram(const HistogramID id, nsresult internal_ParseHistogramData(JSContext* aCx, JS::HandleId aEntryId, JS::HandleObject aContainerObj, nsACString& aOutName, - nsTArray& aOutCountArray, int64_t& aOutSum) + nsTArray& aOutCountArray, int64_t& aOutSum) { // Get the histogram name. nsAutoJSString histogramName; @@ -2765,7 +2764,7 @@ internal_ParseHistogramData(JSContext* aCx, JS::HandleId aEntryId, } if (!histogramData.isObject()) { - // Histogram data need to be an object. If that's not the case, skip it + // base::Histogram data need to be an object. If that's not the case, skip it // and try to load the rest of the data. return NS_ERROR_FAILURE; } @@ -2948,7 +2947,7 @@ TelemetryHistogram::DeserializeHistograms(JSContext* aCx, JS::HandleValue aData) return NS_OK; } - typedef mozilla::Tuple, int64_t> + typedef mozilla::Tuple, int64_t> PersistedHistogramTuple; typedef mozilla::Vector PersistedHistogramArray; typedef mozilla::Vector PersistedHistogramStorage; @@ -3027,7 +3026,7 @@ TelemetryHistogram::DeserializeHistograms(JSContext* aCx, JS::HandleValue aData) histogram = histogramVal; int64_t sum = 0; - nsTArray deserializedCounts; + nsTArray deserializedCounts; nsCString histogramName; if (NS_FAILED(internal_ParseHistogramData(aCx, histogram, processDataObj, histogramName, deserializedCounts, sum))) { @@ -3063,7 +3062,7 @@ TelemetryHistogram::DeserializeHistograms(JSContext* aCx, JS::HandleValue aData) } // Get the Histogram instance: this will instantiate it if it doesn't exist. - Histogram* h = internal_GetHistogramById(locker, id, procID); + base::Histogram* h = internal_GetHistogramById(locker, id, procID); MOZ_ASSERT(h); if (!h || internal_IsExpired(locker, h)) { @@ -3104,7 +3103,7 @@ TelemetryHistogram::DeserializeKeyedHistograms(JSContext* aCx, JS::HandleValue a return NS_OK; } - typedef mozilla::Tuple, int64_t> + typedef mozilla::Tuple, int64_t> PersistedKeyedHistogramTuple; typedef mozilla::Vector PersistedKeyedHistogramArray; typedef mozilla::Vector PersistedKeyedHistogramStorage; @@ -3208,7 +3207,7 @@ TelemetryHistogram::DeserializeKeyedHistograms(JSContext* aCx, JS::HandleValue a key = keyVal; int64_t sum = 0; - nsTArray deserializedCounts; + nsTArray deserializedCounts; nsCString keyName; if (NS_FAILED(internal_ParseHistogramData(aCx, key, keysDataObj, keyName, deserializedCounts, sum))) { @@ -3254,7 +3253,7 @@ TelemetryHistogram::DeserializeKeyedHistograms(JSContext* aCx, JS::HandleValue a } // Get data for the key we're looking for. - Histogram* h = nullptr; + base::Histogram* h = nullptr; if (NS_FAILED(keyed->GetHistogram(mozilla::Get<1>(histogramData), &h))) { continue; } From 274e44864fe2e394d0b6e449900b7fd5b36e07f5 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 19 Nov 2018 14:44:25 +0000 Subject: [PATCH 06/21] Bug 1498166 - Add multi-storage to plain histograms r=chutten This introduces a new wrapper `Histogram` that keeps track of the multiple stores a histogram can be in. Histograms are stored in a hash table, indexed by the name of the store. It has one optimization to support the majority of cases: a single `main` store. For that it stores a direct pointer to the underlying base::Histogram and skips populating the map. This saves an indirection and memory overhead of actually placing it into the hash table. For now a snapshot only ever returns data from the main store. Clearing a snapshot only clears the main store. (This will both change in a follow-up) Depends on D11904 Differential Revision: https://phabricator.services.mozilla.com/D11905 --HG-- extra : moz-landing-system : lando --- .../telemetry/core/TelemetryHistogram.cpp | 333 +++++++++++++++--- 1 file changed, 278 insertions(+), 55 deletions(-) diff --git a/toolkit/components/telemetry/core/TelemetryHistogram.cpp b/toolkit/components/telemetry/core/TelemetryHistogram.cpp index 5eec069ffb76..2c2f3e628ce3 100644 --- a/toolkit/components/telemetry/core/TelemetryHistogram.cpp +++ b/toolkit/components/telemetry/core/TelemetryHistogram.cpp @@ -153,6 +153,7 @@ struct HistogramInfo { const char *expiration() const; nsresult label_id(const char* label, uint32_t* labelId) const; bool allows_key(const nsACString& key) const; + bool is_single_store() const; }; // Structs used to keep information about the histograms for which a @@ -182,6 +183,54 @@ struct KeyedHistogramSnapshotInfo { typedef mozilla::Vector KeyedHistogramSnapshotsArray; typedef mozilla::Vector KeyedHistogramProcessSnapshotsArray; +/** + * A Histogram storage engine. + * + * Takes care of recording data into multiple stores if necessary. + */ +class Histogram { +public: + /* + * Create a new histogram instance from the given info. + * + * If the histogram is already expired, this does not allocate. + */ + Histogram(HistogramID histogramId, const HistogramInfo& info, bool expired); + ~Histogram(); + + /** + * Add a sample to this histogram in all registered stores. + */ + void Add(uint32_t sample); + + /** + * Clear the main store for this histogram. + * TODO(bug 1498173): Add clearing of specific store. + */ + void Clear(); + + /** + * Get the histogram instance from the named store. + */ + bool GetHistogram(const nsACString& store, base::Histogram** h); + + bool IsExpired() const { return mIsExpired; } + + size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf); + +private: + // String -> Histogram* + typedef nsClassHashtable HistogramStore; + HistogramStore mStorage; + + // A valid pointer if this histogram belongs to only the main store + base::Histogram* mSingleStore; + + // We don't track stores for expired histograms. + // We just store a single flag and all other operations become a no-op. + bool mIsExpired; +}; + class KeyedHistogram { public: KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired); @@ -237,12 +286,13 @@ bool gCanRecordExtended = false; // The storage for actual Histogram instances. // We use separate ones for plain and keyed histograms. -base::Histogram** gHistogramStorage; +Histogram** gHistogramStorage; // Keyed histograms internally map string keys to individual Histogram instances. KeyedHistogram** gKeyedHistogramStorage; // To simplify logic below we use a single histogram instance for all expired histograms. -base::Histogram* gExpiredHistogram = nullptr; +base::Histogram* gExpiredBaseHistogram = nullptr; +Histogram* gExpiredHistogram = nullptr; // The single placeholder for expired keyed histograms. KeyedHistogram* gExpiredKeyedHistogram = nullptr; @@ -302,9 +352,9 @@ size_t internal_HistogramStorageIndex(const StaticMutexAutoLock& aLock, return aHistogramId * size_t(ProcessID::Count) + size_t(aProcessId); } -base::Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLock, - HistogramID aHistogramId, - ProcessID aProcessId) +Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLock, + HistogramID aHistogramId, + ProcessID aProcessId) { size_t index = internal_HistogramStorageIndex(aLock, aHistogramId, aProcessId); return gHistogramStorage[index]; @@ -313,7 +363,7 @@ base::Histogram* internal_GetHistogramFromStorage(const StaticMutexAutoLock& aLo void internal_SetHistogramInStorage(const StaticMutexAutoLock& aLock, HistogramID aHistogramId, ProcessID aProcessId, - base::Histogram* aHistogram) + Histogram* aHistogram) { MOZ_ASSERT(XRE_IsParentProcess(), "Histograms are stored only in the parent process."); @@ -344,9 +394,13 @@ void internal_SetKeyedHistogramInStorage(HistogramID aHistogramId, gKeyedHistogramStorage[index] = aKeyedHistogram; } -// Factory function for histogram instances. +// Factory function for base::Histogram instances. base::Histogram* -internal_CreateHistogramInstance(const HistogramInfo& info, int bucketsOffset); +internal_CreateBaseHistogramInstance(const HistogramInfo& info, int bucketsOffset); + +// Factory function for histogram instances. +Histogram* +internal_CreateHistogramInstance(HistogramID histogramId); bool internal_IsHistogramEnumId(HistogramID aID) @@ -356,7 +410,7 @@ internal_IsHistogramEnumId(HistogramID aID) } // Look up a plain histogram by id. -base::Histogram* +Histogram* internal_GetHistogramById(const StaticMutexAutoLock& aLock, HistogramID histogramId, ProcessID processId, @@ -366,16 +420,15 @@ internal_GetHistogramById(const StaticMutexAutoLock& aLock, MOZ_ASSERT(!gHistogramInfos[histogramId].keyed); MOZ_ASSERT(processId < ProcessID::Count); - base::Histogram* h = internal_GetHistogramFromStorage(aLock, histogramId, processId); + Histogram* h = internal_GetHistogramFromStorage(aLock, histogramId, processId); if (h || !instantiate) { return h; } - const HistogramInfo& info = gHistogramInfos[histogramId]; - const int bucketsOffset = gHistogramBucketLowerBoundIndex[histogramId]; - h = internal_CreateHistogramInstance(info, bucketsOffset); + h = internal_CreateHistogramInstance(histogramId); MOZ_ASSERT(h); internal_SetHistogramInStorage(aLock, histogramId, processId, h); + return h; } @@ -435,21 +488,6 @@ internal_GetHistogramIdByName(const StaticMutexAutoLock& aLock, return NS_ERROR_ILLEGAL_VALUE; } -// Clear a histogram from storage. -void -internal_ClearHistogramById(const StaticMutexAutoLock& aLock, - HistogramID histogramId, - ProcessID processId) -{ - size_t index = internal_HistogramStorageIndex(aLock, histogramId, processId); - if (gHistogramStorage[index] == gExpiredHistogram) { - // We keep gExpiredHistogram until TelemetryHistogram::DeInitializeGlobalState - return; - } - delete gHistogramStorage[index]; - gHistogramStorage[index] = nullptr; -} - } //////////////////////////////////////////////////////////////////////// @@ -489,7 +527,7 @@ internal_IsEmpty(const StaticMutexAutoLock& aLock, const base::Histogram *h) bool internal_IsExpired(const StaticMutexAutoLock& aLock, base::Histogram* h) { - return h == gExpiredHistogram; + return h == gExpiredBaseHistogram; } void @@ -571,6 +609,12 @@ HistogramInfo::allows_key(const nsACString& key) const return false; } +bool +HistogramInfo::is_single_store() const +{ + return store_count == 1 && store_index == UINT16_MAX; +} + } // namespace @@ -604,8 +648,33 @@ internal_CheckHistogramArguments(const HistogramInfo& info) return NS_OK; } +Histogram* +internal_CreateHistogramInstance(HistogramID histogramId) +{ + const HistogramInfo& info = gHistogramInfos[histogramId]; + + if (NS_FAILED(internal_CheckHistogramArguments(info))) { + MOZ_ASSERT(false, "Failed histogram argument checks."); + return nullptr; + } + + const bool isExpired = IsExpiredVersion(info.expiration()); + + if (isExpired) { + if (!gExpiredHistogram) { + gExpiredHistogram = new Histogram(histogramId, info, /* expired */ true); + } + + return gExpiredHistogram; + } + + Histogram *wrapper = new Histogram(histogramId, info, /* expired */ false); + + return wrapper; +} + base::Histogram* -internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOffset) +internal_CreateBaseHistogramInstance(const HistogramInfo& passedInfo, int bucketsOffset) { if (NS_FAILED(internal_CheckHistogramArguments(passedInfo))) { MOZ_ASSERT(false, "Failed histogram argument checks."); @@ -619,8 +688,8 @@ internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOff const int* buckets = &gHistogramBucketLowerBounds[bucketsOffset]; if (isExpired) { - if (gExpiredHistogram) { - return gExpiredHistogram; + if (gExpiredBaseHistogram) { + return gExpiredBaseHistogram; } // The first values in gHistogramBucketLowerBounds are reserved for @@ -657,7 +726,7 @@ internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOff } if (isExpired) { - gExpiredHistogram = h; + gExpiredBaseHistogram = h; } return h; @@ -665,7 +734,7 @@ internal_CreateHistogramInstance(const HistogramInfo& passedInfo, int bucketsOff nsresult internal_HistogramAdd(const StaticMutexAutoLock& aLock, - base::Histogram& histogram, + Histogram& histogram, const HistogramID id, uint32_t value, ProcessID aProcessType) @@ -904,9 +973,19 @@ internal_GetHistogramsSnapshot(const StaticMutexAutoLock& aLock, bool shouldInstantiate = info.histogramType == nsITelemetry::HISTOGRAM_FLAG; - base::Histogram* h = internal_GetHistogramById(aLock, id, ProcessID(process), + Histogram* w = internal_GetHistogramById(aLock, id, ProcessID(process), shouldInstantiate); - if (!h || internal_IsExpired(aLock, h) || !internal_ShouldReflectHistogram(aLock, h, id)) { + if (!w || w->IsExpired()) { + continue; + } + + base::Histogram *h = nullptr; + NS_NAMED_LITERAL_CSTRING(store, "main"); + if (!w->GetHistogram(store, &h)) { + continue; + } + + if (!internal_ShouldReflectHistogram(aLock, h, id)) { continue; } @@ -938,6 +1017,125 @@ internal_GetHistogramsSnapshot(const StaticMutexAutoLock& aLock, } // namespace +//////////////////////////////////////////////////////////////////////// +//////////////////////////////////////////////////////////////////////// +// +// PRIVATE: class Histogram + +namespace { + +Histogram::Histogram(HistogramID histogramId, const HistogramInfo& info, bool expired) + : mStorage() + , mSingleStore(nullptr) + , mIsExpired(expired) +{ + if (IsExpired()) { + return; + } + + base::Histogram* h; + const int bucketsOffset = gHistogramBucketLowerBoundIndex[histogramId]; + + if (info.is_single_store()) { + mSingleStore = internal_CreateBaseHistogramInstance(info, bucketsOffset); + } else { + for (uint32_t i = 0; i < info.store_count; i++) { + auto store = nsDependentCString(&gHistogramStringTable[gHistogramStoresTable[info.store_index + i]]); + h = internal_CreateBaseHistogramInstance(info, bucketsOffset); + mStorage.Put(store, h); + } + } +} + +Histogram::~Histogram() +{ + delete mSingleStore; +} + +void +Histogram::Add(uint32_t sample) +{ + MOZ_ASSERT(XRE_IsParentProcess(), "Only add to histograms in the parent process"); + if (!XRE_IsParentProcess()) { + return; + } + + if (IsExpired()) { + return; + } + + if (mSingleStore != nullptr) { + mSingleStore->Add(sample); + } else { + for (auto iter = mStorage.Iter(); !iter.Done(); iter.Next()) { + auto& h = iter.Data(); + h->Add(sample); + } + } +} + +void +Histogram::Clear() +{ + MOZ_ASSERT(XRE_IsParentProcess(), "Only clear histograms in the parent process"); + if (!XRE_IsParentProcess()) { + return; + } + + if (mSingleStore != nullptr) { + mSingleStore->Clear(); + } else { + base::Histogram* h = nullptr; + bool found = GetHistogram(NS_LITERAL_CSTRING("main"), &h); + if (!found) { + return; + } + MOZ_ASSERT(h, "Should have found a valid histogram in the main store"); + + h->Clear(); + } +} + +bool +Histogram::GetHistogram(const nsACString& store, base::Histogram** h) +{ + MOZ_ASSERT(!IsExpired()); + if (IsExpired()) { + return false; + } + + if (mSingleStore != nullptr){ + *h = mSingleStore; + return true; + } + + return mStorage.Get(store, h); +} + +size_t +Histogram::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) +{ + size_t n = 0; + n += aMallocSizeOf(this); + /* + * In theory mStorage.SizeOfExcludingThis should included the data part of the map, + * but the numbers seemed low, so we are only taking the shallow size and do + * the iteration here. + */ + n += mStorage.ShallowSizeOfExcludingThis(aMallocSizeOf); + for (auto iter = mStorage.Iter(); !iter.Done(); iter.Next()) { + auto& h = iter.Data(); + n += h->SizeOfIncludingThis(aMallocSizeOf); + } + if (mSingleStore != nullptr) { + // base::Histogram doesn't have SizeOfExcludingThis, so we are overcounting the pointer here. + n += mSingleStore->SizeOfIncludingThis(aMallocSizeOf); + } + return n; +} + +} // namespace + //////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////// // @@ -986,7 +1184,7 @@ KeyedHistogram::~KeyedHistogram() { for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { base::Histogram* h = iter.Get()->mData; - if (h == gExpiredHistogram) { + if (h == gExpiredBaseHistogram) { continue; } delete h; @@ -1004,7 +1202,7 @@ KeyedHistogram::GetHistogram(const nsCString& key, base::Histogram** histogram) } int bucketsOffset = gHistogramBucketLowerBoundIndex[mId]; - base::Histogram* h = internal_CreateHistogramInstance(mHistogramInfo, bucketsOffset); + base::Histogram* h = internal_CreateBaseHistogramInstance(mHistogramInfo, bucketsOffset); if (!h) { return NS_ERROR_FAILURE; } @@ -1085,7 +1283,7 @@ KeyedHistogram::Clear() for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { base::Histogram* h = iter.Get()->mData; - if (h == gExpiredHistogram) { + if (h == gExpiredBaseHistogram) { continue; } delete h; @@ -1289,9 +1487,9 @@ void internal_Accumulate(const StaticMutexAutoLock& aLock, HistogramID aId, uint return; } - base::Histogram *h = internal_GetHistogramById(aLock, aId, ProcessID::Parent); - MOZ_ASSERT(h); - internal_HistogramAdd(aLock, *h, aId, aSample, ProcessID::Parent); + Histogram *w = internal_GetHistogramById(aLock, aId, ProcessID::Parent); + MOZ_ASSERT(w); + internal_HistogramAdd(aLock, *w, aId, aSample, ProcessID::Parent); } void @@ -1318,10 +1516,11 @@ internal_AccumulateChild(const StaticMutexAutoLock& aLock, return; } - if (base::Histogram* h = internal_GetHistogramById(aLock, aId, aProcessType)) { - internal_HistogramAdd(aLock, *h, aId, aSample, aProcessType); - } else { + Histogram *w = internal_GetHistogramById(aLock, aId, aProcessType); + if (w == nullptr) { NS_WARNING("Failed GetHistogramById for CHILD"); + } else { + internal_HistogramAdd(aLock, *w, aId, aSample, aProcessType); } } @@ -1354,11 +1553,14 @@ internal_ClearHistogram(const StaticMutexAutoLock& aLock, HistogramID id) kh->Clear(); } } - } - - // Now reset the histograms instances for all processes. - for (uint32_t process = 0; process < static_cast(ProcessID::Count); ++process) { - internal_ClearHistogramById(aLock, id, static_cast(process)); + } else { + // Reset the histograms instances for all processes. + for (uint32_t process = 0; process < static_cast(ProcessID::Count); ++process) { + Histogram* h = internal_GetHistogramById(aLock, id, static_cast(process), /* instantiate = */ false); + if (h) { + h->Clear(); + } + } } } @@ -1588,7 +1790,15 @@ internal_JSHistogram_Snapshot(JSContext *cx, unsigned argc, JS::Value *vp) // This is not good standard behavior given that we have histogram instances // covering multiple processes. // However, changing this requires some broader changes to callers. - base::Histogram* h = internal_GetHistogramById(locker, id, ProcessID::Parent); + Histogram* w = internal_GetHistogramById(locker, id, ProcessID::Parent); + base::Histogram *h = nullptr; + NS_NAMED_LITERAL_CSTRING(store, "main"); + if (!w->GetHistogram(store, &h)) { + // When it's not in the 'main' store, let's skip the snapshot completely, + // but don't fail + args.rval().setUndefined(); + return true; + } // Take a snapshot of the data here, protected by the lock, and then, // outside of the lock protection, mirror it to a JS structure if (NS_FAILED(internal_GetHistogramAndSamples(locker, h, dataSnapshot))) { @@ -2051,7 +2261,7 @@ void TelemetryHistogram::InitializeGlobalState(bool canRecordBase, if (XRE_IsParentProcess()) { gHistogramStorage = - new base::Histogram*[HistogramCount * size_t(ProcessID::Count)] {}; + new Histogram*[HistogramCount * size_t(ProcessID::Count)] {}; gKeyedHistogramStorage = new KeyedHistogram*[HistogramCount * size_t(ProcessID::Count)] {}; } @@ -2101,6 +2311,8 @@ void TelemetryHistogram::DeInitializeGlobalState() delete[] gHistogramStorage; delete[] gKeyedHistogramStorage; } + delete gExpiredBaseHistogram; + gExpiredBaseHistogram = nullptr; delete gExpiredHistogram; gExpiredHistogram = nullptr; delete gExpiredKeyedHistogram; @@ -2643,7 +2855,7 @@ TelemetryHistogram::GetHistogramSizesOfIncludingThis(mozilla::MallocSizeOf // If we allocated the array, let's count the number of pointers in there. if (gHistogramStorage) { - n += HistogramCount * size_t(ProcessID::Count) * sizeof(base::Histogram*); + n += HistogramCount * size_t(ProcessID::Count) * sizeof(Histogram*); for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) { if (gHistogramStorage[i] && gHistogramStorage[i] != gExpiredHistogram) { n += gHistogramStorage[i]->SizeOfIncludingThis(aMallocSizeOf); @@ -3062,10 +3274,21 @@ TelemetryHistogram::DeserializeHistograms(JSContext* aCx, JS::HandleValue aData) } // Get the Histogram instance: this will instantiate it if it doesn't exist. - base::Histogram* h = internal_GetHistogramById(locker, id, procID); + Histogram* w = internal_GetHistogramById(locker, id, procID); + MOZ_ASSERT(w); + + if (!w || w->IsExpired()) { + continue; + } + + base::Histogram *h = nullptr; + NS_NAMED_LITERAL_CSTRING(store, "main"); + if (!w->GetHistogram(store, &h)) { + continue; + } MOZ_ASSERT(h); - if (!h || internal_IsExpired(locker, h)) { + if (!h) { // Don't restore expired histograms. continue; } From 74fb630a31c402faf90318e06c76dc0e3ebcc0aa Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 21 Nov 2018 08:34:09 +0000 Subject: [PATCH 07/21] Bug 1498166 - Add multi-storage to keyed histograms r=chutten This extendes the `KeyedHistogram` to keep track of the multiple stores a histogram can be in. Keyed histograms are stored in a hash table, indexed by the name of the store. It has one optimization to support the majority of cases: a single `main` store. For that it stores a direct pointer to a map from keys to the underlying base::Histogram and skips populating the map. This saves an indirection and memory overhead of actually placing it into a second hash table. For now a snapshot only ever returns data from the main store. Clearing a snapshot only clears the main store. Getting the keys of a keyed histogram will only get it from the main store. (This will change in a follow-up) Depends on D11905 Differential Revision: https://phabricator.services.mozilla.com/D11906 --HG-- extra : moz-landing-system : lando --- .../telemetry/core/TelemetryHistogram.cpp | 275 ++++++++++++------ 1 file changed, 181 insertions(+), 94 deletions(-) diff --git a/toolkit/components/telemetry/core/TelemetryHistogram.cpp b/toolkit/components/telemetry/core/TelemetryHistogram.cpp index 2c2f3e628ce3..95ac4c1e84ec 100644 --- a/toolkit/components/telemetry/core/TelemetryHistogram.cpp +++ b/toolkit/components/telemetry/core/TelemetryHistogram.cpp @@ -19,10 +19,8 @@ #include "mozilla/StartupTimeline.h" #include "mozilla/StaticMutex.h" #include "mozilla/Unused.h" -#include "nsBaseHashtable.h" #include "nsClassHashtable.h" #include "nsString.h" -#include "nsTHashtable.h" #include "nsHashKeys.h" #include "nsITelemetry.h" #include "nsPrintfCString.h" @@ -45,7 +43,6 @@ using mozilla::Telemetry::HistogramCount; using mozilla::Telemetry::HistogramIDByNameLookup; using mozilla::Telemetry::Common::LogToBrowserConsole; using mozilla::Telemetry::Common::RecordedProcessType; -using mozilla::Telemetry::Common::AutoHashtable; using mozilla::Telemetry::Common::GetNameForProcessID; using mozilla::Telemetry::Common::GetIDForProcessName; using mozilla::Telemetry::Common::IsExpiredVersion; @@ -235,10 +232,10 @@ class KeyedHistogram { public: KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired); ~KeyedHistogram(); - nsresult GetHistogram(const nsCString& name, base::Histogram** histogram); - base::Histogram* GetHistogram(const nsCString& name); + nsresult GetHistogram(const nsCString& store, const nsCString& key, base::Histogram** histogram); + base::Histogram* GetHistogram(const nsCString& store, const nsCString& key); uint32_t GetHistogramType() const { return mHistogramInfo.histogramType; } - nsresult GetKeys(const StaticMutexAutoLock& aLock, nsTArray& aKeys); + nsresult GetKeys(const StaticMutexAutoLock& aLock, const nsCString& store, nsTArray& aKeys); // Note: unlike other methods, GetJSSnapshot is thread safe. nsresult GetJSSnapshot(JSContext* cx, JS::Handle obj, bool clearSubsession); @@ -250,16 +247,19 @@ public: HistogramID GetHistogramID() const { return mId; } - bool IsEmpty() const { return mHistogramMap.IsEmpty(); } + bool IsEmpty() const; bool IsExpired() const { return mIsExpired; } size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf); private: - typedef nsBaseHashtableET KeyedHistogramEntry; - typedef AutoHashtable KeyedHistogramMapType; - KeyedHistogramMapType mHistogramMap; + typedef nsClassHashtable KeyedHistogramMapType; + typedef nsClassHashtable StoreMapType; + + StoreMapType mStorage; + // A valid map if this histogram belongs to only the main store + KeyedHistogramMapType* mSingleStore; const HistogramID mId; const HistogramInfo& mHistogramInfo; @@ -291,7 +291,6 @@ Histogram** gHistogramStorage; KeyedHistogram** gKeyedHistogramStorage; // To simplify logic below we use a single histogram instance for all expired histograms. -base::Histogram* gExpiredBaseHistogram = nullptr; Histogram* gExpiredHistogram = nullptr; // The single placeholder for expired keyed histograms. @@ -524,12 +523,6 @@ internal_IsEmpty(const StaticMutexAutoLock& aLock, const base::Histogram *h) return h->is_empty(); } -bool -internal_IsExpired(const StaticMutexAutoLock& aLock, base::Histogram* h) -{ - return h == gExpiredBaseHistogram; -} - void internal_SetHistogramRecordingEnabled(const StaticMutexAutoLock& aLock, HistogramID id, @@ -681,26 +674,12 @@ internal_CreateBaseHistogramInstance(const HistogramInfo& passedInfo, int bucket return nullptr; } - // To keep the code simple we map all the calls to expired histograms to the same histogram instance. - // We create that instance lazily when needed. - const bool isExpired = IsExpiredVersion(passedInfo.expiration()); + // We don't actually store data for expired histograms at all. + MOZ_ASSERT(!IsExpiredVersion(passedInfo.expiration())); + HistogramInfo info = passedInfo; const int* buckets = &gHistogramBucketLowerBounds[bucketsOffset]; - if (isExpired) { - if (gExpiredBaseHistogram) { - return gExpiredBaseHistogram; - } - - // The first values in gHistogramBucketLowerBounds are reserved for - // expired histograms. - buckets = gHistogramBucketLowerBounds; - info.min = 1; - info.max = 2; - info.bucketCount = 3; - info.histogramType = nsITelemetry::HISTOGRAM_LINEAR; - } - base::Histogram::Flags flags = base::Histogram::kNoFlags; base::Histogram* h = nullptr; switch (info.histogramType) { @@ -725,10 +704,6 @@ internal_CreateBaseHistogramInstance(const HistogramInfo& passedInfo, int bucket return nullptr; } - if (isExpired) { - gExpiredBaseHistogram = h; - } - return h; } @@ -1173,31 +1148,53 @@ internal_ReflectKeyedHistogram(const KeyedHistogramSnapshotData& aSnapshot, } KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired) - : mHistogramMap() + : mStorage() + , mSingleStore(nullptr) , mId(id) , mHistogramInfo(info) , mIsExpired(expired) { + if (IsExpired()) { + return; + } + + if (info.is_single_store()) { + mSingleStore = new KeyedHistogramMapType; + } else { + for (uint32_t i = 0; i < info.store_count; i++) { + auto store = nsDependentCString(&gHistogramStringTable[gHistogramStoresTable[info.store_index + i]]); + mStorage.Put(store, new KeyedHistogramMapType); + } + } } KeyedHistogram::~KeyedHistogram() { - for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { - base::Histogram* h = iter.Get()->mData; - if (h == gExpiredBaseHistogram) { - continue; - } - delete h; - } - mHistogramMap.Clear(); + delete mSingleStore; } nsresult -KeyedHistogram::GetHistogram(const nsCString& key, base::Histogram** histogram) +KeyedHistogram::GetHistogram(const nsCString& store, const nsCString& key, base::Histogram** histogram) { - KeyedHistogramEntry* entry = mHistogramMap.GetEntry(key); - if (entry) { - *histogram = entry->mData; + if (IsExpired()) { + MOZ_ASSERT(false, "KeyedHistogram::GetHistogram called on an expired histogram."); + return NS_ERROR_FAILURE; + } + + KeyedHistogramMapType* histogramMap; + bool found; + + if (mSingleStore != nullptr) { + histogramMap = mSingleStore; + } else { + found = mStorage.Get(store, &histogramMap); + if (!found) { + return NS_ERROR_FAILURE; + } + } + + found = histogramMap->Get(key, histogram); + if (found) { return NS_OK; } @@ -1210,20 +1207,18 @@ KeyedHistogram::GetHistogram(const nsCString& key, base::Histogram** histogram) h->ClearFlags(base::Histogram::kUmaTargetedHistogramFlag); *histogram = h; - entry = mHistogramMap.PutEntry(key); - if (MOZ_UNLIKELY(!entry)) { + bool inserted = histogramMap->Put(key, h, mozilla::fallible); + if (MOZ_UNLIKELY(!inserted)) { return NS_ERROR_OUT_OF_MEMORY; } - - entry->mData = h; return NS_OK; } base::Histogram* -KeyedHistogram::GetHistogram(const nsCString& key) +KeyedHistogram::GetHistogram(const nsCString& store, const nsCString& key) { base::Histogram* h = nullptr; - if (NS_FAILED(GetHistogram(key, &h))) { + if (NS_FAILED(GetHistogram(store, key, &h))) { return nullptr; } return h; @@ -1233,6 +1228,11 @@ nsresult KeyedHistogram::Add(const nsCString& key, uint32_t sample, ProcessID aProcessType) { + MOZ_ASSERT(XRE_IsParentProcess(), "Only add to keyed histograms in the parent process"); + if (!XRE_IsParentProcess()) { + return NS_ERROR_FAILURE; + } + bool canRecordDataset = CanRecordDataset(mHistogramInfo.dataset, internal_CanRecordBase(), internal_CanRecordExtended()); @@ -1253,12 +1253,6 @@ KeyedHistogram::Add(const nsCString& key, uint32_t sample, return NS_OK; } - base::Histogram* histogram = GetHistogram(key); - MOZ_ASSERT(histogram); - if (!histogram) { - return NS_ERROR_FAILURE; - } - // The internal representation of a base::Histogram's buckets uses `int`. // Clamp large values of `sample` to be INT_MAX so they continue to be treated // as large values (instead of negative ones). @@ -1269,26 +1263,71 @@ KeyedHistogram::Add(const nsCString& key, uint32_t sample, sample = INT_MAX; } - histogram->Add(sample); + base::Histogram* histogram; + if (mSingleStore != nullptr) { + // We know there's only a single store, no need to look for a store + histogram = GetHistogram(VoidCString(), key); + if (!histogram) { + MOZ_ASSERT(false, "Missing histogram in single store."); + return NS_ERROR_FAILURE; + } + + histogram->Add(sample); + } else { + for (uint32_t i = 0; i < mHistogramInfo.store_count; i++) { + auto store = nsDependentCString(&gHistogramStringTable[gHistogramStoresTable[mHistogramInfo.store_index + i]]); + base::Histogram* histogram = GetHistogram(store, key); + MOZ_ASSERT(histogram); + if (histogram) { + histogram->Add(sample); + } else { + return NS_ERROR_FAILURE; + } + } + } + return NS_OK; } void KeyedHistogram::Clear() { - MOZ_ASSERT(XRE_IsParentProcess()); + MOZ_ASSERT(XRE_IsParentProcess(), "Only clear keyed histograms in the parent process"); if (!XRE_IsParentProcess()) { return; } - for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { - base::Histogram* h = iter.Get()->mData; - if (h == gExpiredBaseHistogram) { - continue; - } - delete h; + if (IsExpired()) { + return; } - mHistogramMap.Clear(); + + if (mSingleStore) { + mSingleStore->Clear(); + return; + } + + KeyedHistogramMapType* histogramMap; + bool found = mStorage.Get(NS_LITERAL_CSTRING("main"), &histogramMap); + if (!found) { + return; + } + + histogramMap->Clear(); +} + +bool +KeyedHistogram::IsEmpty() const +{ + if (mSingleStore != nullptr) { + return mSingleStore->IsEmpty(); + } + + KeyedHistogramMapType* histogramMap; + bool found = mStorage.Get(NS_LITERAL_CSTRING("main"), &histogramMap); + if (!found) { + return true; + } + return histogramMap->IsEmpty(); } size_t @@ -1296,19 +1335,42 @@ KeyedHistogram::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) { size_t n = 0; n += aMallocSizeOf(this); - n += mHistogramMap.SizeOfIncludingThis(aMallocSizeOf); + /* + * In theory mStorage.SizeOfExcludingThis should included the data part of the map, + * but the numbers seemed low, so we are only taking the shallow size and do + * the iteration here. + */ + n += mStorage.ShallowSizeOfExcludingThis(aMallocSizeOf); + for (auto iter = mStorage.Iter(); !iter.Done(); iter.Next()) { + auto& h = iter.Data(); + n += h->SizeOfIncludingThis(aMallocSizeOf); + } + if (mSingleStore != nullptr) { + // base::Histogram doesn't have SizeOfExcludingThis, so we are overcounting the pointer here. + n += mSingleStore->SizeOfExcludingThis(aMallocSizeOf); + } return n; } nsresult -KeyedHistogram::GetKeys(const StaticMutexAutoLock& aLock, nsTArray& aKeys) +KeyedHistogram::GetKeys(const StaticMutexAutoLock& aLock, const nsCString& store, nsTArray& aKeys) { - if (!aKeys.SetCapacity(mHistogramMap.Count(), mozilla::fallible)) { + KeyedHistogramMapType* histogramMap; + if (mSingleStore != nullptr) { + histogramMap = mSingleStore; + } else { + bool found = mStorage.Get(store, &histogramMap); + if (!found) { + return NS_ERROR_FAILURE; + } + } + + if (!aKeys.SetCapacity(histogramMap->Count(), mozilla::fallible)) { return NS_ERROR_OUT_OF_MEMORY; } - for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) { - if (!aKeys.AppendElement(iter.Get()->GetKey(), mozilla::fallible)) { + for (auto iter = histogramMap->Iter(); !iter.Done(); iter.Next()) { + if (!aKeys.AppendElement(iter.Key(), mozilla::fallible)) { return NS_ERROR_OUT_OF_MEMORY; } } @@ -1327,8 +1389,9 @@ KeyedHistogram::GetJSSnapshot(JSContext* cx, JS::Handle obj, bool cle // Take a snapshot of the data here, protected by the lock, and then, // outside of the lock protection, mirror it to a JS structure. - if (NS_FAILED(GetSnapshot(locker, dataSnapshot, clearSubsession))) { - return NS_ERROR_FAILURE; + nsresult rv = GetSnapshot(locker, dataSnapshot, clearSubsession); + if (NS_FAILED(rv)) { + return rv; } } @@ -1336,13 +1399,31 @@ KeyedHistogram::GetJSSnapshot(JSContext* cx, JS::Handle obj, bool cle return internal_ReflectKeyedHistogram(dataSnapshot, gHistogramInfos[mId], cx, obj); } +/** + * Return a histogram snapshot for the main store. + * + * If getting the snapshot succeeds, NS_OK is returned and `aSnapshot` contains the snapshot data. + * If the histogram is not available in the main store, NS_ERROR_NO_CONTENT is returned. + * For other errors, NS_ERROR_FAILURE is returned. + */ nsresult KeyedHistogram::GetSnapshot(const StaticMutexAutoLock& aLock, KeyedHistogramSnapshotData& aSnapshot, bool aClearSubsession) { + KeyedHistogramMapType* histogramMap; + if (mSingleStore != nullptr) { + histogramMap = mSingleStore; + } else { + bool found = mStorage.Get(NS_LITERAL_CSTRING("main"), &histogramMap); + if (!found) { + // Nothing in the main store is fine, it's just handled as empty + return NS_ERROR_NO_CONTENT; + } + } + // Snapshot every key. - for (auto iter = mHistogramMap.ConstIter(); !iter.Done(); iter.Next()) { - base::Histogram* keyData = iter.Get()->mData; + for (auto iter = histogramMap->ConstIter(); !iter.Done(); iter.Next()) { + base::Histogram* keyData = iter.Data(); if (!keyData) { return NS_ERROR_FAILURE; } @@ -1353,7 +1434,7 @@ KeyedHistogram::GetSnapshot(const StaticMutexAutoLock& aLock, } // Append to the final snapshot. - aSnapshot.Put(iter.Get()->GetKey(), std::move(keySnapshot)); + aSnapshot.Put(iter.Key(), std::move(keySnapshot)); } if (aClearSubsession) { @@ -1973,7 +2054,15 @@ internal_KeyedHistogram_SnapshotImpl(JSContext *cx, unsigned argc, return false; } - if (!NS_SUCCEEDED(keyed->GetJSSnapshot(cx, snapshot, clearSubsession))) { + nsresult rv = keyed->GetJSSnapshot(cx, snapshot, clearSubsession); + + // If the main store is not available, we return nothing and don't fail + if (rv == NS_ERROR_NO_CONTENT) { + args.rval().setUndefined(); + return true; + } + + if (!NS_SUCCEEDED(rv)) { JS_ReportErrorASCII(cx, "Failed to reflect keyed histograms"); return false; } @@ -1996,7 +2085,7 @@ internal_KeyedHistogram_SnapshotImpl(JSContext *cx, unsigned argc, // Get data for the key we're looking for. base::Histogram* h = nullptr; - nsresult rv = keyed->GetHistogram(NS_ConvertUTF16toUTF8(key), &h); + nsresult rv = keyed->GetHistogram(NS_LITERAL_CSTRING("main"), NS_ConvertUTF16toUTF8(key), &h); if (NS_FAILED(rv)) { return false; } @@ -2117,7 +2206,7 @@ internal_JSKeyedHistogram_Keys(JSContext *cx, unsigned argc, JS::Value *vp) return false; } - if (NS_FAILED(keyed->GetKeys(locker, keys))) { + if (NS_FAILED(keyed->GetKeys(locker, NS_LITERAL_CSTRING("main"), keys))) { return false; } } @@ -2300,8 +2389,7 @@ void TelemetryHistogram::DeInitializeGlobalState() // FactoryGet `new`s Histograms for us, but requires us to manually delete. if (XRE_IsParentProcess()) { for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) { - if (i < HistogramCount * size_t(ProcessID::Count) - && gKeyedHistogramStorage[i] != gExpiredKeyedHistogram) { + if (gKeyedHistogramStorage[i] != gExpiredKeyedHistogram) { delete gKeyedHistogramStorage[i]; } if (gHistogramStorage[i] != gExpiredHistogram) { @@ -2311,8 +2399,6 @@ void TelemetryHistogram::DeInitializeGlobalState() delete[] gHistogramStorage; delete[] gKeyedHistogramStorage; } - delete gExpiredBaseHistogram; - gExpiredBaseHistogram = nullptr; delete gExpiredHistogram; gExpiredHistogram = nullptr; delete gExpiredKeyedHistogram; @@ -3470,20 +3556,21 @@ TelemetryHistogram::DeserializeKeyedHistograms(JSContext* aCx, JS::HandleValue a KeyedHistogram* keyed = internal_GetKeyedHistogramById(id, procID); MOZ_ASSERT(keyed); - if (!keyed) { - // Don't restore if we don't have a destination storage. + if (!keyed || keyed->IsExpired()) { + // Don't restore if we don't have a destination storage or the + // histogram is expired. continue; } // Get data for the key we're looking for. base::Histogram* h = nullptr; - if (NS_FAILED(keyed->GetHistogram(mozilla::Get<1>(histogramData), &h))) { + if (NS_FAILED(keyed->GetHistogram(NS_LITERAL_CSTRING("main"), mozilla::Get<1>(histogramData), &h))) { continue; } MOZ_ASSERT(h); - if (!h || internal_IsExpired(locker, h)) { - // Don't restore expired histograms. + if (!h) { + // Don't restore if we don't have a destination storage. continue; } From 3d2eb62eb7f5ba5370327afe6382febb4a7d5c54 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 19 Nov 2018 13:01:05 +0000 Subject: [PATCH 08/21] Bug 1498166 - Test storing into multiple stores (and snapshotting main store) r=chutten For now we test that storing still works and the main store is still accessed for snapshots and clearing. Depends on D11906 Differential Revision: https://phabricator.services.mozilla.com/D11907 --HG-- extra : moz-landing-system : lando --- toolkit/components/telemetry/Histograms.json | 26 ++++ .../tests/unit/test_TelemetryHistograms.js | 134 ++++++++++++++++++ 2 files changed, 160 insertions(+) diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index e5693dc2441f..693ca1687f8f 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -7801,6 +7801,32 @@ "description": "a testing histogram; not meant to be touched", "record_into_store": ["main", "sync"] }, + "TELEMETRY_TEST_KEYED_SYNC_ONLY": { + "record_in_processes": ["main"], + "alert_emails": ["telemetry-client-dev@mozilla.com"], + "expires_in_version": "never", + "kind": "linear", + "keyed": true, + "low": 1, + "high": 2147483646, + "n_buckets": 10, + "bug_numbers": [1498164], + "description": "a testing histogram; not meant to be touched", + "record_into_store": ["sync"] + }, + "TELEMETRY_TEST_KEYED_MULTIPLE_STORES": { + "record_in_processes": ["main"], + "alert_emails": ["telemetry-client-dev@mozilla.com"], + "expires_in_version": "never", + "kind": "linear", + "keyed": true, + "low": 1, + "high": 2147483646, + "n_buckets": 10, + "bug_numbers": [1498164], + "description": "a testing histogram; not meant to be touched", + "record_into_store": ["main", "sync"] + }, "STARTUP_CRASH_DETECTED": { "record_in_processes": ["main", "content"], "expires_in_version": "never", diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js index 3a1bae3b13e8..f6fbf29c822b 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js @@ -1183,3 +1183,137 @@ add_task(async function test_valid_os_smoketest() { Assert.ok(existingProbe in snapshot, `${existingProbe} should be recorded on ${AppConstants.platform}`); Assert.equal(snapshot[existingProbe].sum, 1); }); + +add_task(async function test_multistore_individual_histogram() { + Telemetry.canRecordExtended = true; + + let id; + let hist; + let snapshot; + + id = "TELEMETRY_TEST_MAIN_ONLY"; + hist = Telemetry.getHistogramById(id); + snapshot = hist.snapshot(); + Assert.equal(0, snapshot.sum, `Histogram ${id} should be empty.`); + hist.add(1); + snapshot = hist.snapshot(); + Assert.equal(1, snapshot.sum, `Histogram ${id} should have recorded one value.`); + hist.clear(); + snapshot = hist.snapshot(); + Assert.equal(0, snapshot.sum, `Histogram ${id} should be cleared.`); + + id = "TELEMETRY_TEST_MULTIPLE_STORES"; + hist = Telemetry.getHistogramById(id); + snapshot = hist.snapshot(); + Assert.equal(0, snapshot.sum, `Histogram ${id} should be empty.`); + hist.add(1); + snapshot = hist.snapshot(); + Assert.equal(1, snapshot.sum, `Histogram ${id} should have recorded one value.`); + hist.clear(); + snapshot = hist.snapshot(); + Assert.equal(0, snapshot.sum, `Histogram ${id} should be cleared.`); + + // When sync only, then the snapshot will be empty on the main store + id = "TELEMETRY_TEST_SYNC_ONLY"; + hist = Telemetry.getHistogramById(id); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); + hist.add(1); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); + hist.clear(); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); + + id = "TELEMETRY_TEST_KEYED_MULTIPLE_STORES"; + hist = Telemetry.getKeyedHistogramById(id); + snapshot = hist.snapshot(); + Assert.deepEqual({}, snapshot, `Histogram ${id} should be empty.`); + hist.add("key-a", 1); + snapshot = hist.snapshot(); + Assert.equal(1, snapshot["key-a"].sum, `Histogram ${id} should have recorded one value.`); + hist.clear(); + snapshot = hist.snapshot(); + Assert.deepEqual({}, snapshot, `Histogram ${id} should be cleared.`); + + // When sync only, then the snapshot will be empty on the main store + id = "TELEMETRY_TEST_KEYED_SYNC_ONLY"; + hist = Telemetry.getKeyedHistogramById(id); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); + hist.add("key-a", 1); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); + hist.clear(); + snapshot = hist.snapshot(); + Assert.equal(undefined, snapshot, `Histogram ${id} should not be in the 'main' storage`); +}); + +add_task(async function test_multistore_main_snapshot() { + Telemetry.canRecordExtended = true; + // Clear histograms + Telemetry.getSnapshotForHistograms("main", true); + Telemetry.getSnapshotForKeyedHistograms("main", true); + + let id; + let hist; + let snapshot; + + // Plain histograms + + // Fill with data + id = "TELEMETRY_TEST_MAIN_ONLY"; + hist = Telemetry.getHistogramById(id); + hist.add(1); + + id = "TELEMETRY_TEST_MULTIPLE_STORES"; + hist = Telemetry.getHistogramById(id); + hist.add(1); + + id = "TELEMETRY_TEST_SYNC_ONLY"; + hist = Telemetry.getHistogramById(id); + hist.add(1); + + // Getting snapshot and clearing + snapshot = Telemetry.getSnapshotForHistograms("main", /* clear */ true).parent; + id = "TELEMETRY_TEST_MAIN_ONLY"; + Assert.ok(id in snapshot, `${id} should be in a main store snapshot`); + id = "TELEMETRY_TEST_MULTIPLE_STORES"; + Assert.ok(id in snapshot, `${id} should be in a main store snapshot`); + id = "TELEMETRY_TEST_SYNC_ONLY"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + + // Should be empty after clearing + snapshot = Telemetry.getSnapshotForHistograms("main", /* clear */ false).parent; + id = "TELEMETRY_TEST_MAIN_ONLY"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + id = "TELEMETRY_TEST_MULTIPLE_STORES"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + id = "TELEMETRY_TEST_SYNC_ONLY"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + + // Keyed histograms + + // Fill with data + id = "TELEMETRY_TEST_KEYED_MULTIPLE_STORES"; + hist = Telemetry.getKeyedHistogramById(id); + hist.add("key-a", 1); + + id = "TELEMETRY_TEST_KEYED_SYNC_ONLY"; + hist = Telemetry.getKeyedHistogramById(id); + hist.add("key-b", 1); + + // Getting snapshot and clearing + snapshot = Telemetry.getSnapshotForKeyedHistograms("main", /* clear */ true).parent; + id = "TELEMETRY_TEST_KEYED_MULTIPLE_STORES"; + Assert.ok(id in snapshot, `${id} should be in a main store snapshot`); + id = "TELEMETRY_TEST_KEYED_SYNC_ONLY"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + + // Should be empty after clearing + snapshot = Telemetry.getSnapshotForKeyedHistograms("main", /* clear */ false).parent; + id = "TELEMETRY_TEST_KEYED_MULTIPLE_STORES"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); + id = "TELEMETRY_TEST_KEYED_SYNC_ONLY"; + Assert.ok(!(id in snapshot), `${id} should not be in a main store snapshot`); +}); From c88c8173d9247ac96d65457a5cd7777501434c44 Mon Sep 17 00:00:00 2001 From: Razvan Caliman Date: Tue, 20 Nov 2018 16:36:01 +0000 Subject: [PATCH 09/21] Bug 1507476 - Match exact tracked declaration when renaming a property. r=pbro Ensure the exact declaration is matched when aggregating changes and attempting to remove declarations which cancel each other out. Without checking both the index and the property name, we used to lose tracked declarations that were renamed. The test checks for the expected rename behaviour and that renaming the declaration to its original clears any tracked changes. Differential Revision: https://phabricator.services.mozilla.com/D12434 --HG-- extra : moz-landing-system : lando --- .../inspector/changes/reducers/changes.js | 4 +- .../client/inspector/changes/test/browser.ini | 1 + .../browser_changes_declaration_rename.js | 61 +++++++++++++++++++ .../client/inspector/changes/test/head.js | 31 ++++++++++ devtools/client/inspector/rules/test/head.js | 28 +++++++++ 5 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 devtools/client/inspector/changes/test/browser_changes_declaration_rename.js diff --git a/devtools/client/inspector/changes/reducers/changes.js b/devtools/client/inspector/changes/reducers/changes.js index 44d22152406b..c68a325638b5 100644 --- a/devtools/client/inspector/changes/reducers/changes.js +++ b/devtools/client/inspector/changes/reducers/changes.js @@ -266,7 +266,9 @@ const reducers = { return addDecl.index === decl.index; }); - if (rule.remove[removeIndex] && rule.remove[removeIndex].value === decl.value) { + if (rule.remove[removeIndex] && + rule.remove[removeIndex].value === decl.value && + rule.remove[removeIndex].property === decl.property) { // Delete any previous remove operation which would be canceled out by this add. rule.remove.splice(removeIndex, 1); } else if (rule.add[addIndex]) { diff --git a/devtools/client/inspector/changes/test/browser.ini b/devtools/client/inspector/changes/test/browser.ini index 97341e060265..cb8536b75541 100644 --- a/devtools/client/inspector/changes/test/browser.ini +++ b/devtools/client/inspector/changes/test/browser.ini @@ -17,4 +17,5 @@ support-files = [browser_changes_declaration_edit_value.js] [browser_changes_declaration_remove_ahead.js] [browser_changes_declaration_remove.js] +[browser_changes_declaration_rename.js] [browser_changes_rule_selector.js] diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_rename.js b/devtools/client/inspector/changes/test/browser_changes_declaration_rename.js new file mode 100644 index 000000000000..64603c36fe43 --- /dev/null +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_rename.js @@ -0,0 +1,61 @@ +/* vim: set ts=2 et sw=2 tw=80: */ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +// Test that renaming the property of a CSS declaration in the Rule view is tracked. + +const TEST_URI = ` + +
+`; + +add_task(async function() { + await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); + const { inspector, view: ruleView } = await openRuleView(); + const { document: doc, store } = selectChangesView(inspector); + + await selectNode("div", inspector); + const rule = getRuleViewRuleEditor(ruleView, 1).rule; + const prop = rule.textProps[0]; + + let onTrackChange; + + const oldPropertyName = "color"; + const newPropertyName = "background-color"; + + info(`Rename the CSS declaration name to ${newPropertyName}`); + onTrackChange = waitUntilAction(store, "TRACK_CHANGE"); + await renameProperty(ruleView, prop, newPropertyName); + info("Wait for the change to be tracked"); + await onTrackChange; + + let removeDecl = getRemovedDeclarations(doc); + let addDecl = getAddedDeclarations(doc); + + is(removeDecl.length, 1, "One declaration tracked as removed"); + is(removeDecl[0].property, oldPropertyName, + `Removed declaration has old property name: ${oldPropertyName}` + ); + is(addDecl.length, 1, "One declaration tracked as added"); + is(addDecl[0].property, newPropertyName, + `Added declaration has new property name: ${newPropertyName}` + ); + + info(`Reverting the CSS declaration name to ${oldPropertyName} should clear changes.`); + onTrackChange = waitUntilAction(store, "TRACK_CHANGE"); + await renameProperty(ruleView, prop, oldPropertyName); + info("Wait for the change to be tracked"); + await onTrackChange; + + removeDecl = getRemovedDeclarations(doc); + addDecl = getAddedDeclarations(doc); + + is(removeDecl.length, 0, "No declaration tracked as removed"); + is(addDecl.length, 0, "No declaration tracked as added"); +}); diff --git a/devtools/client/inspector/changes/test/head.js b/devtools/client/inspector/changes/test/head.js index 00b791f6d612..49fca8d669ed 100644 --- a/devtools/client/inspector/changes/test/head.js +++ b/devtools/client/inspector/changes/test/head.js @@ -29,3 +29,34 @@ registerCleanupFunction(() => { Services.prefs.clearUserPref("devtools.inspector.changes.enabled"); Services.prefs.clearUserPref("devtools.inspector.three-pane-enabled"); }); + +/** + * Get an array of objects with property/value pairs of the CSS declarations rendered + * in the Changes panel. + * + * @param {Document} panelDoc + * Host document of the Changes panel. + * @param {String} selector + * Optional selector to filter rendered declaration DOM elements. + * One of ".diff-remove" or ".diff-add". + * If omitted, all declarations will be returned. + * @return {Array} + */ +function getDeclarations(panelDoc, selector = "") { + const els = panelDoc.querySelectorAll(`#sidebar-panel-changes .declaration${selector}`); + + return [...els].map(el => { + return { + property: el.querySelector(".declaration-name").textContent, + value: el.querySelector(".declaration-value").textContent, + }; + }); +} + +function getAddedDeclarations(panelDoc) { + return getDeclarations(panelDoc, ".diff-add"); +} + +function getRemovedDeclarations(panelDoc) { + return getDeclarations(panelDoc, ".diff-remove"); +} diff --git a/devtools/client/inspector/rules/test/head.js b/devtools/client/inspector/rules/test/head.js index 1c21cea27edb..d86003cc5d01 100644 --- a/devtools/client/inspector/rules/test/head.js +++ b/devtools/client/inspector/rules/test/head.js @@ -366,6 +366,34 @@ var setProperty = async function(view, textProp, value, } }; +/** + * Change the name of a property in a rule in the rule-view. + * + * @param {CssRuleView} view + * The instance of the rule-view panel. + * @param {TextProperty} textProp + * The instance of the TextProperty to be changed. + * @param {String} name + * The new property name. + */ +var renameProperty = async function(view, textProp, name) { + await focusEditableField(view, textProp.editor.nameSpan); + + const onNameDone = view.once("ruleview-changed"); + info(`Rename the property to ${name}`); + EventUtils.sendString(name, view.styleWindow); + EventUtils.synthesizeKey("VK_RETURN", {}, view.styleWindow); + info("Wait for property name."); + await onNameDone; + // Renaming the property auto-advances the focus to the value input. Exiting without + // committing will still fire a change event. @see TextPropertyEditor._onValueDone(). + // Wait for that event too before proceeding. + const onValueDone = view.once("ruleview-changed"); + EventUtils.synthesizeKey("VK_ESCAPE", {}, view.styleWindow); + info("Wait for property value."); + await onValueDone; +}; + /** * Simulate removing a property from an existing rule in the rule-view. * From 12acae9b30e1b12fb44cdd6b156daf2f4ca5bbd2 Mon Sep 17 00:00:00 2001 From: Razvan Caliman Date: Wed, 21 Nov 2018 08:16:06 +0000 Subject: [PATCH 10/21] Bug 1507476 - Update Changes panel tests to use helpers for rendered declarations. r=pbro Depends on D12434 Differential Revision: https://phabricator.services.mozilla.com/D12445 --HG-- extra : moz-landing-system : lando --- .../browser_changes_declaration_disable.js | 7 ++-- .../browser_changes_declaration_duplicate.js | 36 ++++++++----------- .../browser_changes_declaration_edit_value.js | 22 ++++++------ .../browser_changes_declaration_remove.js | 9 +++-- ...rowser_changes_declaration_remove_ahead.js | 13 +++---- 5 files changed, 36 insertions(+), 51 deletions(-) diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_disable.js b/devtools/client/inspector/changes/test/browser_changes_declaration_disable.js index 34ea56854adc..5ad99cff91f5 100644 --- a/devtools/client/inspector/changes/test/browser_changes_declaration_disable.js +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_disable.js @@ -19,7 +19,6 @@ add_task(async function() { await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); const { inspector, view: ruleView } = await openRuleView(); const { document: doc, store } = selectChangesView(inspector); - const panel = doc.querySelector("#sidebar-panel-changes"); await selectNode("div", inspector); const rule = getRuleViewRuleEditor(ruleView, 1).rule; @@ -31,7 +30,7 @@ add_task(async function() { info("Wait for change to be tracked"); await onTrackChange; - let removedDeclarations = panel.querySelectorAll(".diff-remove"); + let removedDeclarations = getRemovedDeclarations(doc); is(removedDeclarations.length, 1, "Only one declaration was tracked as removed"); onTrackChange = waitUntilAction(store, "TRACK_CHANGE"); @@ -40,8 +39,8 @@ add_task(async function() { info("Wait for change to be tracked"); await onTrackChange; - const addedDeclarations = panel.querySelectorAll(".diff-add"); - removedDeclarations = panel.querySelectorAll(".diff-remove"); + const addedDeclarations = getAddedDeclarations(doc); + removedDeclarations = getRemovedDeclarations(doc); is(addedDeclarations.length, 0, "No declarations were tracked as added"); is(removedDeclarations.length, 0, "No declarations were tracked as removed"); }); diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_duplicate.js b/devtools/client/inspector/changes/test/browser_changes_declaration_duplicate.js index 9ba040aa4519..34defb071074 100644 --- a/devtools/client/inspector/changes/test/browser_changes_declaration_duplicate.js +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_duplicate.js @@ -18,15 +18,14 @@ add_task(async function() { await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); const { inspector, view: ruleView } = await openRuleView(); const { document: doc, store } = selectChangesView(inspector); - const panel = doc.querySelector("#sidebar-panel-changes"); await selectNode("div", inspector); - await testAddDuplicateDeclarations(ruleView, store, panel); - await testChangeDuplicateDeclarations(ruleView, store, panel); - await testRemoveDuplicateDeclarations(ruleView, store, panel); + await testAddDuplicateDeclarations(ruleView, store, doc); + await testChangeDuplicateDeclarations(ruleView, store, doc); + await testRemoveDuplicateDeclarations(ruleView, store, doc); }); -async function testAddDuplicateDeclarations(ruleView, store, panel) { +async function testAddDuplicateDeclarations(ruleView, store, doc) { info(`Test that adding declarations with the same property name and value are both tracked.`); @@ -42,18 +41,17 @@ async function testAddDuplicateDeclarations(ruleView, store, panel) { info("Wait for the change to be tracked"); await onTrackChange; - const addDecl = panel.querySelectorAll(".declaration.diff-add"); + const addDecl = getAddedDeclarations(doc); is(addDecl.length, 2, "Two declarations were tracked as added"); - is(addDecl.item(0).querySelector(".declaration-value").textContent, "red", + is(addDecl[0].value, "red", "First declaration has correct property value" ); - is(addDecl.item(0).querySelector(".declaration-value").textContent, - addDecl.item(1).querySelector(".declaration-value").textContent, + is(addDecl[0].value, addDecl[1].value, "First and second declarations have identical property values" ); } -async function testChangeDuplicateDeclarations(ruleView, store, panel) { +async function testChangeDuplicateDeclarations(ruleView, store, doc) { info("Test that changing one of the duplicate declarations won't change the other"); const rule = getRuleViewRuleEditor(ruleView, 1).rule; const prop = rule.textProps[0]; @@ -64,16 +62,12 @@ async function testChangeDuplicateDeclarations(ruleView, store, panel) { info("Wait for the change to be tracked"); await onTrackChange; - const addDecl = panel.querySelectorAll(".declaration.diff-add"); - is(addDecl.item(0).querySelector(".declaration-value").textContent, "black", - "First declaration has changed property value" - ); - is(addDecl.item(1).querySelector(".declaration-value").textContent, "red", - "Second declaration has not changed property value" - ); + const addDecl = getAddedDeclarations(doc); + is(addDecl[0].value, "black", "First declaration has changed property value"); + is(addDecl[1].value, "red", "Second declaration has not changed property value"); } -async function testRemoveDuplicateDeclarations(ruleView, store, panel) { +async function testRemoveDuplicateDeclarations(ruleView, store, doc) { info(`Test that removing the first of the duplicate declarations will not remove the second.`); const rule = getRuleViewRuleEditor(ruleView, 1).rule; @@ -85,12 +79,12 @@ async function testRemoveDuplicateDeclarations(ruleView, store, panel) { info("Wait for the change to be tracked"); await onTrackChange; - const addDecl = panel.querySelectorAll(".declaration.diff-add"); - const removeDecl = panel.querySelectorAll(".declaration.diff-remove"); + const addDecl = getAddedDeclarations(doc); + const removeDecl = getRemovedDeclarations(doc); // Expect no remove operation tracked because it cancels out the original add operation. is(removeDecl.length, 0, "No declaration was tracked as removed"); is(addDecl.length, 1, "Just one declaration left tracked as added"); - is(addDecl.item(0).querySelector(".declaration-value").textContent, "red", + is(addDecl[0].value, "red", "Leftover declaration has property value of the former second declaration" ); } diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_edit_value.js b/devtools/client/inspector/changes/test/browser_changes_declaration_edit_value.js index 13ebdf8e2e1b..86b71adc6638 100644 --- a/devtools/client/inspector/changes/test/browser_changes_declaration_edit_value.js +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_edit_value.js @@ -53,15 +53,14 @@ add_task(async function() { await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); const { inspector, view: ruleView } = await openRuleView(); const { document: doc, store } = selectChangesView(inspector); - const panel = doc.querySelector("#sidebar-panel-changes"); await selectNode("div", inspector); const rule = getRuleViewRuleEditor(ruleView, 1).rule; const prop = rule.textProps[0]; let onTrackChange; - let removeValue; - let addValue; + let removeDecl; + let addDecl; for (const { value, add, remove } of VALUE_CHANGE_ITERATIONS) { onTrackChange = waitUntilAction(store, "TRACK_CHANGE"); @@ -71,25 +70,24 @@ add_task(async function() { info("Wait for the change to be tracked"); await onTrackChange; - addValue = panel.querySelector(".diff-add .declaration-value"); - removeValue = panel.querySelector(".diff-remove .declaration-value"); + addDecl = getAddedDeclarations(doc); + removeDecl = getRemovedDeclarations(doc); if (add) { - is(addValue.textContent, add.value, + is(addDecl[0].value, add.value, `Added declaration has expected value: ${add.value}`); - is(panel.querySelectorAll(".diff-add").length, 1, - "Only one declaration was tracked as added."); + is(addDecl.length, 1, "Only one declaration was tracked as added."); } else { - ok(!addValue, `Added declaration was cleared`); + is(addDecl.length, 0, "Added declaration was cleared"); } if (remove) { - is(removeValue.textContent, remove.value, + is(removeDecl[0].value, remove.value, `Removed declaration has expected value: ${remove.value}`); - is(panel.querySelectorAll(".diff-remove").length, 1, + is(removeDecl.length, 1, "Only one declaration was tracked as removed."); } else { - ok(!removeValue, `Removed declaration was cleared`); + is(removeDecl.length, 0, "Removed declaration was cleared"); } } }); diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_remove.js b/devtools/client/inspector/changes/test/browser_changes_declaration_remove.js index cd87ca852d36..c7bba7986dec 100644 --- a/devtools/client/inspector/changes/test/browser_changes_declaration_remove.js +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_remove.js @@ -19,7 +19,6 @@ add_task(async function() { await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); const { inspector, view: ruleView } = await openRuleView(); const { document: doc, store } = selectChangesView(inspector); - const panel = doc.querySelector("#sidebar-panel-changes"); await selectNode("div", inspector); const rule = getRuleViewRuleEditor(ruleView, 1).rule; @@ -31,8 +30,8 @@ add_task(async function() { info("Wait for change to be tracked"); await onTrackChange; - const removeName = panel.querySelector(".diff-remove .declaration-name"); - const removeValue = panel.querySelector(".diff-remove .declaration-value"); - is(removeName.textContent, "color", "Correct declaration name was tracked as removed"); - is(removeValue.textContent, "red", "Correct declaration value was tracked as removed"); + const removeDecl = getRemovedDeclarations(doc); + is(removeDecl.length, 1, "One declaration was tracked as removed"); + is(removeDecl[0].property, "color", "Correct declaration name was tracked as removed"); + is(removeDecl[0].value, "red", "Correct declaration value was tracked as removed"); }); diff --git a/devtools/client/inspector/changes/test/browser_changes_declaration_remove_ahead.js b/devtools/client/inspector/changes/test/browser_changes_declaration_remove_ahead.js index ff9554cef768..715538319d80 100644 --- a/devtools/client/inspector/changes/test/browser_changes_declaration_remove_ahead.js +++ b/devtools/client/inspector/changes/test/browser_changes_declaration_remove_ahead.js @@ -21,7 +21,6 @@ add_task(async function() { await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI)); const { inspector, view: ruleView } = await openRuleView(); const { document: doc, store } = selectChangesView(inspector); - const panel = doc.querySelector("#sidebar-panel-changes"); await selectNode("div", inspector); const rule = getRuleViewRuleEditor(ruleView, 1).rule; @@ -44,16 +43,12 @@ add_task(async function() { info("Wait for change to be tracked"); await onTrackChange; - const removeDecl = panel.querySelectorAll(".declaration.diff-remove"); - const addDecl = panel.querySelectorAll(".declaration.diff-add"); + const removeDecl = getRemovedDeclarations(doc); + const addDecl = getAddedDeclarations(doc); is(removeDecl.length, 2, "Two declarations tracked as removed"); is(addDecl.length, 1, "One declaration tracked as added"); // Ensure changes to the second declaration were tracked after removing the first one. - is(addDecl.item(0).querySelector(".declaration-name").textContent, "display", - "Added declaration has updated property name" - ); - is(addDecl.item(0).querySelector(".declaration-value").textContent, "flex", - "Added declaration has updated property value" - ); + is(addDecl[0].property, "display", "Added declaration has updated property name"); + is(addDecl[0].value, "flex", "Added declaration has updated property value"); }); From 00d2226d66524bbad675f6aee8b0002ac09a4afc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A3o=20Gottwald?= Date: Wed, 21 Nov 2018 11:11:22 +0000 Subject: [PATCH 11/21] Bug 1507109 - Fix popup notification footer button colors. r=ntim Differential Revision: https://phabricator.services.mozilla.com/D11867 --HG-- extra : moz-landing-system : lando --- toolkit/themes/shared/popupnotification.inc.css | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/toolkit/themes/shared/popupnotification.inc.css b/toolkit/themes/shared/popupnotification.inc.css index c63ae202d221..8d5e2c6b0899 100644 --- a/toolkit/themes/shared/popupnotification.inc.css +++ b/toolkit/themes/shared/popupnotification.inc.css @@ -60,22 +60,21 @@ .popup-notification-button { flex: 1; -moz-appearance: none; - background-color: transparent; color: inherit; margin: 0; - padding: 0; min-width: 0; min-height: 41px; - border: none; border-top: 1px solid var(--panel-separator-color); } .popup-notification-button:hover:not([disabled]) { background-color: var(--arrowpanel-dimmed); + color: inherit; /* override button.css on Linux */ } .popup-notification-button:hover:active:not([disabled]) { background-color: var(--arrowpanel-dimmed-further); + color: inherit; /* override button.css on Mac */ box-shadow: 0 1px 0 hsla(210,4%,10%,.05) inset; } From 5eb9ef226716667254a339cd4d4698fabacd9cf3 Mon Sep 17 00:00:00 2001 From: Imanol Fernandez Date: Wed, 21 Nov 2018 10:34:52 +0000 Subject: [PATCH 12/21] Bug 1499758 - Fix potential null pointer deref in WebGLContext::GetVRFrame r=jgilbert Differential Revision: https://phabricator.services.mozilla.com/D12480 --HG-- extra : moz-landing-system : lando --- dom/canvas/WebGLContext.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dom/canvas/WebGLContext.cpp b/dom/canvas/WebGLContext.cpp index e9f5fbea0ba7..18fe9ea35596 100644 --- a/dom/canvas/WebGLContext.cpp +++ b/dom/canvas/WebGLContext.cpp @@ -2328,6 +2328,9 @@ WebGLContext::GetVRFrame() already_AddRefed WebGLContext::GetVRFrame() { + if (!gl) + return nullptr; + EnsureVRReady(); /** * Swap buffers as though composition has occurred. @@ -2337,9 +2340,6 @@ WebGLContext::GetVRFrame() BeginComposition(); EndComposition(); - if (!gl) - return nullptr; - gl::GLScreenBuffer* screen = gl->Screen(); if (!screen) return nullptr; From 388e64d85789b653999966f7cc8ff8dbd47f1457 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Tue, 20 Nov 2018 14:24:06 +0000 Subject: [PATCH 13/21] Bug 1504911 - part 0: Add "input" event tests into existing tests r=smaug It's difficult to create new test which checks "input" events caused by all edit operations especially when text is inserted from our UI. Therefore, this adds "input" event type checks into existing tests. Additionally, this adds new test for MozEditableElement.setUserInput() whose behavior needs to be fixed in this bug. Currently, InputEvent interface should be used only on text controls or contenteditable editor when dispatching "input" event. https://w3c.github.io/input-events/#events-inputevents You may feel odd to use different event interface for same "input" events. However, other browsers also use InputEvent interface only in the cases. So, we should follow them for now. Differential Revision: https://phabricator.services.mozilla.com/D12243 --HG-- extra : moz-landing-system : lando --- .../test/mochitest/formautofill_common.js | 17 +- .../test/mochitest/test_clear_form.html | 10 +- .../test_multi_locale_CA_address_form.html | 15 +- .../chrome/window_nsITextInputProcessor.xul | 30 ++ dom/html/test/forms/mochitest.ini | 1 + .../test_MozEditableElement_setUserInput.html | 244 +++++++++++ dom/html/test/forms/test_input_event.html | 94 ++++- .../forms/test_input_number_mouse_events.html | 12 + ...t_abs_positioner_positioning_elements.html | 21 + .../test_dom_input_event_on_htmleditor.html | 2 + .../test_dom_input_event_on_texteditor.html | 2 + editor/libeditor/tests/test_dragdrop.html | 73 +++- .../tests/test_middle_click_paste.html | 77 ++++ ...torMailSupport_insertAsCitedQuotation.html | 33 ++ ...st_nsIPlaintextEditor_insertLineBreak.html | 138 ++++-- .../test_nsITableEditor_deleteTableCell.html | 123 +++++- ...sITableEditor_deleteTableCellContents.html | 53 ++- .../test_nsITableEditor_deleteTableRow.html | 107 +++++ .../test_nsITableEditor_insertTableCell.html | 90 ++++ ...test_nsITableEditor_insertTableColumn.html | 54 +++ .../test_nsITableEditor_insertTableRow.html | 54 +++ .../test_resizers_resizing_elements.html | 21 + ...undo_after_spellchecker_replaces_word.html | 32 ++ ...t_undo_redo_stack_after_setting_value.html | 52 +++ .../satchel/test/test_form_autocomplete.html | 4 +- .../test_form_autocomplete_with_list.html | 4 +- .../test/test_submit_on_keydown_enter.html | 8 +- .../chrome/file_editor_with_autocomplete.js | 136 +++++- ...st_editor_for_input_with_autocomplete.html | 4 +- ...t_editor_for_textbox_with_autocomplete.xul | 4 +- .../window_composition_text_querycontent.xul | 398 ++++++++++++------ 31 files changed, 1721 insertions(+), 192 deletions(-) create mode 100644 dom/html/test/forms/test_MozEditableElement_setUserInput.html diff --git a/browser/extensions/formautofill/test/mochitest/formautofill_common.js b/browser/extensions/formautofill/test/mochitest/formautofill_common.js index 7ada1b68c1c2..690b5a5d4388 100644 --- a/browser/extensions/formautofill/test/mochitest/formautofill_common.js +++ b/browser/extensions/formautofill/test/mochitest/formautofill_common.js @@ -113,7 +113,22 @@ function triggerAutofillAndCheckProfile(profile) { const element = document.getElementById(fieldName); const expectingEvent = document.activeElement == element ? "DOMAutoComplete" : "change"; const checkFieldAutofilled = Promise.all([ - new Promise(resolve => element.addEventListener("input", resolve, {once: true})), + new Promise(resolve => element.addEventListener("input", (event) => { + if (element.tagName == "INPUT" && element.type == "text") { + todo(event instanceof InputEvent, + `"input" event should be dispatched with InputEvent interface on ${element.tagName}`); + todo_is(event.cancelable, false, + `"input" event should be never cancelable on ${element.tagName}`); + } else { + todo(event instanceof Event && !(event instanceof UIEvent), + `"input" event should be dispatched with Event interface on ${element.tagName}`); + is(event.cancelable, false, + `"input" event should be never cancelable on ${element.tagName}`); + } + is(event.bubbles, true, + `"input" event should always bubble on ${element.tagName}`); + resolve(); + }, {once: true})), new Promise(resolve => element.addEventListener(expectingEvent, resolve, {once: true})), ]).then(() => checkFieldValue(element, value)); diff --git a/browser/extensions/formautofill/test/mochitest/test_clear_form.html b/browser/extensions/formautofill/test/mochitest/test_clear_form.html index 834c9251fded..81041b991d38 100644 --- a/browser/extensions/formautofill/test/mochitest/test_clear_form.html +++ b/browser/extensions/formautofill/test/mochitest/test_clear_form.html @@ -67,7 +67,15 @@ function checkIsFormCleared(patch = {}) { async function confirmClear(selector) { info("Await for clearing input"); let promise = new Promise(resolve => - document.querySelector(selector).addEventListener("input", resolve, {once: true}) + document.querySelector(selector).addEventListener("input", (event) => { + todo(event instanceof InputEvent, + '"input" event should be dispatched with InputEvent interface'); + todo_is(event.cancelable, false, + '"input" event should be never cancelable'); + is(event.bubbles, true, + '"input" event should always bubble'); + resolve(); + }, {once: true}) ); synthesizeKey("KEY_Enter"); await promise; diff --git a/browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html b/browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html index 0cbfc845077f..87b84817bfe8 100644 --- a/browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html +++ b/browser/extensions/formautofill/test/mochitest/test_multi_locale_CA_address_form.html @@ -47,8 +47,21 @@ let MOCK_STORAGE = [{ function checkElementFilled(element, expectedvalue) { return [ new Promise(resolve => { - element.addEventListener("input", function onInput() { + element.addEventListener("input", function onInput(event) { ok(true, "Checking " + element.name + " field fires input event"); + if (element.tagName == "INPUT" && element.type == "text") { + todo(event instanceof InputEvent, + `"input" event should be dispatched with InputEvent interface on ${element.name}`); + todo_is(event.cancelable, false, + `"input" event should be never cancelable on ${element.name}`); + } else { + todo(event instanceof Event && !(event instanceof UIEvent), + `"input" event should be dispatched with Event interface on ${element.name}`); + is(event.cancelable, false, + `"input" event should be never cancelable on ${element.name}`); + } + is(event.bubbles, true, + `"input" event should always bubble on ${element.name}`); resolve(); }, {once: true}); }), diff --git a/dom/base/test/chrome/window_nsITextInputProcessor.xul b/dom/base/test/chrome/window_nsITextInputProcessor.xul index 674bb348b1d9..82606cca9810 100644 --- a/dom/base/test/chrome/window_nsITextInputProcessor.xul +++ b/dom/base/test/chrome/window_nsITextInputProcessor.xul @@ -59,6 +59,16 @@ function onunload() SimpleTest.finish(); } +function checkInputEvent(aEvent, aIsComposing, aDescription) { + if (aEvent.type != "input") { + return; + } + ok(aEvent instanceof InputEvent, `${aDescription}"input" event should be dispatched with InputEvent interface`); + is(aEvent.cancelable, false, `${aDescription}"input" event should be never cancelable`); + is(aEvent.bubbles, true, `${aDescription}"input" event should always bubble`); + is(aEvent.isComposing, aIsComposing, `${aDescription}isComposing should be ${aIsComposing}`); +} + const kIsMac = (navigator.platform.indexOf("Mac") == 0); var iframe = document.getElementById("iframe"); @@ -222,6 +232,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be text"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], true, description); TIP1.cancelComposition(); // Let's check if beginInputTransaction() fails to steal the rights of TextEventDispatcher during commitComposition(). @@ -257,6 +268,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be compositionend"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); // Let's check if beginInputTransaction() fails to steal the rights of TextEventDispatcher during commitCompositionWith("bar"). events = []; @@ -304,6 +316,7 @@ function runBeginInputTransactionMethodTests() description + "events[3] should be compositionend"); is(events[4].type, "input", description + "events[4] should be input"); + checkInputEvent(events[4], false, description); // Let's check if beginInputTransaction() fails to steal the rights of TextEventDispatcher during cancelComposition(). events = []; @@ -346,6 +359,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be compositionend"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], false, description); // Let's check if beginInputTransaction() fails to steal the rights of TextEventDispatcher during keydown() and keyup(). events = []; @@ -385,6 +399,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be keypress"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); is(events[3].type, "keyup", description + "events[3] should be keyup"); @@ -442,6 +457,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be text"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], true, description); TIP1.cancelComposition(); // Let's check if beginInputTransactionForTests() fails to steal the rights of TextEventDispatcher during commitComposition(). @@ -477,6 +493,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be compositionend"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); // Let's check if beginInputTransactionForTests() fails to steal the rights of TextEventDispatcher during commitCompositionWith("bar"). events = []; @@ -524,6 +541,7 @@ function runBeginInputTransactionMethodTests() description + "events[3] should be compositionend"); is(events[4].type, "input", description + "events[4] should be input"); + checkInputEvent(events[4], false, description); // Let's check if beginInputTransactionForTests() fails to steal the rights of TextEventDispatcher during cancelComposition(). events = []; @@ -566,6 +584,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be compositionend"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], false, description); // Let's check if beginInputTransactionForTests() fails to steal the rights of TextEventDispatcher during keydown() and keyup(). events = []; @@ -605,6 +624,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be keypress"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); is(events[3].type, "keyup", description + "events[3] should be keyup"); @@ -692,6 +712,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be text"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], true, description); TIP1.cancelComposition(); // Let's check if beginInputTransaction() with another window fails to begin new input transaction with different TextEventDispatcher during commitComposition(). @@ -745,6 +766,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be compositionend"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); // Let's check if beginInputTransaction() with another window fails to begin new input transaction with different TextEventDispatcher during commitCompositionWith("bar");. events = []; @@ -822,6 +844,7 @@ function runBeginInputTransactionMethodTests() description + "events[3] should be compositionend"); is(events[4].type, "input", description + "events[4] should be input"); + checkInputEvent(events[4], false, description); // Let's check if beginInputTransaction() with another window fails to begin new input transaction with different TextEventDispatcher during cancelComposition();. events = []; @@ -888,6 +911,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be compositionend"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], false, description); // Let's check if beginInputTransaction() with another window fails to begin new input transaction with different TextEventDispatcher during keydown() and keyup();. events = []; @@ -951,6 +975,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be keypress"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); is(events[3].type, "keyup", description + "events[3] should be keyup"); @@ -1038,6 +1063,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be text"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], true, description); TIP1.cancelComposition(); // Let's check if beginInputTransactionForTests() with another window fails to begin new input transaction with different TextEventDispatcher during commitComposition(). @@ -1091,6 +1117,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be compositionend"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); // Let's check if beginInputTransactionForTests() with another window fails to begin new input transaction with different TextEventDispatcher during commitCompositionWith("bar");. events = []; @@ -1168,6 +1195,7 @@ function runBeginInputTransactionMethodTests() description + "events[3] should be compositionend"); is(events[4].type, "input", description + "events[4] should be input"); + checkInputEvent(events[4], false, description); // Let's check if beginInputTransactionForTests() with another window fails to begin new input transaction with different TextEventDispatcher during cancelComposition();. events = []; @@ -1234,6 +1262,7 @@ function runBeginInputTransactionMethodTests() description + "events[2] should be compositionend"); is(events[3].type, "input", description + "events[3] should be input"); + checkInputEvent(events[3], false, description); // Let's check if beginInputTransactionForTests() with another window fails to begin new input transaction with different TextEventDispatcher during keydown() and keyup();. events = []; @@ -1297,6 +1326,7 @@ function runBeginInputTransactionMethodTests() description + "events[1] should be keypress"); is(events[2].type, "input", description + "events[2] should be input"); + checkInputEvent(events[2], false, description); is(events[3].type, "keyup", description + "events[3] should be keyup"); diff --git a/dom/html/test/forms/mochitest.ini b/dom/html/test/forms/mochitest.ini index 59fc3d723a9d..9c94bdd561ec 100644 --- a/dom/html/test/forms/mochitest.ini +++ b/dom/html/test/forms/mochitest.ini @@ -82,6 +82,7 @@ skip-if = os == "android" && debug # bug 1397615 [test_meter_element.html] [test_meter_pseudo-classes.html] [test_min_attribute.html] +[test_MozEditableElement_setUserInput.html] [test_mozistextfield.html] [test_novalidate_attribute.html] [test_option_disabled.html] diff --git a/dom/html/test/forms/test_MozEditableElement_setUserInput.html b/dom/html/test/forms/test_MozEditableElement_setUserInput.html new file mode 100644 index 000000000000..2e22bc98150a --- /dev/null +++ b/dom/html/test/forms/test_MozEditableElement_setUserInput.html @@ -0,0 +1,244 @@ + + + + Test for MozEditableElement.setUserInput() + + + + +
+
+
+
+
+ + + + diff --git a/dom/html/test/forms/test_input_event.html b/dom/html/test/forms/test_input_event.html index 7c8b861d4583..8883ca78a865 100644 --- a/dom/html/test/forms/test_input_event.html +++ b/dom/html/test/forms/test_input_event.html @@ -13,24 +13,24 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=851780 Mozilla Bug 851780

- - - - - - - - + + + + + + + + - - - - - - - - + + + + + + + + 
@@ -40,18 +40,73 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=851780
 
   const isDesktop = !/Mobile|Tablet/.test(navigator.userAgent);
 
-  var textareaInput = 0;
+  function checkIfInputIsInputEvent(aEvent, aToDo, aDescription) {
+    if (aToDo) {
+      // Probably, key operation should fire "input" event with InputEvent interface.
+      // See https://github.com/w3c/input-events/issues/88
+      todo(aEvent instanceof InputEvent,
+         `"input" event should be dispatched with InputEvent interface ${aDescription}`);
+    } else {
+      ok(aEvent instanceof InputEvent,
+         `"input" event should be dispatched with InputEvent interface ${aDescription}`);
+    }
+    is(aEvent.cancelable, false,
+       `"input" event should be never cancelable ${aDescription}`);
+    is(aEvent.bubbles, true,
+       `"input" event should always bubble ${aDescription}`);
+  }
 
-  // Those are types were the input event apply.
+  function checkIfInputIsEvent(aEvent, aDescription) {
+    if (event.target.type === "checkbox" || event.target.type === "radio") {
+      todo(event instanceof Event && !(event instanceof UIEvent),
+           `"input" event should be dispatched with InputEvent interface ${aDescription}`);
+    } else {
+      ok(event instanceof Event && !(event instanceof UIEvent),
+         `"input" event should be dispatched with InputEvent interface ${aDescription}`);
+    }
+    is(aEvent.cancelable, false,
+       `"input" event should be never cancelable ${aDescription}`);
+    is(aEvent.bubbles, true,
+       `"input" event should always bubble ${aDescription}`);
+  }
+
+  var textareaInput = 0;
+  document.getElementById("textarea").oninput = (aEvent) => {
+    ++textareaInput;
+    checkIfInputIsInputEvent(aEvent, false, "on textarea element");
+  };
+
+  // These are the type were the input event apply.
   var textTypes = ["text", "email", "search", "tel", "url", "password"];
   var textInput = [0, 0, 0, 0, 0, 0];
+  for (let id of ["input_text", "input_email", "input_search", "input_tel", "input_url", "input_password"]) {
+    document.getElementById(id).oninput = (aEvent) => {
+      ++textInput[textTypes.indexOf(aEvent.target.type)];
+      checkIfInputIsInputEvent(aEvent, false, `on input element whose type is ${aEvent.target.type}`);
+    };
+  }
 
-  // Those are events were the input event does not apply.
+  // These are the type were the input event does not apply.
   var NonTextTypes = ["button", "submit", "image", "reset", "radio", "checkbox"];
   var NonTextInput = [0, 0, 0, 0, 0, 0];
+  for (let id of ["input_button", "input_submit", "input_image", "input_reset", "input_radio", "input_checkbox"]) {
+    document.getElementById(id).oninput = (aEvent) => {
+      ++NonTextInput[NonTextTypes.indexOf(aEvent.target.type)];
+      checkIfInputIsEvent(aEvent, `on input element whose type is ${aEvent.target.type}`);
+    };
+  }
 
   var rangeInput = 0;
+  document.getElementById("input_range").oninput = (aEvent) => {
+    ++rangeInput;
+    checkIfInputIsEvent(aEvent, "on input element whose type is range");
+  };
+
   var numberInput = 0;
+  document.getElementById("input_number").oninput = (aEvent) => {
+    ++numberInput;
+    checkIfInputIsInputEvent(aEvent, true, "on input element whose type is number");
+  };
 
   SimpleTest.waitForExplicitFinish();
   var MockFilePicker = SpecialPowers.MockFilePicker;
@@ -66,6 +121,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=851780
 
     input.addEventListener("input", function (aEvent) {
       ok(true, "input event should have been dispatched on file input.");
+      checkIfInputIsEvent(aEvent, "on file input");
     });
 
     input.click();
diff --git a/dom/html/test/forms/test_input_number_mouse_events.html b/dom/html/test/forms/test_input_number_mouse_events.html
index 32434aa393c0..f8c0907b8992 100644
--- a/dom/html/test/forms/test_input_number_mouse_events.html
+++ b/dom/html/test/forms/test_input_number_mouse_events.html
@@ -48,6 +48,14 @@ const SPIN_UP_Y = 3;
 const SPIN_DOWN_X = inputRect.width - 3;
 const SPIN_DOWN_Y = inputRect.height - 3;
 
+function checkInputEvent(aEvent, aDescription) {
+  // Probably, key operation should fire "input" event with InputEvent interface.
+  // See https://github.com/w3c/input-events/issues/88
+  todo(aEvent instanceof InputEvent, `"input" event should be dispatched with InputEvent interface on input element whose type is number ${aDescription}`);
+  is(aEvent.cancelable, false, `"input" event should be never cancelable on input element whose type is number ${aDescription}`);
+  is(aEvent.bubbles, true, `"input" event should always bubble on input element whose type is number ${aDescription}`);
+}
+
 function test() {
   input.value = 0;
 
@@ -134,6 +142,7 @@ var spinTests = [
     input.value = 0;
     input.addEventListener("input", function(evt) {
       ++inputEventCount;
+      checkInputEvent(event, "#1");
       if (inputEventCount == 3) {
         is(input.value, "3", "Testing spin-up button");
         synthesizeMouse(input, SPIN_DOWN_X, SPIN_DOWN_Y, { type: "mousemove" });
@@ -154,6 +163,7 @@ var spinTests = [
     input.value = 0;
     input.addEventListener("input", function(evt) {
       ++inputEventCount;
+      checkInputEvent(event, "#2");
       if (inputEventCount == 3) {
         is(input.value, "-3", "Testing spin-down button");
         synthesizeMouse(input, SPIN_UP_X, SPIN_UP_Y, { type: "mousemove" });
@@ -174,6 +184,7 @@ var spinTests = [
     input.value = 0;
     input.addEventListener("input", function(evt) {
       ++inputEventCount;
+      checkInputEvent(event, "#3");
       if (inputEventCount == 3) {
         synthesizeMouse(input, -1, -1, { type: "mousemove" });
         var eventHandler = arguments.callee;
@@ -195,6 +206,7 @@ var spinTests = [
     input.value = 0;
     input.addEventListener("input", function(evt) {
       ++inputEventCount;
+      checkInputEvent(event, "#4");
       if (inputEventCount == 3) {
         input.type = "text"
         var eventHandler = arguments.callee;
diff --git a/editor/libeditor/tests/test_abs_positioner_positioning_elements.html b/editor/libeditor/tests/test_abs_positioner_positioning_elements.html
index d81eddf8daa2..18f26b94ee09 100644
--- a/editor/libeditor/tests/test_abs_positioner_positioning_elements.html
+++ b/editor/libeditor/tests/test_abs_positioner_positioning_elements.html
@@ -72,18 +72,39 @@ SimpleTest.waitForFocus(async function() {
       // XXX Perhaps, we need to add border-top-width here if you add new test to have thick border.
       const kPositionerY = -7;
 
+      let inputEventExpected = true;
+      function onInput(aEvent) {
+        if (!inputEventExpected) {
+          ok(false, "\"input\" event shouldn't be fired after stopping resizing");
+          return;
+        }
+        ok(aEvent instanceof InputEvent,
+           '"input" event should be dispatched with InputEvent interface');
+        is(aEvent.cancelable, false,
+           '"input" event should be never cancelable');
+        is(aEvent.bubbles, true,
+           '"input" event should always bubble');
+      }
+
+      content.addEventListener("input", onInput);
+
       // Click on the positioner.
       synthesizeMouse(target, kPositionerX, kPositionerY, {type: "mousedown"});
       // Drag it delta pixels.
       synthesizeMouse(target, kPositionerX + aDeltaX, kPositionerY + aDeltaY, {type: "mousemove"});
       // Release the mouse button
       synthesizeMouse(target, kPositionerX + aDeltaX, kPositionerY + aDeltaY, {type: "mouseup"});
+
+      inputEventExpected = false;
+
       // Move the mouse delta more pixels to the same direction to make sure that the
       // positioning operation has stopped.
       synthesizeMouse(target, kPositionerX + aDeltaX * 2, kPositionerY + aDeltaY * 2, {type: "mousemove"});
       // Click outside of the image to hide the positioner.
       synthesizeMouseAtCenter(outOfEditor, {});
 
+      content.removeEventListener("input", onInput);
+
       // Get the new dimensions for the absolute positioned element.
       let newRect = target.getBoundingClientRect();
       isfuzzy(newRect.x, rect.x + aDeltaX, 1, description + "The left should be increased by " + aDeltaX + " pixels");
diff --git a/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html b/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html
index 0a71f1946e81..9e13cf207641 100644
--- a/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html
+++ b/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html
@@ -63,6 +63,8 @@ function runTests() {
     var handler = function(aEvent) {
       is(aEvent.target, eventTarget,
          "input event is fired on unexpected element: " + aEvent.target.tagName);
+      ok(aEvent instanceof InputEvent,
+         "input event should be dispatched with InputEvent interface");
       ok(!aEvent.cancelable, "input event must not be cancelable");
       ok(aEvent.bubbles, "input event must be bubbles");
       if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
diff --git a/editor/libeditor/tests/test_dom_input_event_on_texteditor.html b/editor/libeditor/tests/test_dom_input_event_on_texteditor.html
index 4035ab3f415d..0edc1e80502a 100644
--- a/editor/libeditor/tests/test_dom_input_event_on_texteditor.html
+++ b/editor/libeditor/tests/test_dom_input_event_on_texteditor.html
@@ -37,6 +37,8 @@ function runTests() {
     var handler = function(aEvent) {
       is(aEvent.target, aElement,
          "input event is fired on unexpected element: " + aEvent.target.tagName);
+      ok(aEvent instanceof InputEvent,
+         "input event should be dispatched with InputEvent interface");
       ok(!aEvent.cancelable, "input event must not be cancelable");
       ok(aEvent.bubbles, "input event must be bubbles");
       if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
diff --git a/editor/libeditor/tests/test_dragdrop.html b/editor/libeditor/tests/test_dragdrop.html
index 8cdf5e59363b..6e16362b9c62 100644
--- a/editor/libeditor/tests/test_dragdrop.html
+++ b/editor/libeditor/tests/test_dragdrop.html
@@ -24,6 +24,13 @@ SimpleTest.waitForExplicitFinish();
 var shouldClear = false;
 window.addEventListener("dragstart", function(event) { if (shouldClear) event.dataTransfer.clearData(); }, true);
 
+function checkInputEvent(aEvent, aExpectedTarget, aDescription) {
+  ok(aEvent instanceof InputEvent, `"input" event should be dispatched with InputEvent interface ${aDescription}`);
+  is(aEvent.cancelable, false, `"input" event should be never cancelable ${aDescription}`);
+  is(aEvent.bubbles, true, `"input" event should always bubble ${aDescription}`);
+  is(aEvent.target, aExpectedTarget, `"input" event should be fired on the <${aExpectedTarget.tagName.toLowerCase()}> element ${aDescription}`);
+}
+
 function doTest() {
   const htmlContextData = { type: "text/_moz_htmlcontext",
                             data: "" };
@@ -39,56 +46,84 @@ function doTest() {
   var input = document.getElementById("input");
   var contenteditable = document.getElementById("contenteditable");
 
+  var inputEvents = [];
+  function onInput(event) {
+    inputEvents.push(event);
+  }
+  document.addEventListener("input", onInput);
+
   var selection = window.getSelection();
 
   // -------- Test dragging regular text
   selection.selectAllChildren(text);
+  inputEvents = [];
   var result = synthesizeDragStart(text, [[htmlContextData, htmlInfoData, htmlData,
                                            {type: "text/plain", data: "Some Text"}]], window, 40, 10);
   is(result, null, "Test dragging regular text");
+  is(inputEvents.length, 0,
+     'No "input" event should be fired when dragging from text in  element');
 
   // -------- Test dragging text from an 
   input.setSelectionRange(1, 4);
+  inputEvents = [];
   result = synthesizeDragStart(input, [[{type: "text/plain", data: "rag"}]], window, 25, 8);
   is(result, null, "Test dragging input");
+  is(inputEvents.length, 0,
+     'No "input" event should be fired when dragging from text in  element');
 
   // -------- Test dragging text from a