Re-prompt Chrome users to run the SRT when a finch param changes.

Re-prompt Chrome users to run the SRT when a finch param changes.

BUG=438287
Committed: https://crrev.com/de1df47e5aef43721d79a49d9fa554355ecf87cc
Cr-Commit-Position: refs/heads/master@{#313359}

[MAD, 2015-01-21 20:10:22]

Patchset #1 (id:1) has been deleted

[MAD, 2015-01-21 20:10:47]

mad@chromium.org changed reviewers:
+ macourteau@chromium.org

[MAD, 2015-01-21 20:10:48]

Here's the other CL I was talking about.

Thanks!

BYE
MAD

[macourteau, 2015-01-21 20:53:14]

lgtm w/ nits (and a question)

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:178: bool
valid_new_seed = seed_iter != params.end() && !seed_iter->second.empty();
nit: this doesn't mean there is necessarily a new seed, just whether there is a
valid seed. Maybe just "valid_seed"? Or "has_valid_seed"?

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:199: browser = new
Browser(Browser::CreateParams(profile, desktop_type));
Any chance this can leak? (not familiar with Browser, maybe it's lifetime is
being managed elsewhere and I don't know)

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:432:
prefs::kSwReporterPromptSeed, "",
nit: Maybe keep the "" on a separate line for consistency (or change the others
above)?

[MAD, 2015-01-23 04:34:02]

mad@chromium.org changed reviewers:
+ gab@chromium.org, jwd@chromium.org, waffles@chromium.org

[MAD, 2015-01-23 04:34:03]

Thanks all addressed, now own to owners:

waffles@ for sw_reporter_installer_win
jwd@ for histograms.xml
gab@ for pref service factory

Thanks!

BYE
MAD

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
File chrome/browser/component_updater/sw_reporter_installer_win.cc (right):

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:178: bool
valid_new_seed = seed_iter != params.end() && !seed_iter->second.empty();
On 2015/01/21 20:53:14, macourteau wrote:
> nit: this doesn't mean there is necessarily a new seed, just whether there is
a
> valid seed. Maybe just "valid_seed"? Or "has_valid_seed"?

I was trying to differentiate the Finch incoming seed to the persisted one.
Changed it to incoming to make it clearer.

Thanks!

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:199: browser = new
Browser(Browser::CreateParams(profile, desktop_type));
On 2015/01/21 20:53:14, macourteau wrote:
> Any chance this can leak? (not familiar with Browser, maybe it's lifetime is
> being managed elsewhere and I don't know)

This opens a window that the user will close. So we are OK, no leaks.

https://codereview.chromium.org/856393005/diff/20001/chrome/browser/component...
chrome/browser/component_updater/sw_reporter_installer_win.cc:432:
prefs::kSwReporterPromptSeed, "",
On 2015/01/21 20:53:14, macourteau wrote:
> nit: Maybe keep the "" on a separate line for consistency (or change the
others
> above)?

git cl format did that, so I updated the others.

[jwd, 2015-01-23 16:25:45]

histograms.xml LGTM

[waffles, 2015-01-23 16:38:20]

Do you mind rebasing after refs/heads/master@{#312652}? I see some overlap
there.

[MAD, 2015-01-24 00:19:19]

Done, but now the other patch has been reverted... ;-(

Stil, I'll fix and re-commit the other patch before committing this one...

Thanks!

BYE
MAD

[waffles, 2015-01-24 02:02:03]

lgtm, thanks!

[MAD, 2015-01-24 02:15:08]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2015-01-24 02:16:16]

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

[commit-bot: I haz the power, 2015-01-24 02:18:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-24 02:18:43]

Try jobs failed on following builders:
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[MAD, 2015-01-24 03:02:33]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2015-01-24 03:03:33]

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

[commit-bot: I haz the power, 2015-01-24 03:08:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-24 03:08:47]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MAD, 2015-01-24 03:27:45]

Oups... Still missing Gab's approval for the change in the pref service
factory...

BYE
MAD

[gab, 2015-01-26 13:29:12]

Sorry for the delay.

https://codereview.chromium.org/856393005/diff/80001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/856393005/diff/80001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:84: {0,
I know this is the probably the new format of git cl format, but I'd prefer if
such a simple change didn't touch so many things in this file. Per the typical
"follow local style" Chromium style rule that rules them all, could you please
add your new addition at the bottom by following the current style on trunk for
kTrackedPrefs[] without having to touch every other pref here?

https://codereview.chromium.org/856393005/diff/80001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/856393005/diff/80001/components/component_upd...
components/component_updater/pref_names.cc:35: // A value uniquely identifying
an SRTPrompt campaign so that users that have
s/value/string (or "string value") -- i.e., make sure it's obvious from this
comment that this is a string pref (I had to go look in the code to find how it
was used to derive its type).

[MAD, 2015-01-27 19:34:19]

All done... Anything else?

Thanks!

BYE
MAD

https://codereview.chromium.org/856393005/diff/80001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/856393005/diff/80001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:84: {0,
On 2015/01/26 13:29:12, gab wrote:
> I know this is the probably the new format of git cl format, but I'd prefer if
> such a simple change didn't touch so many things in this file. Per the typical
> "follow local style" Chromium style rule that rules them all, could you please
> add your new addition at the bottom by following the current style on trunk
for
> kTrackedPrefs[] without having to touch every other pref here?

OK, I disagree, and expressed my disagreement, but you the owner... ;-)

https://codereview.chromium.org/856393005/diff/80001/components/component_upd...
File components/component_updater/pref_names.cc (right):

https://codereview.chromium.org/856393005/diff/80001/components/component_upd...
components/component_updater/pref_names.cc:35: // A value uniquely identifying
an SRTPrompt campaign so that users that have
On 2015/01/26 13:29:12, gab wrote:
> s/value/string (or "string value") -- i.e., make sure it's obvious from this
> comment that this is a string pref (I had to go look in the code to find how
it
> was used to derive its type).

Done.

[gab, 2015-01-27 19:53:09]

lgtm w/ nit.

Thanks,
Gab

https://codereview.chromium.org/856393005/diff/120001/chrome/browser/prefs/ch...
File chrome/browser/prefs/chrome_pref_service_factory.cc (right):

https://codereview.chromium.org/856393005/diff/120001/chrome/browser/prefs/ch...
chrome/browser/prefs/chrome_pref_service_factory.cc:223:
prefs::kSwReporterPromptSeed,
nit: For consistency with other entries in this list, do not wrap this second
parameter.

I understand why you disagree (i.e. prefer git cl format everywhere), but the
current format makes this list easy to read (every item is formatted the exact
same way and it's easy to find what you're looking for); whereas git cl format
blindly packs parameters which makes some items packed differently than others
based on parameter length in this case...

Nice/consistent/human readable argument packing is not really a thing that a
tool like git cl format can do easily.

I have seen numerous times from Chromium OWNERS (e.g. pkasting) on chromium-dev
that human readability in some cases should be favored over git cl format's
output when both respect the style rules. I think this is such a case.

And for your argument that the next person changing this code and running git cl
format will run in the same thing, true, but by then git cl format may have
changed which would reformat this entire list again anyways. The recommendation
I've thus seen on chromium-dev to avoid that is to stick to the style in a local
file and not generate git cl format churn (i.e. this is the exact reason why we
don't git cl format the entire repo).

[MAD, 2015-01-27 20:00:57]

Oups, sorry, this slipped as a git cl format leftover... 

Would be so much simpler if we could just use what git cl format does, and then
be done with it... But hey... as I said... you the owner... ;)

Thanks!

BYE
MAD

[MAD, 2015-01-27 20:01:02]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2015-01-27 20:01:45]

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

[gab, 2015-01-27 20:16:03]

On 2015/01/27 20:00:57, MAD wrote:
> Oups, sorry, this slipped as a git cl format leftover... 
> 
> Would be so much simpler if we could just use what git cl format does, and
then
> be done with it... But hey... as I said... you the owner... ;)

Here's a thread worth reading if you're truly passionate about this:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/EyN2_85BTLc/dis...

I'm trying my best to echo the community's belief here.

Cheers,
Gab

[commit-bot: I haz the power, 2015-01-27 21:25:06]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-01-27 21:26:49]

Patchset 7 (id:??) landed as
https://crrev.com/de1df47e5aef43721d79a49d9fa554355ecf87cc
Cr-Commit-Position: refs/heads/master@{#313359}