Bug 1463081 - Instrument inspection of "Edit Rule" in Rule View with event telemetry r?yulia draft
authorMichael Ratcliffe <mratcliffe@mozilla.com>
Wed, 30 May 2018 17:53:29 +0100
changeset 802106 5c676b7331db3f8ece5961aa80347259f91a595f
parent 802102 763f30c3421233a45ef9e67a695c5c241a2c8a3a
child 802111 50cde18b23b02f594422e89bdeda12e2806134e7
push id111817
push usermratcliffe@mozilla.com
push dateThu, 31 May 2018 11:25:18 +0000
reviewersyulia
bugs1463081
milestone62.0a1
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
devtools/client/inspector/rules/test/browser_rules_edit-property_01.js
devtools/client/inspector/rules/views/rule-editor.js
devtools/client/inspector/rules/views/text-property-editor.js
devtools/client/shared/telemetry.js
toolkit/components/telemetry/Events.yaml
--- 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