Small fixes to SRTPRompt.

Small fixes to SRTPrompt.

Add histograms.xml info for the SRTPrompt, and don't create a browser if we are not going to prompt the user.

TBR=noelutz
For a simple histogram string rename in chrome\browser\safe_browsing\srt_global_error_win.cc

Committed: https://crrev.com/38ada396f91e25cb09e85e64b0120dcda2cc99d0
Cr-Commit-Position: refs/heads/master@{#296858}

[MAD, 2014-09-25 16:42:28]

mad@chromium.org changed reviewers:
+ csharp@chromium.org, gab@chromium.org

[csharp, 2014-09-25 16:51:52]

lgtm

[gab, 2014-09-25 16:55:33]

Reporter updates lgtm.

Thanks!
Gab

[MAD, 2014-09-25 19:30:25]

mad@chromium.org changed reviewers:
+ asvitkine@chromium.org, waffles@chromium.org

[MAD, 2014-09-25 19:30:26]

OWNER approvals please?

Trivial change in sw reporter component and adding missing histograms.xml
changes from previous patch: https://codereview.chromium.org/599653002/

Thanks...

BYE
MAD
P.S.: waffles@ I have another trivial change coming to the same file, which is
to add a field trial gate to running the reporter for non UMA users, and
launching the SRT Prompt (https://codereview.chromium.org/599633005). So you may
want to do both at the same time if it's more convenient for you...

[Alexei Svitkine (slow), 2014-09-25 19:34:05]

asvitkine@chromium.org changed reviewers:
+ jwd@chromium.org

[Alexei Svitkine (slow), 2014-09-25 19:34:05]

Jesse, could you help with this review?

[Alexei Svitkine (slow), 2014-09-25 19:34:18]

asvitkine@chromium.org changed reviewers:
- asvitkine@chromium.org

[waffles, 2014-09-25 19:55:37]

component updater lgtm

[Alexei Svitkine (slow), 2014-09-25 20:54:39]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-09-25 20:54:40]

https://codereview.chromium.org/607573002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/607573002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:32696: +<histogram
name="SoftwareReporter.MajorVersion">
I don't see the code that's recording these histograms. Am I missing something?

[MAD, 2014-09-25 21:10:00]

As mentioned in the origin OWNER approval request message:

[...] adding missing histograms.xml changes from previous patch:
https://codereview.chromium.org/599653002/

Thanks!


On Thu, Sep 25, 2014 at 4:54 PM, <asvitkine@chromium.org> wrote:

>
> https://codereview.chromium.org/607573002/diff/1/tools/
> metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/607573002/diff/1/tools/
> metrics/histograms/histograms.xml#newcode32696
> tools/metrics/histograms/histograms.xml:32696: +<histogram
> name="SoftwareReporter.MajorVersion">
> I don't see the code that's recording these histograms. Am I missing
> something?
>
> https://codereview.chromium.org/607573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-09-25 21:19:13]

Sorry, missed that.

Some comments below. If you're worried about the branch point, feel free to
split the histograms changes to a separate CL and land the code changes of this
CL before then (histograms.xml gets picked up asynchronously from the branch
point and applies to all branches). But also happy to take another look at this
tomorrow, so if that works for you, then can keep in the same CL.

https://codereview.chromium.org/607573002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/607573002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:32708: +  <summary>The last component of
the version of the software reporter.</summary>
For all of these descriptions, please mention when the value is logged. Is it
every time the tool is run? Is it on start up of the browser? etc.

https://codereview.chromium.org/607573002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:32891: +<histogram name="SRTPrompt"
enum="SRTPrompt">
Ugh, is there a reason you named the histogram thus? Maybe it's not too late to
rename it to a better name (i.e. Softwarereporter.PromptUsage), if you don't
care about the minimal data we saw from the past 2 days after you landed your
other CL.

[MAD, 2014-09-25 21:32:14]

OK, thanks... I'll split the histograms.xml changes out of this CL, and
rename the histogram in the code that I will commit...

Will send you the other patch for the histogram that you could review
tomorrow... I need to have the other code, and proper histogram name in the
code in before EOD... /methinks...



On Thu, Sep 25, 2014 at 5:19 PM, <asvitkine@chromium.org> wrote:

> Sorry, missed that.
>
> Some comments below. If you're worried about the branch point, feel free to
> split the histograms changes to a separate CL and land the code changes of
> this
> CL before then (histograms.xml gets picked up asynchronously from the
> branch
> point and applies to all branches). But also happy to take another look at
> this
> tomorrow, so if that works for you, then can keep in the same CL.
>
>
> https://codereview.chromium.org/607573002/diff/1/tools/
> metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/607573002/diff/1/tools/
> metrics/histograms/histograms.xml#newcode32708
> tools/metrics/histograms/histograms.xml:32708: +  <summary>The last
> component of the version of the software reporter.</summary>
> For all of these descriptions, please mention when the value is logged.
> Is it every time the tool is run? Is it on start up of the browser? etc.
>
> https://codereview.chromium.org/607573002/diff/1/tools/
> metrics/histograms/histograms.xml#newcode32891
> tools/metrics/histograms/histograms.xml:32891: +<histogram
> name="SRTPrompt" enum="SRTPrompt">
> Ugh, is there a reason you named the histogram thus? Maybe it's not too
> late to rename it to a better name (i.e. Softwarereporter.PromptUsage),
> if you don't care about the minimal data we saw from the past 2 days
> after you landed your other CL.
>
> https://codereview.chromium.org/607573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-09-25 21:35:41]

SG, thanks!

On Thu, Sep 25, 2014 at 5:31 PM, Marc-Andre Decoste <mad@chromium.org>
wrote:

> OK, thanks... I'll split the histograms.xml changes out of this CL, and
> rename the histogram in the code that I will commit...
>
> Will send you the other patch for the histogram that you could review
> tomorrow... I need to have the other code, and proper histogram name in the
> code in before EOD... /methinks...
>
>
>
> On Thu, Sep 25, 2014 at 5:19 PM, <asvitkine@chromium.org> wrote:
>
>> Sorry, missed that.
>>
>> Some comments below. If you're worried about the branch point, feel free
>> to
>> split the histograms changes to a separate CL and land the code changes
>> of this
>> CL before then (histograms.xml gets picked up asynchronously from the
>> branch
>> point and applies to all branches). But also happy to take another look
>> at this
>> tomorrow, so if that works for you, then can keep in the same CL.
>>
>>
>> https://codereview.chromium.org/607573002/diff/1/tools/
>> metrics/histograms/histograms.xml
>> File tools/metrics/histograms/histograms.xml (right):
>>
>> https://codereview.chromium.org/607573002/diff/1/tools/
>> metrics/histograms/histograms.xml#newcode32708
>> tools/metrics/histograms/histograms.xml:32708: +  <summary>The last
>> component of the version of the software reporter.</summary>
>> For all of these descriptions, please mention when the value is logged.
>> Is it every time the tool is run? Is it on start up of the browser? etc.
>>
>> https://codereview.chromium.org/607573002/diff/1/tools/
>> metrics/histograms/histograms.xml#newcode32891
>> tools/metrics/histograms/histograms.xml:32891: +<histogram
>> name="SRTPrompt" enum="SRTPrompt">
>> Ugh, is there a reason you named the histogram thus? Maybe it's not too
>> late to rename it to a better name (i.e. Softwarereporter.PromptUsage),
>> if you don't care about the minimal data we saw from the past 2 days
>> after you landed your other CL.
>>
>> https://codereview.chromium.org/607573002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MAD, 2014-09-26 00:07:54]

mad@chromium.org changed reviewers:
+ noelutz@chromium.org
- asvitkine@chromium.org, jwd@chromium.org

[MAD, 2014-09-26 00:08:13]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2014-09-26 00:11:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/607573002/20001

[noé, 2014-09-26 00:19:23]

lgtm

[commit-bot: I haz the power, 2014-09-26 02:15:45]

Committed patchset #2 (id:20001) as 9af80fe10b9e4f30c717b2edbe692aecbd7644a2

[commit-bot: I haz the power, 2014-09-26 02:16:25]

Patchset 2 (id:??) landed as
https://crrev.com/38ada396f91e25cb09e85e64b0120dcda2cc99d0
Cr-Commit-Position: refs/heads/master@{#296858}