Closed Bug 1304735 Opened 3 years ago Closed 3 years ago

Remove ClearHistogram

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: chutten, Assigned: userajay0, Mentored)

Details

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

Attachments

(1 file)

58 bytes, text/x-review-board-request
chutten
: review+
Details
According to dxr [1], no one uses ClearHistogram. 

So let's remove it.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=ClearHistogram&case=true&=mozilla-central
Mentor: chutten, gfritzsche
Priority: -- → P3
Whiteboard: [measurement:client] [lang=c++]
Whiteboard: [measurement:client] [lang=c++] → [measurement:client] [lang=c++][good first bug]
Hello, I have submitted a patch for the Bug 1304735.
Could the reviewers review the patch and notify me of any issues

Thanks
Comment on attachment 8793843 [details]
Bug 1304735 - Remove ClearHistogram

https://reviewboard.mozilla.org/r/80460/#review79140

::: toolkit/components/telemetry/Telemetry.h:145
(Diff revision 1)
>   *
>   * @param aAccumulations - accumulation actions to perform
>   */
>  void AccumulateChildKeyed(const nsTArray<KeyedAccumulation>& aAccumulations);
>  
>  /**

You should also remove the associated comment.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
(Diff revision 1)
> -  }
> -
> -  Histogram *h;
> -  nsresult rv = internal_GetHistogramByEnumId(aId, &h);
> -  if (NS_SUCCEEDED(rv) && h) {
> -    internal_HistogramClear(*h, false);

Is internal_HistogramClear used by any other function in this file? If not, it should be removed as well.
'internal_HistogramClear' is used by 'internal_JSHistogram_Clear'. So the only change that I need to make now is to remove the comment associated with 'ClearHistogram'. Is it fine?

Now regarding publishing a patch for removing the comment is the following process correct?
 1)Remove the comment associated with 'ClearHistogram' locally and commit the change with the commit
   message ' Bug 1304735 - Remove comment associated with ClearHistogram '
 2)Push the changeset associated with this commit using ' hg push -c <ID of the changeset> review '
Yup! Just remove the comment. I think it would be a bit cleaner if you cleared out the comment in the original commit. I don't think we need two commits.

So long as you leave the mozreview id in a commit alone, the interdiffs will be updated nicely up on mozreview. Then we can get it landed.
Assignee: nobody → ajay.krishn97
Attachment #8793843 - Flags: review?(gfritzsche)
Attachment #8793843 - Flags: review?(gfritzsche)
I removed the comment as well, in the updated commit. Could the reviewers review the commit.

Thanks
Comment on attachment 8793843 [details]
Bug 1304735 - Remove ClearHistogram

https://reviewboard.mozilla.org/r/80460/#review80330

LGTM
Attachment #8793843 - Flags: review+
Now that an 'r+' is given, am I supposed to remove 'gfritzsche' from reviewers list or it doesn't matter
Attachment #8793843 - Flags: review?(gfritzsche)
I've taken care of it. Soon, autolander should land this in the tree and everything will be done!
https://hg.mozilla.org/mozilla-central/rev/5f49c8956938
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Now that your code's in the tree, do you need help finding another good bug?
Flags: needinfo?(ajay.krishn97)
Right now I'm not planning on another bug fix. Is it okay if I ask for a good bug sometime later?
Thanks for all the help
Flags: needinfo?(ajay.krishn97)
Sure! Or, if I'm not around, you can browse for one that appeals to you. Good resources for introductory bugs are the lists of unassigned [good first bug] and [good second bug]-tagged bugs:

https://mzl.la/2cQGyUc
https://mzl.la/2dwMTCx
Okay
Thanks
You need to log in before you can comment on or make changes to this bug.