From fe1128dd14c9b16d7dc629acf675edd0aa657940 Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Tue, 20 Jun 2017 16:32:18 -0700 Subject: [PATCH] Bug 1373483 - Add telemetry for the grid inspector. r=pbro. data-r=bsmedberg --- .../client/inspector/boxmodel/box-model.js | 7 +- .../client/inspector/grids/grid-inspector.js | 65 ++++++++++------- .../client/inspector/grids/test/browser.ini | 1 + ...ser_grids_number-of-css-grids-telemetry.js | 45 ++++++++++++ devtools/client/inspector/inspector.js | 4 ++ .../inspector/shared/highlighters-overlay.js | 21 ++++-- devtools/client/shared/telemetry.js | 9 +++ devtools/server/actors/layout.js | 12 ---- devtools/shared/specs/layout.js | 7 -- toolkit/components/telemetry/Histograms.json | 11 +++ toolkit/components/telemetry/Scalars.yaml | 70 +++++++++++++++++++ 11 files changed, 199 insertions(+), 53 deletions(-) create mode 100644 devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js diff --git a/devtools/client/inspector/boxmodel/box-model.js b/devtools/client/inspector/boxmodel/box-model.js index cc3f1e435838..01ed5e579ac6 100644 --- a/devtools/client/inspector/boxmodel/box-model.js +++ b/devtools/client/inspector/boxmodel/box-model.js @@ -347,10 +347,9 @@ BoxModel.prototype = { }, /** - * Handler for the inspector sidebar select event. Starts listening for - * "grid-layout-changed" if the layout panel is visible. Otherwise, stop - * listening for grid layout changes. Finally, refresh the layout view if - * it is visible. + * Handler for the inspector sidebar select event. Starts tracking reflows if the + * layout panel is visible. Otherwise, stop tracking reflows. Finally, refresh the box + * model view if it is visible. */ onSidebarSelect() { if (!this.isPanelVisible()) { diff --git a/devtools/client/inspector/grids/grid-inspector.js b/devtools/client/inspector/grids/grid-inspector.js index b879e7ce1a64..8efa464c1695 100644 --- a/devtools/client/inspector/grids/grid-inspector.js +++ b/devtools/client/inspector/grids/grid-inspector.js @@ -20,6 +20,8 @@ const { updateShowInfiniteLines, } = require("./actions/highlighter-settings"); +const CSS_GRID_COUNT_HISTOGRAM_ID = "DEVTOOLS_NUMBER_OF_CSS_GRIDS_IN_A_PAGE"; + const SHOW_GRID_AREAS = "devtools.gridinspector.showGridAreas"; const SHOW_GRID_LINE_NUMBERS = "devtools.gridinspector.showGridLineNumbers"; const SHOW_INFINITE_LINES_PREF = "devtools.gridinspector.showInfiniteLines"; @@ -43,6 +45,7 @@ function GridInspector(inspector, window) { this.highlighters = inspector.highlighters; this.inspector = inspector; this.store = inspector.store; + this.telemetry = inspector.telemetry; this.walker = this.inspector.walker; this.getSwatchColorPickerTooltip = this.getSwatchColorPickerTooltip.bind(this); @@ -91,6 +94,7 @@ GridInspector.prototype = { this.highlighters.on("grid-highlighter-hidden", this.onHighlighterChange); this.highlighters.on("grid-highlighter-shown", this.onHighlighterChange); this.inspector.sidebar.on("select", this.onSidebarSelect); + this.inspector.target.on("navigate", this.onGridLayoutChange); this.onSidebarSelect(); }), @@ -103,7 +107,7 @@ GridInspector.prototype = { this.highlighters.off("grid-highlighter-hidden", this.onHighlighterChange); this.highlighters.off("grid-highlighter-shown", this.onHighlighterChange); this.inspector.sidebar.off("select", this.onSidebarSelect); - this.layoutInspector.off("grid-layout-changed", this.onGridLayoutChange); + this.inspector.target.off("navigate", this.onGridLayoutChange); this.inspector.reflowTracker.untrackReflows(this, this.onReflow); @@ -240,31 +244,34 @@ GridInspector.prototype = { this.lastHighlighterNode = node; this.lastHighlighterState = node !== this.highlighters.gridHighlighterShown; - this.highlighters.toggleGridHighlighter(node, settings); + this.highlighters.toggleGridHighlighter(node, settings, "grid"); }, /** * Updates the grid panel by dispatching the new grid data. This is called when the * layout view becomes visible or the view needs to be updated with new grid data. - * - * @param {Array|null} gridFronts - * Optional array of all GridFront in the current page. */ - updateGridPanel: Task.async(function* (gridFronts) { + updateGridPanel: Task.async(function* () { // Stop refreshing if the inspector or store is already destroyed. if (!this.inspector || !this.store) { return; } // Get all the GridFront from the server if no gridFronts were provided. - if (!gridFronts) { - try { - gridFronts = yield this.layoutInspector.getAllGrids(this.walker.rootNode); - } catch (e) { - // This call might fail if called asynchrously after the toolbox is finished - // closing. - return; - } + let gridFronts; + try { + gridFronts = yield this.layoutInspector.getAllGrids(this.walker.rootNode); + } catch (e) { + // This call might fail if called asynchrously after the toolbox is finished + // closing. + return; + } + + // Log how many CSS Grid elements DevTools sees. + if (gridFronts.length > 0 && + this.inspector.target.url != this.inspector.previousURL) { + this.telemetry.log(CSS_GRID_COUNT_HISTOGRAM_ID, gridFronts.length); + this.inspector.previousURL = this.inspector.target.url; } let grids = []; @@ -296,14 +303,11 @@ GridInspector.prototype = { }), /** - * Handler for "grid-layout-changed" events emitted from the LayoutActor. - * - * @param {Array} grids - * Array of all GridFront in the current page. + * Handler for "navigate" event fired by the tab target. Updates grid panel contents. */ - onGridLayoutChange(grids) { + onGridLayoutChange() { if (this.isPanelVisible()) { - this.updateGridPanel(grids); + this.updateGridPanel(); } }, @@ -461,14 +465,12 @@ GridInspector.prototype = { }, /** - * Handler for the inspector sidebar select event. Starts listening for - * "grid-layout-changed" if the layout panel is visible. Otherwise, stop - * listening for grid layout changes. Finally, refresh the layout view if - * it is visible. + * Handler for the inspector sidebar "select" event. Starts tracking reflows + * if the layout panel is visible. Otherwise, stop tracking reflows. + * Finally, refresh the layout view if it is visible. */ onSidebarSelect() { if (!this.isPanelVisible()) { - this.layoutInspector.off("grid-layout-changed", this.onGridLayoutChange); this.inspector.reflowTracker.untrackReflows(this, this.onReflow); return; } @@ -477,7 +479,6 @@ GridInspector.prototype = { Services.prefs.setIntPref(PROMOTE_COUNT_PREF, 0); this.inspector.reflowTracker.trackReflows(this, this.onReflow); - this.layoutInspector.on("grid-layout-changed", this.onGridLayoutChange); this.updateGridPanel(); }, @@ -509,6 +510,10 @@ GridInspector.prototype = { this.store.dispatch(updateShowGridAreas(enabled)); Services.prefs.setBoolPref(SHOW_GRID_AREAS, enabled); + if (enabled) { + this.telemetry.toolOpened("gridInspectorShowGridAreasOverlayChecked"); + } + let { grids } = this.store.getState(); for (let grid of grids) { @@ -532,6 +537,10 @@ GridInspector.prototype = { this.store.dispatch(updateShowGridLineNumbers(enabled)); Services.prefs.setBoolPref(SHOW_GRID_LINE_NUMBERS, enabled); + if (enabled) { + this.telemetry.toolOpened("gridInspectorShowGridLineNumbersChecked"); + } + let { grids } = this.store.getState(); for (let grid of grids) { @@ -555,6 +564,10 @@ GridInspector.prototype = { this.store.dispatch(updateShowInfiniteLines(enabled)); Services.prefs.setBoolPref(SHOW_INFINITE_LINES_PREF, enabled); + if (enabled) { + this.telemetry.toolOpened("gridInspectorShowInfiniteLinesChecked"); + } + let { grids } = this.store.getState(); for (let grid of grids) { diff --git a/devtools/client/inspector/grids/test/browser.ini b/devtools/client/inspector/grids/test/browser.ini index f3aad88eb655..fc23ef1e5ca7 100644 --- a/devtools/client/inspector/grids/test/browser.ini +++ b/devtools/client/inspector/grids/test/browser.ini @@ -29,3 +29,4 @@ support-files = [browser_grids_grid-outline-highlight-cell.js] [browser_grids_grid-outline-selected-grid.js] [browser_grids_highlighter-setting-rules-grid-toggle.js] +[browser_grids_number-of-css-grids-telemetry.js] diff --git a/devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js b/devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js new file mode 100644 index 000000000000..ef901d302e5a --- /dev/null +++ b/devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js @@ -0,0 +1,45 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +// Tests that the telemetry count for the number of CSS Grid Elements on a page navigation +// is correct when the toolbox is opened. + +const TEST_URI1 = ` +
+`; + +const TEST_URI2 = ` + +
+
cell1
+
cell2
+
+`; + +const CSS_GRID_COUNT_HISTOGRAM_ID = "DEVTOOLS_NUMBER_OF_CSS_GRIDS_IN_A_PAGE"; + +add_task(function* () { + yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI1)); + + let { inspector } = yield openLayoutView(); + let { store } = inspector; + + info("Navigate to TEST_URI2"); + + let onGridListUpdate = waitUntilState(store, state => state.grids.length == 1); + yield navigateTo(inspector, + "data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI2)); + yield onGridListUpdate; + + let histogram = Services.telemetry.getHistogramById(CSS_GRID_COUNT_HISTOGRAM_ID); + let snapshot = histogram.snapshot(); + + is(snapshot.counts[1], 1, "Got a count of 1 for 1 CSS Grid element seen."); + is(snapshot.sum, 1, "Got the correct sum."); +}); diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js index d1fa320a6b5a..37f835d31a8b 100644 --- a/devtools/client/inspector/inspector.js +++ b/devtools/client/inspector/inspector.js @@ -101,6 +101,10 @@ function Inspector(toolbox) { this.store = Store(); this.telemetry = new Telemetry(); + // Store the URL of the target page prior to navigation in order to ensure + // telemetry counts in the Grid Inspector are not double counted on reload. + this.previousURL = this.target.url; + this.nodeMenuTriggerInfo = null; this._handleRejectionIfNotDestroyed = this._handleRejectionIfNotDestroyed.bind(this); diff --git a/devtools/client/inspector/shared/highlighters-overlay.js b/devtools/client/inspector/shared/highlighters-overlay.js index 5ba230fcdb01..809b9fd28780 100644 --- a/devtools/client/inspector/shared/highlighters-overlay.js +++ b/devtools/client/inspector/shared/highlighters-overlay.js @@ -6,6 +6,7 @@ "use strict"; +const Services = require("Services"); const {Task} = require("devtools/shared/task"); const EventEmitter = require("devtools/shared/event-emitter"); const { VIEW_NODE_VALUE_TYPE } = require("devtools/client/inspector/shared/node-types"); @@ -107,14 +108,18 @@ HighlightersOverlay.prototype = { * The NodeFront of the grid container element to highlight. * @param {Object} options * Object used for passing options to the grid highlighter. + * @param. {String|null} trigger + * String name matching "grid" or "rule" to indicate where the + * grid highlighter was toggled on from. "grid" represents the grid view + * "rule" represents the rule view. */ - toggleGridHighlighter: Task.async(function* (node, options = {}) { + toggleGridHighlighter: Task.async(function* (node, options = {}, trigger) { if (node == this.gridHighlighterShown) { yield this.hideGridHighlighter(node); return; } - yield this.showGridHighlighter(node, options); + yield this.showGridHighlighter(node, options, trigger); }), /** @@ -125,7 +130,7 @@ HighlightersOverlay.prototype = { * @param {Object} options * Object used for passing options to the grid highlighter. */ - showGridHighlighter: Task.async(function* (node, options) { + showGridHighlighter: Task.async(function* (node, options, trigger) { let highlighter = yield this._getHighlighter("CssGridHighlighter"); if (!highlighter) { return; @@ -138,6 +143,12 @@ HighlightersOverlay.prototype = { this._toggleRuleViewGridIcon(node, true); + if (trigger == "grid") { + Services.telemetry.scalarAdd("devtools.grid.gridinspector.opened", 1); + } else if (trigger == "rule") { + Services.telemetry.scalarAdd("devtools.rules.gridinspector.opened", 1); + } + try { // Save grid highlighter state. let { url } = this.inspector.target; @@ -391,7 +402,8 @@ HighlightersOverlay.prototype = { highlighterSettings.color = grid ? grid.color : DEFAULT_GRID_COLOR; - this.toggleGridHighlighter(this.inspector.selection.nodeFront, highlighterSettings); + this.toggleGridHighlighter(this.inspector.selection.nodeFront, highlighterSettings, + "rule"); }, onMouseMove: function (event) { @@ -516,6 +528,7 @@ HighlightersOverlay.prototype = { this.highlighters = null; this.highlighterUtils = null; this.supportsHighlighters = null; + this.state = null; this.geometryEditorHighlighterShown = null; this.gridHighlighterShown = null; diff --git a/devtools/client/shared/telemetry.js b/devtools/client/shared/telemetry.js index 775604877c29..3cbb8f80c1c4 100644 --- a/devtools/client/shared/telemetry.js +++ b/devtools/client/shared/telemetry.js @@ -177,6 +177,15 @@ Telemetry.prototype = { reloadAddonReload: { histogram: "DEVTOOLS_RELOAD_ADDON_RELOAD_COUNT", }, + gridInspectorShowGridAreasOverlayChecked: { + scalar: "devtools.grid.showGridAreasOverlay.checked", + }, + gridInspectorShowGridLineNumbersChecked: { + scalar: "devtools.grid.showGridLineNumbers.checked", + }, + gridInspectorShowInfiniteLinesChecked: { + scalar: "devtools.grid.showInfiniteLines.checked", + }, }, /** diff --git a/devtools/server/actors/layout.js b/devtools/server/actors/layout.js index 2edbd70b59a2..1650eef0bc30 100644 --- a/devtools/server/actors/layout.js +++ b/devtools/server/actors/layout.js @@ -4,7 +4,6 @@ "use strict"; -const events = require("sdk/event/core"); const { Actor, ActorClassWithSpec } = require("devtools/shared/protocol"); const { getStringifiableFragments } = require("devtools/server/actors/utils/css-grid-utils"); @@ -72,17 +71,11 @@ var LayoutActor = ActorClassWithSpec(layoutSpec, { this.tabActor = tabActor; this.walker = walker; - - this.onNavigate = this.onNavigate.bind(this); - - events.on(this.tabActor, "navigate", this.onNavigate); }, destroy: function () { Actor.prototype.destroy.call(this); - events.off(this.tabActor, "navigate", this.onNavigate); - this.tabActor = null; this.walker = null; }, @@ -142,11 +135,6 @@ var LayoutActor = ActorClassWithSpec(layoutSpec, { return grids; }, - onNavigate: function () { - let grids = this.getAllGrids(this.walker.rootNode); - events.emit(this, "grid-layout-changed", grids); - }, - }); exports.GridActor = GridActor; diff --git a/devtools/shared/specs/layout.js b/devtools/shared/specs/layout.js index d6976258334d..1a9bf5ea2e56 100644 --- a/devtools/shared/specs/layout.js +++ b/devtools/shared/specs/layout.js @@ -16,13 +16,6 @@ const gridSpec = generateActorSpec({ const layoutSpec = generateActorSpec({ typeName: "layout", - events: { - "grid-layout-changed": { - type: "grid-layout-changed", - grids: Arg(0, "array:grid") - } - }, - methods: { getAllGrids: { request: { diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 0b9f20d19444..c45739cb6ec5 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -10097,6 +10097,17 @@ "kind": "count", "description": "Reports the command name used in GCLI e.g. 'screenshot'" }, + "DEVTOOLS_NUMBER_OF_CSS_GRIDS_IN_A_PAGE": { + "record_in_processes": ["main", "content"], + "alert_emails": ["dev-developer-tools@lists.mozilla.org"], + "expires_in_version": "never", + "kind": "linear", + "high": 30, + "n_buckets": 29, + "bug_numbers": [1373483], + "description": "On page load, record the number of CSS Grid elements present on a page when the DevTools is open", + "releaseChannelCollection": "opt-out" + }, "VIEW_SOURCE_IN_BROWSER_OPENED_BOOLEAN": { "record_in_processes": ["main", "content"], "alert_emails": ["mozilla-dev-developer-tools@lists.mozilla.org", "jryans@mozilla.com"], diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml index 4a00cc61a024..6db736b591b8 100644 --- a/toolkit/components/telemetry/Scalars.yaml +++ b/toolkit/components/telemetry/Scalars.yaml @@ -476,6 +476,76 @@ devtools.copy.xpath: record_in_processes: - 'main' +devtools.rules.gridinspector: + opened: + bug_numbers: + - 1373483 + description: > + Number of times the DevTools grid inspector was opened from the rules view. + expires: never + kind: uint + notification_emails: + - dev-developer-tools@lists.mozilla.org + release_channel_collection: opt-out + record_in_processes: + - 'main' + +devtools.grid.gridinspector: + opened: + bug_numbers: + - 1373483 + description: > + Number of times the DevTools grid inspector was opened from the grid view. + expires: never + kind: uint + notification_emails: + - dev-developer-tools@lists.mozilla.org + release_channel_collection: opt-out + record_in_processes: + - 'main' + +devtools.grid.showGridAreasOverlay: + checked: + bug_numbers: + - 1373483 + description: > + Number of times the DevTools grid inspector's "Display grid areas" was checked. + expires: never + kind: uint + notification_emails: + - dev-developer-tools@lists.mozilla.org + release_channel_collection: opt-out + record_in_processes: + - 'main' + +devtools.grid.showGridLineNumbers: + checked: + bug_numbers: + - 1373483 + description: > + Number of times the DevTools grid inspector's "Display grid numbers" was checked. + expires: never + kind: uint + notification_emails: + - dev-developer-tools@lists.mozilla.org + release_channel_collection: opt-out + record_in_processes: + - 'main' + +devtools.grid.showInfiniteLines: + checked: + bug_numbers: + - 1373483 + description: > + Number of times the DevTools grid inspector's "Extend grid lines infinitely" was checked. + expires: never + kind: uint + notification_emails: + - dev-developer-tools@lists.mozilla.org + release_channel_collection: opt-out + record_in_processes: + - 'main' + navigator.storage: estimate_count: bug_numbers: