Bug 1463081 - Instrument inspection of "Edit Rule" in Rule View with event telemetry r?yulia
The changes to telemetry.js::recordEvent() were necessary because the optional value and extra params cannot be sent to Services.telemetry.recordEvent() as undefined without throwing... using null instead works just fine.
MozReview-Commit-ID: CgoqSeR6mkl
--- a/devtools/client/inspector/rules/test/browser_rules_edit-property_01.js
+++ b/devtools/client/inspector/rules/test/browser_rules_edit-property_01.js
@@ -4,16 +4,18 @@
"use strict";
// Testing adding new properties via the inplace-editors in the rule
// view.
// FIXME: some of the inplace-editor focus/blur/commit/revert stuff
// should be factored out in head.js
+const OPTOUT = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
+
const TEST_URI = `
<style type="text/css">
#testid {
color: red;
background-color: blue;
}
.testclass, .unmatched {
background-color: green;
@@ -26,25 +28,67 @@ const TEST_URI = `
var BACKGROUND_IMAGE_URL = 'url("' + URL_ROOT + 'doc_test_image.png")';
var TEST_DATA = [
{ name: "border-color", value: "red", isValid: true },
{ name: "background-image", value: BACKGROUND_IMAGE_URL, isValid: true },
{ name: "border", value: "solid 1px foo", isValid: false },
];
+const DATA = [
+ {
+ timestamp: null,
+ category: "devtools.main",
+ method: "edit_rule",
+ object: "ruleview"
+ },
+ {
+ timestamp: null,
+ category: "devtools.main",
+ method: "edit_rule",
+ object: "ruleview"
+ },
+ {
+ timestamp: null,
+ category: "devtools.main",
+ method: "edit_rule",
+ object: "ruleview"
+ },
+ {
+ timestamp: null,
+ category: "devtools.main",
+ method: "edit_rule",
+ object: "ruleview"
+ },
+ {
+ timestamp: null,
+ category: "devtools.main",
+ method: "edit_rule",
+ object: "ruleview"
+ }
+];
+
add_task(async function() {
+ // Let's reset the counts.
+ Services.telemetry.clearEvents();
+
+ // Ensure no events have been logged
+ const snapshot = Services.telemetry.snapshotEvents(OPTOUT, true);
+ ok(!snapshot.parent, "No events have been logged for the main process");
+
await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
let {inspector, view} = await openRuleView();
await selectNode("#testid", inspector);
let rule = getRuleViewRuleEditor(view, 1).rule;
for (let {name, value, isValid} of TEST_DATA) {
await testEditProperty(view, rule, name, value, isValid);
}
+
+ checkResults();
});
async function testEditProperty(view, rule, name, value, isValid) {
info("Test editing existing property name/value fields");
let doc = rule.editor.doc;
let prop = rule.textProps[0];
@@ -86,8 +130,27 @@ async function testEditProperty(view, ru
});
if (isValid) {
is(propValue, value, name + " should have been set.");
} else {
isnot(propValue, value, name + " shouldn't have been set.");
}
}
+
+function checkResults() {
+ const snapshot = Services.telemetry.snapshotEvents(OPTOUT, true);
+ const events = snapshot.parent.filter(event => event[1] === "devtools.main" &&
+ event[2] === "edit_rule" &&
+ event[3] === "ruleview"
+ );
+
+ for (let i in DATA) {
+ const [ timestamp, category, method, object ] = events[i];
+ const expected = DATA[i];
+
+ // ignore timestamp
+ ok(timestamp > 0, "timestamp is greater than 0");
+ is(category, expected.category, "category is correct");
+ is(method, expected.method, "method is correct");
+ is(object, expected.object, "object is correct");
+ }
+}
--- a/devtools/client/inspector/rules/views/rule-editor.js
+++ b/devtools/client/inspector/rules/views/rule-editor.js
@@ -49,16 +49,17 @@ const STYLE_INSPECTOR_L10N = new Localiz
* The Rule object we're editing.
*/
function RuleEditor(ruleView, rule) {
EventEmitter.decorate(this);
this.ruleView = ruleView;
this.doc = this.ruleView.styleDocument;
this.toolbox = this.ruleView.inspector.toolbox;
+ this.telemetry = this.toolbox.telemetry;
this.rule = rule;
this.isEditable = !rule.isSystem;
// Flag that blocks updates of the selector and properties when it is
// being edited
this.isEditing = false;
this._onNewProperty = this._onNewProperty.bind(this);
@@ -584,16 +585,18 @@ RuleEditor.prototype = {
// case, we're creating a new declaration, it doesn't make sense to accept
// these entries
this.multipleAddedProperties =
parseNamedDeclarations(this.rule.cssProperties.isKnown, value, true);
// Blur the editor field now and deal with adding declarations later when
// the field gets destroyed (see _newPropertyDestroy)
this.editor.input.blur();
+
+ this.telemetry.recordEvent("devtools.main", "edit_rule", "ruleview");
},
/**
* Called when the new property editor is destroyed.
* This is where the properties (type TextProperty) are actually being
* added, since we want to wait until after the inplace editor `destroy`
* event has been fired to keep consistent UI state.
*/
--- a/devtools/client/inspector/rules/views/text-property-editor.js
+++ b/devtools/client/inspector/rules/views/text-property-editor.js
@@ -77,16 +77,17 @@ function TextPropertyEditor(ruleEditor,
this.prop = property;
this.prop.editor = this;
this.browserWindow = this.doc.defaultView.top;
this._populatedComputed = false;
this._hasPendingClick = false;
this._clickedElementOptions = null;
const toolbox = this.ruleView.inspector.toolbox;
+ this.telemetry = toolbox.telemetry;
this.cssProperties = getCssProperties(toolbox);
this.getGridlineNames = this.getGridlineNames.bind(this);
this.update = this.update.bind(this);
this.updatePropertyState = this.updatePropertyState.bind(this);
this._onEnableClicked = this._onEnableClicked.bind(this);
this._onExpandClicked = this._onExpandClicked.bind(this);
this._onNameDone = this._onNameDone.bind(this);
@@ -736,16 +737,17 @@ TextPropertyEditor.prototype = {
let checked = this.enable.hasAttribute("checked");
if (checked) {
this.enable.removeAttribute("checked");
} else {
this.enable.setAttribute("checked", "");
}
this.prop.setEnabled(!checked);
event.stopPropagation();
+ this.telemetry.recordEvent("devtools.main", "edit_rule", "ruleview");
},
/**
* Handles clicks on the computed property expander. If the computed list is
* open due to user expanding or style filtering, collapse the computed list
* and close the expander. Otherwise, add user-open attribute which is used to
* expand the computed list and tracks whether or not the computed list is
* expanded by manually by the user.
@@ -806,16 +808,18 @@ TextPropertyEditor.prototype = {
*/
_onNameDone: function(value, commit, direction) {
let isNameUnchanged = (!commit && !this.ruleEditor.isEditing) ||
this.committed.name === value;
if (this.prop.value && isNameUnchanged) {
return;
}
+ this.telemetry.recordEvent("devtools.main", "edit_rule", "ruleview");
+
// Remove a property if the name is empty
if (!value.trim()) {
this.remove(direction);
return;
}
// Remove a property if the property value is empty and the property
// value is not about to be focused
@@ -897,16 +901,18 @@ TextPropertyEditor.prototype = {
// its original value and enabled or disabled state
if (value.trim() && isValueUnchanged) {
this.ruleEditor.rule.previewPropertyValue(this.prop, val.value,
val.priority);
this.rule.setPropertyEnabled(this.prop, this.prop.enabled);
return;
}
+ this.telemetry.recordEvent("devtools.main", "edit_rule", "ruleview");
+
// Since the value was changed, check if the original propertywas a flex or grid
// display declaration and hide their respective highlighters.
if (this.isDisplayFlex()) {
this.ruleView.highlighters.hideFlexboxHighlighter();
}
if (this.isDisplayGrid()) {
this.ruleView.highlighters.hideGridHighlighter();
--- a/devtools/client/shared/telemetry.js
+++ b/devtools/client/shared/telemetry.js
@@ -495,41 +495,43 @@ class Telemetry {
* The telemetry event category (a group name for events and helps to
* avoid name conflicts) e.g. "devtools.main"
* @param {String} method
* The telemetry event method (describes the type of event that
* occurred e.g. "open")
* @param {String} object
* The telemetry event object name (the name of the object the event
* occurred on) e.g. "tools" or "setting"
- * @param {String|null} value
- * The telemetry event value (a user defined value, providing context
- * for the event) e.g. "console"
- * @param {Object} extra
- * The telemetry event extra object containing the properties that will
- * be sent with the event e.g.
+ * @param {String|null} [value]
+ * Optional telemetry event value (a user defined value, providing
+ * context for the event) e.g. "console"
+ * @param {Object} [extra]
+ * Optional telemetry event extra object containing the properties that
+ * will be sent with the event e.g.
* {
* host: "bottom",
* width: "1024"
* }
*/
- recordEvent(category, method, object, value, extra) {
+ recordEvent(category, method, object, value = null, extra = null) {
// Only string values are allowed so cast all values to strings.
- for (let [name, val] of Object.entries(extra)) {
- val = val + "";
- extra[name] = val;
+ if (extra) {
+ for (let [name, val] of Object.entries(extra)) {
+ val = val + "";
+ extra[name] = val;
- if (val.length > 80) {
- const sig = `${category},${method},${object},${value}`;
+ if (val.length > 80) {
+ const sig = `${category},${method},${object},${value}`;
- throw new Error(`The property "${name}" was added to a telemetry ` +
- `event with the signature ${sig} but it's value ` +
- `"${val}" is longer than the maximum allowed length ` +
- `of 80 characters\n` +
- `CALLER: ${getCaller()}`);
+ throw new Error(`The property "${name}" was added to a telemetry ` +
+ `event with the signature ${sig} but it's value ` +
+ `"${val}" is longer than the maximum allowed length ` +
+ `of 80 characters\n` +
+ `CALLER: ${getCaller()}`);
+ }
}
}
Services.telemetry.recordEvent(category, method, object, value, extra);
}
/**
* Sends telemetry pings to indicate that a tool has been opened.
*
--- a/toolkit/components/telemetry/Events.yaml
+++ b/toolkit/components/telemetry/Events.yaml
@@ -277,8 +277,16 @@ devtools.main:
notification_emails: ["dev-developer-tools@lists.mozilla.org", "hkirschner@mozilla.com"]
record_in_processes: ["main"]
description: User is editing HTML via the context menu item in the markup view.
release_channel_collection: opt-out
expiry_version: never
extra_keys:
made_changes: Indicates whether changes were made.
time_open: The amount of time in ms that the HTML editor was open.
+ edit_rule:
+ objects: ["ruleview"]
+ bug_numbers: [1463081]
+ notification_emails: ["dev-developer-tools@lists.mozilla.org", "hkirschner@mozilla.com"]
+ record_in_processes: ["main"]
+ description: User is editing a CSS rule by clicking on or next to a CSS property, enabling / disabling a rule or creating a new property.
+ release_channel_collection: opt-out
+ expiry_version: never