Closed Bug 1304735 Opened 3 years ago Closed 3 years ago

Remove ClearHistogram


(Toolkit :: Telemetry, defect, P3)




Tracking Status
firefox52 --- fixed


(Reporter: chutten, Assigned: userajay0, Mentored)


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


(1 file)

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

So let's remove it.

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

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

::: 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.

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

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!
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:
You need to log in before you can comment on or make changes to this bug.