1304735 - Remove ClearHistogram

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Chris H-C :chutten]

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

Comment 1

[Ajay]

Attachment: Bug 1304735 - Remove ClearHistogram

Review commit: https://reviewboard.mozilla.org/r/80460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80460/

Comment 2

[Ajay]

Hello, I have submitted a patch for the Bug 1304735.
Could the reviewers review the patch and notify me of any issues

Thanks

Comment 3

[Chris H-C :chutten]

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.

Comment 4

[Ajay]

'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 '

Comment 5

[Chris H-C :chutten]

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.

Comment 6

[Ajay]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80460/diff/1-2/

Comment 7

[Ajay]

I removed the comment as well, in the updated commit. Could the reviewers review the commit.

Thanks

Comment 8

[Chris H-C :chutten]

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

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

LGTM

Comment 9

[Ajay]

Now that an 'r+' is given, am I supposed to remove 'gfritzsche' from reviewers list or it doesn't matter

Comment 10

[Chris H-C :chutten]

I've taken care of it. Soon, autolander should land this in the tree and everything will be done!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f49c8956938
Remove ClearHistogram r=chutten

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f49c8956938

Comment 13

[Chris H-C :chutten]

Now that your code's in the tree, do you need help finding another good bug?

Comment 14

[Ajay]

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

Comment 15

[Chris H-C :chutten]

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

Comment 16

[Ajay]

Okay
Thanks