Closed Bug 1053467 Opened 6 years ago Closed 6 years ago

Add telemetry to measure favicon sizes attribute usage

Categories

(Firefox :: General, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: rittme, Assigned: rittme)

Details

Attachments

(1 file, 3 obsolete files)

We should add telemetry to measure how widespread is the usage of the sizes attribute in icon link tags and get actual the dimensions used in case of availability.
Flags: firefox-backlog?
QA Whiteboard: [qa-]
Assignee: nobody → bernardo
Iteration: --- → 34.2
Points: --- → 2
Flags: firefox-backlog? → firefox-backlog+
Status: NEW → ASSIGNED
These two probes should allow us to measure the web usage and available sizes of sizes attribute on icon link tags. I have added two probes:
LINK_ICON_SIZES_ATTR_USAGE: measures if the icon link tag has a sizes attribute and of what type is it's value (any, numeric or invalid).

LINK_ICON_SIZES_ATTR_DIMENSION: measures the width of the numeric sizes values.
Attachment #8473992 - Flags: review?(MattN+bmo)
The probes on this patch do not collect information on e10s tabs. 
AFAIK this is caused by bug 1024021 and should be fixed by it.
Fixed white spaces and some bad styling.
Attachment #8473992 - Attachment is obsolete: true
Attachment #8473992 - Flags: review?(MattN+bmo)
Attachment #8474205 - Flags: review?(MattN+bmo)
Comment on attachment 8474205 [details] [diff] [review]
rev 2 - Added telemetry to measure the usage and values of the sizes attribute in link icon tags

Review of attachment 8474205 [details] [diff] [review]:
-----------------------------------------------------------------

Did you confirm that telemetry works from the content JSM when you are running in an E10S window?

::: browser/modules/ContentLinkHandler.jsm
@@ +21,5 @@
> +const SIZES_TELEMETRY_ENUM = {
> +  NO_SIZES: 0,
> +  ANY: 1,
> +  DIMENSION: 2,
> +  INVALID: 3

Nit: always include the trailing comma after object properties so blame doesn't change when a new property is appended.

@@ +87,5 @@
> +              for (let size of link.sizes) {
> +                if (size.toLowerCase() === "any") {
> +                  sizeHistogramTypes.add(SIZES_TELEMETRY_ENUM.ANY);
> +                } else {
> +                  var re = /^([1-9][0-9]*)[xX][1-9][0-9]*$/i;

I think you can use "x" instead of "[xX]" since you have /i

@@ +88,5 @@
> +                if (size.toLowerCase() === "any") {
> +                  sizeHistogramTypes.add(SIZES_TELEMETRY_ENUM.ANY);
> +                } else {
> +                  var re = /^([1-9][0-9]*)[xX][1-9][0-9]*$/i;
> +                  var values = re.exec(size);

Use let instead of var

@@ +93,5 @@
> +                  if (values && values.length > 1) {
> +                    sizeHistogramTypes.add(SIZES_TELEMETRY_ENUM.DIMENSION);
> +                    sizeHistogramDimension.add(parseInt(values[1]));
> +                  } else {
> +                    sizeHistogramTypes.add(SIZES_TELEMETRY_ENUM.INVALID);

I think the data would be easier to understand if we only add to sizeHistogramTypes once per <link re=icon> instead of once per @sizes token. It doesn't seem like the spec disallows sizes="any 1x1" though which I find weird. Is that also your understanding of the spec?

::: toolkit/components/telemetry/Histograms.json
@@ +3088,5 @@
> +  "LINK_ICON_SIZES_ATTR_USAGE": {
> +    "expires_in_version" : "never",
> +    "kind": "enumerated",
> +    "n_values": 4,
> +    "description": "The possible types of the sizes attribute in icon link tags (<link rel='icon'>). 0: No sizes attribute, 1: Size of value ANY, 2: Size of integer value, 3: Size of invalid value."

Nit: put quotes around the first mention of 'sizes'.
Nit: …'sizes' attribute for <link rel=icon>.
Nit: Don't duplicate the word "size(s)" in the descriptions of the values in order to shorten it:
0: Attribute not specified
1: "any"
2: Integer dimensions
3: Invalid value

@@ +3095,5 @@
> +    "expires_in_version" : "never",
> +    "kind": "linear",
> +    "high": 513,
> +    "n_buckets" : 64,
> +    "description": "The width dimension of the sizes attribute in icon link tags (<link rel='icon'>)."

ditto for the first two nits above
Attachment #8474205 - Flags: review?(MattN+bmo) → feedback+
The probes do not work on e10s tabs, but it should be fixed by bug 1024021 and it shouldn't be a blocker for this bug.

For the type probe, if we have multiple dimensions we store only one type: 
If we have an invalid entry it will be set as invalid.
If we have "any" the type will be "any", even if we have other set dimensions.
Attachment #8474205 - Attachment is obsolete: true
Attachment #8474905 - Flags: review?(MattN+bmo)
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Comment on attachment 8474905 [details] [diff] [review]
rev 3 - Added telemetry to measure the usage and values of the sizes attribute in link icon tags

Review of attachment 8474905 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1053467 - Added telemetry to measure the usage and values of the sizes attribute in link icon tags (<link rel='icon'>)

Nit: We rarely call elements "tags". I think "…of the sizes attribute of <link rel=icon>. r=MattN" would be clearer.

r=me with comments

::: browser/modules/ContentLinkHandler.jsm
@@ +81,5 @@
>  
> +            // Telemetry probes for measuring the sizes attribute
> +            // usage and available dimensions.
> +            let sizeHistogramTypes = Services.telemetry.getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");
> +            let sizeHistogramDimension = Services.telemetry.getHistogramById("LINK_ICON_SIZES_ATTR_DIMENSION");

Nit: the comment doesn't need to wrap but the two code lines above do. e.g.
let sizeHistogramTypes = Services.telemetry.
                         getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");

@@ +82,5 @@
> +            // Telemetry probes for measuring the sizes attribute
> +            // usage and available dimensions.
> +            let sizeHistogramTypes = Services.telemetry.getHistogramById("LINK_ICON_SIZES_ATTR_USAGE");
> +            let sizeHistogramDimension = Services.telemetry.getHistogramById("LINK_ICON_SIZES_ATTR_DIMENSION");
> +            let hType;

Nit: hType is unnecessarily short. Maybe "sizesType" since it's not for the type of histogram?

@@ +85,5 @@
> +            let sizeHistogramDimension = Services.telemetry.getHistogramById("LINK_ICON_SIZES_ATTR_DIMENSION");
> +            let hType;
> +            if (link.sizes.length) {
> +              for (let size of link.sizes) {
> +                if (size.toLowerCase() === "any") {

Nit: The coding style is to use "==" unless "===" is really necessary. In this case I think "==" is sufficient.
Attachment #8474905 - Flags: review?(MattN+bmo) → review+
Thank you, Matt.
Here is the patch with the review fixes.
Attachment #8474905 - Attachment is obsolete: true
Attachment #8476475 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/330d323a5043
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.