Count chrome and content callers of XMLDocument's load() method separately in telemetry

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

Actually, we may want to separately count system and non-system callers...
Flags: needinfo?(bzbarsky)
Oh, we already have something here, because it's a DeprecatedOperation and we WarnOnceAbout.

Looks like this only produces a "page" counter, not a "document" counter?  What is the difference between the two?

Am I right that <https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-08-23&keys=__none__!__none__!__none__&max_channel_version=release%252F48&measure=USE_COUNTER2_DEPRECATED_UseOfDOM3LoadMethod_PAGE&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-07-26&table=0&trim=1&use_submission_date=0> shows that 2.65% of pageloads called this method in Firefox 48 release?

I guess we could try changing to separate content/chrome deprecated operations here and see what the split looks like...
Flags: needinfo?(bzbarsky) → needinfo?(nfroyd)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> Looks like this only produces a "page" counter, not a "document" counter? 
> What is the difference between the two?

"page" is roughly "anything in a top-level tab and sub-documents thereof" whereas "document" is individual pages without considering sub-documents.

> Am I right that
> <https://telemetry.mozilla.org/new-pipeline/dist.html#!
> cumulative=0&end_date=2016-08-23&keys=__none__!__none__!
> __none__&max_channel_version=release%252F48&measure=USE_COUNTER2_DEPRECATED_U
> seOfDOM3LoadMethod_PAGE&min_channel_version=null&product=Firefox&sanitize=1&s
> ort_keys=submissions&start_date=2016-07-
> 26&table=0&trim=1&use_submission_date=0> shows that 2.65% of pageloads
> called this method in Firefox 48 release?

That looks like a correct interpretation to me.
Flags: needinfo?(nfroyd)
Created attachment 8795796 [details] [diff] [review]
part 1.  Remove all the methods from nsIDOMXMLDocument and make it not inherit from nsIDOMDocument anymore

This will let us know for sure that we're being called from script in things
like nsXMLDocument::Load.
Attachment #8795796 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8795797 [details] [diff] [review]
part 2.  Change nsXMLDocument::Load telemetry to separately count chrome and content callers
Attachment #8795797 - Flags: review?(bugs)
I'm hoping beyond hope that most of our callers are chrome and hence we can make this chromeonly, but let's try measuring....
Summary: Add a usecounter for XMLDocument's load() method → Count chrome and content callers of XMLDocument's load() method separately in telemetry

Updated

2 years ago
Attachment #8795796 - Flags: review?(bugs) → review+

Comment 6

2 years ago
Comment on attachment 8795797 [details] [diff] [review]
part 2.  Change nsXMLDocument::Load telemetry to separately count chrome and content callers

Telemetry data may look a bit weird for this stuff for awhile, but I guess that is fine. I mean, it collects currently a + b, and after this patch a and b.


And WarnOnceAbout after principal check is fine, since if we throw because of principal check, it would be fine to throw because of missing method too.
Attachment #8795797 - Flags: review?(bugs) → review+

Comment 7

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/521286426a93
part 1.  Remove all the methods from nsIDOMXMLDocument and make it not inherit from nsIDOMDocument anymore.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1124e3458403
part 2.  Change nsXMLDocument::Load telemetry to separately count chrome and content callers.  r=smaug
Um... I bet the problem is that I didn't end up inheriting nsIDOMXMLDocument from nsISupports.  At least given where that QI failure is.

I guess I'll do a try run of the relevant tests...
Flags: needinfo?(bzbarsky)

Comment 10

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f757d49ef3
part 1.  Remove all the methods from nsIDOMXMLDocument and make it not inherit from nsIDOMDocument anymore.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c4ba6cbf07
part 2.  Change nsXMLDocument::Load telemetry to separately count chrome and content callers.  r=smaug

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22f757d49ef3
https://hg.mozilla.org/mozilla-central/rev/e7c4ba6cbf07
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Bug 1310275 filed on both use counters being pretty bogus.
You need to log in before you can comment on or make changes to this bug.