Closed Bug 1305681 Opened 8 years ago Closed 7 years ago

Remove DEVTOOLS_HUD_* histograms

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: gfritzsche, Assigned: matrixisreal, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=js] [good first bug])

Attachments

(1 file, 6 obsolete files)

These histograms are expiring and are probably not actively used:
https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
Tamara, i think this is the only code module that uses them:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/b2g/chrome/content/devtools/hud.js#395

Is it feasible to just make `_logHistogram()` a no-op and remove the Histograms.json entries?
Flags: needinfo?(thills)
Hi Georg,

Yes, I think that's a good idea to make _logHistogram a no-op and remove the Histograms.json entries.  I don't believe anything other than FxOS was using this.

Thanks,
-tamara
Flags: needinfo?(thills)
Thanks Tamara.

So, the plan here is to:
* make _logHistogram() [1] a no-op
* remove all the DEVTOOLS_HUD_* entries in Histograms.json and histogram-whitelists.json [2]

1: https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/b2g/chrome/content/devtools/hud.js#395
2: https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
Mentor: gfritzsche
Whiteboard: [measurement:client] → [measurement:client] [lang=js] [good first bug]
And the convention for making the method a noop would be just: 
>_logHistogram(metric){}
?
Flags: needinfo?(gfritzsche)
(In reply to Jacob Mortelliti[:earlgreyhot] from comment #4)
> And the convention for making the method a noop would be just: 
> >_logHistogram(metric){}
> ?

I'm not aware of a convention, but i think this ok if we add a comment as well. I.e.:

> _logHistogram(metric) {
>   // ... explanation on why it's a no-op.
> }
Flags: needinfo?(gfritzsche)
Hi, I would like to work on this bug.
these are the things I am planning to do:
1. Making the entire _logHistogram() method a noop
2. delete the DEVTOOLS_HUD_* entries in repctive json files.
Correct me if iam wrong.
Thanks :)
(In reply to Pavan Karthik from comment #6)
> Hi, I would like to work on this bug.
> these are the things I am planning to do:
> 1. Making the entire _logHistogram() method a noop
> 2. delete the DEVTOOLS_HUD_* entries in repctive json files.

That sounds good to me :)
Let me know if you need help with getting started.
Assignee: nobody → pavankarthikboddeda
Awesome, Ill start working on this today and let you know the status. :D
Attachment #8816566 - Flags: review?(gfritzsche)
Resubmitting as i submitted a wrong patch before.
Attachment #8816577 - Attachment is obsolete: true
Attachment #8816577 - Flags: review?(gfritzsche)
Attachment #8816709 - Flags: review?(gfritzsche)
Comment on attachment 8816566 [details] [diff] [review]
made the _logHistogram(metric) a noop

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

Thanks for the patch!
This looks good overall, but needs some changes.

::: b2g/chrome/content/devtools/hud.js
@@ +317,4 @@
>    },
>  
>    _logHistogram(metric) {
> +             // method left as no-op as histograms are not in use no more.

The indentation here seems wrong - it should use 2 spaces per indentation level.

@@ -372,5 @@
> -      } catch (err) {
> -        console.error('Histogram error: ' + err);
> -      }
> -    }
> -  }

We should not remove this brace ('}'), it closes the scope of the function.

::: browser/themes/linux/browser.css
@@ +1292,4 @@
>  #tabbrowser-tabs {
>    /* override the global style to allow the selected tab to be above the nav-bar */
>    z-index: auto;
> +

The changes in this file are not needed.
Attachment #8816566 - Flags: review?(gfritzsche)
Comment on attachment 8816709 [details] [diff] [review]
removed the DEVTOOLS_HUD_* json entries in Histograms.json and histogram-whitelists.json

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

This part looks fine, thanks!
Attachment #8816709 - Flags: review?(gfritzsche) → review+
Thanks for the review,I have updated the patch accordingly,
What is the next step in the process?
Attachment #8816566 - Attachment is obsolete: true
Attachment #8816918 - Flags: review?(gfritzsche)
Comment on attachment 8816918 [details] [diff] [review]
made the _logHistogram(metric) a noop

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

Thanks, this looks close.
There is a small nit to fix here.
Also we should improve the message of the patch, e.g.: "Bug 1305681 - Remove DEVTOOLS_HUD_* histograms."

::: b2g/chrome/content/devtools/hud.js
@@ +316,5 @@
>      shell.sendEvent(frame, 'advanced-telemetry-update', Cu.cloneInto(payload, frame));
>    },
>  
>    _logHistogram(metric) {
> +    //method left as no-op as histograms are not in use anymore. 

Small nit: This line has a trailing space at the end.
You can follow the review link for the patch to see it highlighted (or use local tools).
Attachment #8816918 - Flags: review?(gfritzsche) → feedback+
Attached patch v4.patch (obsolete) — Splinter Review
Attachment #8816918 - Attachment is obsolete: true
Attachment #8817878 - Flags: review?(gfritzsche)
Comment on attachment 8817878 [details] [diff] [review]
v4.patch

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

This looks good, thanks!
The other patch should also have a commit message of this form, then we can land these patches.
Alternatively you could also put the changes together in one patch.
Attachment #8817878 - Flags: review?(gfritzsche) → review+
Attached patch Remove DEVTOOLS_HUD_* histograms (obsolete) — Splinter Review
Attachment #8816709 - Attachment is obsolete: true
Attachment #8817878 - Attachment is obsolete: true
Attached patch v5.patchSplinter Review
Attachment #8818106 - Attachment is obsolete: true
Attachment #8818107 - Flags: review?(gfritzsche)
Comment on attachment 8818107 [details] [diff] [review]
v5.patch

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

Thanks!
Attachment #8818107 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd906e69d35
Remove DEVTOOLS_HUD_* histograms. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1fd906e69d35
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
> Thanks!

Hi Could you guide me one what to do next/suggest me some other bugs to work on? =)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: