Closed
Bug 1305681
Opened 7 years ago
Closed 7 years ago
Remove DEVTOOLS_HUD_* histograms
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
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)
12.12 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
These histograms are expiring and are probably not actively used: https://dxr.mozilla.org/mozilla-central/search?q=DEVTOOLS_HUD_&redirect=false
Reporter | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 3•7 years ago
|
||
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]
Comment 4•7 years ago
|
||
And the convention for making the method a noop would be just:
>_logHistogram(metric){}
?
Flags: needinfo?(gfritzsche)
Reporter | ||
Comment 5•7 years ago
|
||
(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)
Assignee | ||
Comment 6•7 years ago
|
||
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 :)
Reporter | ||
Comment 7•7 years ago
|
||
(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
Assignee | ||
Comment 8•7 years ago
|
||
Awesome, Ill start working on this today and let you know the status. :D
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8816566 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8816577 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 11•7 years ago
|
||
Resubmitting as i submitted a wrong patch before.
Attachment #8816577 -
Attachment is obsolete: true
Attachment #8816577 -
Flags: review?(gfritzsche)
Attachment #8816709 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 12•7 years ago
|
||
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)
Reporter | ||
Comment 13•7 years ago
|
||
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+
Assignee | ||
Comment 14•7 years ago
|
||
Thanks for the review,I have updated the patch accordingly, What is the next step in the process?
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8816566 -
Attachment is obsolete: true
Attachment #8816918 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
Attachment #8816918 -
Attachment is obsolete: true
Attachment #8817878 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 18•7 years ago
|
||
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+
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8816709 -
Attachment is obsolete: true
Attachment #8817878 -
Attachment is obsolete: true
Assignee | ||
Comment 20•7 years ago
|
||
Attachment #8818106 -
Attachment is obsolete: true
Attachment #8818107 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 21•7 years ago
|
||
Comment on attachment 8818107 [details] [diff] [review] v5.patch Review of attachment 8818107 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8818107 -
Flags: review?(gfritzsche) → review+
Reporter | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 22•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd906e69d35 Remove DEVTOOLS_HUD_* histograms. r=gfritzsche
Keywords: checkin-needed
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fd906e69d35
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 24•7 years ago
|
||
> Thanks!
Hi Could you guide me one what to do next/suggest me some other bugs to work on? =)
Updated•7 years ago
|
status-firefox52:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•