Simplify and shrink the UMA reporting code for mixed content.

Simplify and shrink the UMA reporting code for mixed content.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107807

[palmer, 2011-10-27 22:40:10]

This new version builds an array of signals to send, and then sends them in a
loop. This avoids expanding the UMA_HISTOGRAM_ENUMERATION macro many times,
making the source and object code smaller and simpler.

[jar (doing other things), 2011-10-27 22:58:12]

The repeating of the macro N times is a waste... but IMO you gain little by your
second step.  The pre-aggregating of the planned addition to the histogram into
the signal[] vector makes this code much harder to read, and is probably a
minimal (or non-existant) perf improvement.

I'd suggest just replacing all the old UMA_HISTOGRAM_ENUMERATION() call that
have a single histogram name with a call to a function, that contains one
instantiation of the HISTOGRAM macro.

It would be cleaner, more readable, and about as efficient.

[palmer, 2011-10-27 23:26:58]

> I'd suggest just replacing all the old UMA_HISTOGRAM_ENUMERATION() call that
> have a single histogram name with a call to a function, that contains one
> instantiation of the HISTOGRAM macro.

Done.

[jar (doing other things), 2011-10-27 23:41:31]

This looks much better IMO.

I was tempted to suggest that the repetitive if ... else if... else if ...
groups just assign a value, and then call the function once after each group
(rather than calling the function in each clause).  Truth is, the compiler may
do this... but... I think it might read better, and make it a tad easier to
read... I'm not sure.  WDYT?

In any case, there is a nit below to handle about curlies.

http://codereview.chromium.org/8400043/diff/3/chrome/renderer/chrome_render_v...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/8400043/diff/3/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.cc:476: else if (host ==
kPlusDotGoogleDotCom)
nit: start all these even lines with a close curly, and end them with an open
curly.  see note below.

http://codereview.chromium.org/8400043/diff/3/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.cc:530: } else if (host ==
kMailDotGoogleDotCom)
nit: Once you start using curlies, you need to use them throughout (i.e., when
used on an "if" clause, you must use it on the "else" clause.  In this case,
line 530 (and all even lines down to 548) should end with a open curly, and
should begin with a close curly.

[palmer, 2011-10-28 18:22:30]

> This looks much better IMO.

And thank you for the suggestion! I don't know what I was thinking.

> I was tempted to suggest that the repetitive if ... else if... else if ...
> groups just assign a value, and then call the function once after each group
> (rather than calling the function in each clause).  Truth is, the compiler may
> do this... but... I think it might read better, and make it a tad easier to
> read... I'm not sure.  WDYT?

I think it would not read as cleanly as the current state. It would be M
assignments + N calls to SendInsecureContentSignal, instead of just M calls to
SendInsecureContentSignal. Let's leave it as it is now.

> nit: Once you start using curlies, you need to use them throughout (i.e., when
> used on an "if" clause, you must use it on the "else" clause.  In this case,
> line 530 (and all even lines down to 548) should end with a open curly, and
> should begin with a close curly.

Done and done.

[jar (doing other things), 2011-10-28 18:39:03]

lgtm