968572 - add telemetry to determine if it is safe to turn 'fill' and 'stroke' into shorthands

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

The SVG Working Group wants to know whether it is safe to turn the fill and stroke properties into shorthands for the fill-* and stroke-* properties (and presumably new fill-paint and fill-stroke ones to store the paint value).

We could use a telemetry probe to see whether there is an appreciable number of "unsafe" usages of fill/stroke that would result in content looking different if they also set all of the proposed component properties.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Taras, can you check the telemetry-related parts of this?  So that's the toolkit/components/telemetry/Histograms.json change and the changes in nsPresContext::~nsPresContext.

David, can you look at the rest?  What I'm checking for are things like this:

* declarations like |p { stroke-width: 2px; stroke: blue; }|

* cascading like |p { stroke-width: 2px; } p { stroke: blue; }|

* inheritance like |<g stroke-width="2px"><rect stroke="blue">...|

* DOM calls like |decl.setProperty("stroke", "blue")| and
  |decl.removeProperty("stroke")| when the declaration already has a
  stroke-width value

Also, is is true that Telemetry is not enabled by default on Release/Beta?  I'm wondering whether we can get useful enough information out of this if that's the case.

Comment 2

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=f3babe27aab5 (with the actual Telemetry::Accumulate calls skipped, but with printfs in its place)

Comment 3

[Cameron McCormack (:heycam)]

Better push: https://tbpl.mozilla.org/?tree=Try&rev=f293017be80d

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 8371178 [details] [diff] [review]
patch

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

> Also, is is true that Telemetry is not enabled by default on Release/Beta? 

Correct

> I'm wondering whether we can get useful enough information out of this if
> that's the case.

I'm not sure. The opt-in and pre-release channel populations do skew toward more technical people and better hardware, but it doesn't seem like that would bias your data in either direction. There might be a geographic bias though.

Also, are you sure you want to report safe/unsafe/no usage of these properties to Telemetry on a per-document basis? It seems like accounting per-site might be more meaningful.

::: layout/base/nsPresContext.h
@@ +1267,5 @@
>    mozilla::TimeStamp    mLastStyleUpdateForAllAnimations;
>  
> +  // For the SVG_FILL_USAGE and SVG_STROKE_USAGE telemetry probes.
> +  enum SVGFillStrokeUsage {
> +    NoUsage,

"NoUsage = 0," is  a little bit clearer

::: toolkit/components/telemetry/Histograms.json
@@ +5677,5 @@
>      "n_buckets": 10,
>      "extended_statistics_ok": true
> +  },
> +  "SVG_FILL_USAGE": {
> +    "expires_in_version": "never",

I'm guessing we don't actually need this data forever. How about setting it to expire in Firefox 34? That gives us 6 months on each of the channels.

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Vladan Djeric (:vladan) from comment #4)
> > Also, is is true that Telemetry is not enabled by default on Release/Beta? 
> 
> Correct
> 
> > I'm wondering whether we can get useful enough information out of this if
> > that's the case.
> 
> I'm not sure. The opt-in and pre-release channel populations do skew toward
> more technical people and better hardware, but it doesn't seem like that
> would bias your data in either direction. There might be a geographic bias
> though.

I could imagine "more technical people and better hardware" might lead to people using different sites, and thus get different reported usages.  Hard to tell for sure though.  Apparently Chrome's UseCounters are enabled even for release builds.

> Also, are you sure you want to report safe/unsafe/no usage of these
> properties to Telemetry on a per-document basis? It seems like accounting
> per-site might be more meaningful.

I don't think it's possible to report this information globally per-site.  Each user could report it only for unique URLs or domains, but I don't think we're able / allowed to report the URLs back to the Telemetry server so it can say how many unique URLs/domains use a given feature.

Number of times a document is loaded that uses a feature is certainly a crude measure, but might actually reflect more accurately the importance of a given feature we're measuring.  Assuming it's not pages refreshing every 10 seconds in the background or something. :)

> ::: toolkit/components/telemetry/Histograms.json
> @@ +5677,5 @@
> >      "n_buckets": 10,
> >      "extended_statistics_ok": true
> > +  },
> > +  "SVG_FILL_USAGE": {
> > +    "expires_in_version": "never",
> 
> I'm guessing we don't actually need this data forever. How about setting it
> to expire in Firefox 34? That gives us 6 months on each of the channels.

OK.

Comment 6

[Vladan Djeric (:vladan)]

(In reply to Cameron McCormack (:heycam) from comment #5)
> I don't think it's possible to report this information globally per-site. 
> Each user could report it only for unique URLs or domains, but I don't think
> we're able / allowed to report the URLs back to the Telemetry server so it
> can say how many unique URLs/domains use a given feature.

No, I wasn't suggesting reporting URLs (that's a big no-no), I meant tracking these stats on a per-site basis somewhere in memory, then reporting to telemetry the final tally of safe/unsafe/no-usage sites. Your call either way :)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8371178 [details] [diff] [review]
patch

># HG changeset patch
># User 
>
>
>

Please include a commit message when posting patches for review.


>+static bool
>+IsZeroCoordOrFactor(const nsStyleCoord& aValue)
>+{
>+  return
>+    (aValue.GetUnit() == eStyleUnit_Coord &&
>+     nsPresContext::AppUnitsToFloatCSSPixels(aValue.GetCoordValue()) == 1.0f) ||
>+    (aValue.GetUnit() == eStyleUnit_Factor &&
>+     aValue.GetFactorValue() == 1.0f);
>+}

This function is very suspicious, for two reasons:

 (1) the function name ("zero") disagrees with what the function does (checking whether it's 1)

 (2) It's using an == check on floats; I'd prefer aValue.GetCoordValue() == nsPresContext::CSSPixelsToAppUnits(1)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, how does this relate to bug 968923?  Would that get us something more like what comment 6 suggests?

Comment 9

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #8)
> Also, how does this relate to bug 968923?  Would that get us something more
> like what comment 6 suggests?

Yes it will.  I'll wait until bug 968923 is wrapped up and then use its infrastructure for this bug.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8371178 [details] [diff] [review]
patch

ok, cancelling this review request for now, then

Comment 11

[Cameron McCormack (:heycam)]

I don't think there are any active plans for turning fill and stroke into shorthands, so closing this bug.