1365526 - Refactor TelemetryHistogram::Accumulate* functions to share the logic

Status: NEW

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from bug 1343855 comment #22)
> @@ +2032,5 @@
> >    }
> >  
> > +  // Check if we're allowed to record in the provided key, for this histogram.
> > +  if (!gHistograms[aID].allows_key(aKey.get())) {
> > +    LogToBrowserConsole(nsIScriptError::errorFlag,
> 
> Both this and the function below end up calling `internal_Accumulate()` and
> the logic is duplicated.
> But i see that this ends up requiring passing status codes around.
> 
> We should really refactor the status/warning/error flow here, but that can
> happen around bug 1278821 (as a follow-up or blocker).
> Can you file something?
> 
> [...]
> 
> Right, i see. The key check should probably still live inside `internal_Accumulate()`.
> If we do this now though, then you can't log errors directly.
> So you would need to pass status codes out to print them from the non-locked part, similar to how the scalars do 3-phase JS function implementation.
> This sounds like a follow-up after bug 1278821.

The function Georg is talking about are here: http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/toolkit/components/telemetry/TelemetryHistogram.cpp#1976,1989