Closed Bug 1245982 Opened 4 years ago Closed 4 years ago

Add Telemetry for Counting uses of FastSeek

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- ?
firefox47 --- fixed

People

(Reporter: lchristie, Assigned: lchristie)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → lchristie
Attached patch bug-1245982-fix.patch (obsolete) — Splinter Review
Adding a probe for counting the uses of HTMLMediaElement::FastSeek().
Attachment #8716121 - Flags: review?(cpearce)
Attachment #8716121 - Flags: feedback?(benjamin)
Comment on attachment 8716121 [details] [diff] [review]
bug-1245982-fix.patch

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

r+ with changes addressed.

::: dom/html/HTMLMediaElement.cpp
@@ +1432,5 @@
>  HTMLMediaElement::FastSeek(double aTime, ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsIRunnable> task = NS_NewRunnableFunction([]() -> void {
> +    LOG(LogLevel::Debug, ("Reporting telemetry USES_OF_FASTSEEK"));
> +    Telemetry::Accumulate(Telemetry::USES_OF_FASTSEEK, 1);

You don't need to dispatch a task to the main thread to do this; you're already on the main thread here. So just call Log/Telementry::Accumulate directly.

::: toolkit/components/telemetry/Histograms.json
@@ +10074,5 @@
>      "bug_numbers": [1241278],
>      "kind": "boolean",
>      "description": "Usage of the deprecated Notification.requestPermission() callback argument"
> +  },
> +  "USES_OF_FASTSEEK": {

Please call it VIDEO_FASTSEEK_USED, so that it appears with other video telemetry.

@@ +10079,5 @@
> +    "alert_emails": ["lchristie@mozilla.com", "cpearce@mozilla.com"],
> +    "expires_in_version": "55",
> +    "bug_numbers": [1245982],
> +    "kind": "count",
> +    "description": "Uses of HTMLMediaElement::FastSeek"

HTMLMediaElement.fastSeek
Attachment #8716121 - Flags: review?(cpearce) → review+
Attached patch bug-1245982-fix.patch (obsolete) — Splinter Review
Updated with cpearce's comments addressed.
Attachment #8716144 - Flags: review+
Attachment #8716144 - Flags: feedback?(benjamin)
Attachment #8716121 - Flags: feedback?(benjamin)
Attachment #8716144 - Flags: review+
Comment on attachment 8716144 [details] [diff] [review]
bug-1245982-fix.patch

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

::: dom/html/HTMLMediaElement.cpp
@@ +1430,5 @@
>  
>  void
>  HTMLMediaElement::FastSeek(double aTime, ErrorResult& aRv)
>  {
> +  LOG(LogLevel::Debug, ("Reporting telemetry USES_OF_FASTSEEK"));

VIDEO_FASTSEEK_USED
Attachment #8716121 - Attachment is obsolete: true
Attachment #8716144 - Attachment is obsolete: true
Attachment #8716144 - Flags: feedback?(benjamin)
Attachment #8716152 - Flags: feedback?(benjamin)
Attachment #8716152 - Flags: feedback?(benjamin) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ee65964121ba
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Louis, do you want to uplift your telemetry change to Aurora 46? Your change is low-risk and you'll get more data sooner by shipping in 46. :)
Flags: needinfo?(lchristie)
Sure, sounds good to me. What do I need to do?
Flags: needinfo?(lchristie) → needinfo?(cpeterson)
Louis, you just need to edit the "Details" (link) of your attached patch and set the "approval‑mozilla‑aurora" flag to '?' and provide an explanation of why your patch is low risk and worth shipping sooner.

The Release Management team will approve or deny the uplift request and then a "tree sheriff" will merge your patch to the mozilla-aurora tree for you. Here is more information about the uplift process:

https://wiki.mozilla.org/Release_Management/Uplift_rules
Flags: needinfo?(cpeterson)
Probably not worth uplifting. I don't hate fastSeek that much.
You need to log in before you can comment on or make changes to this bug.