Add UMA and Rappor metrics to capture URLs passed in at startup.

Add UMA and Rappor metrics to capture URLs passed in at startup.

BUG=520098
Committed: https://crrev.com/97f419951b4f4301a17266a39ba3f77cc4577ff4
Cr-Commit-Position: refs/heads/master@{#343205}

[rkaplow, 2015-08-12 15:19:30]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org, holte@chromium.org

[rkaplow, 2015-08-12 15:20:25]

On 2015/08/12 15:19:30, rkaplow wrote:
> mailto:rkaplow@chromium.org changed reviewers:
> + mailto:asvitkine@chromium.org, mailto:holte@chromium.org

Steve can you look at the rappor, alexei in general (and the new histogram
specifically). Haven't done a rappor metric so let me know if I'm omitting
something.

[Alexei Svitkine (slow), 2015-08-12 18:10:12]

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
!urls_to_open.empty());
If you're adding this, maybe just record the url count instead of the boolean?

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1026: for (auto& url :
urls_to_open) {
Nit: const auto& url or write it out completely - const GURL& url, since it's
short.

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

https://codereview.chromium.org/1283243002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:43185: +    When Chrome is launched,
true if it was given a command line containing URLs
This is also logged if a running Chrome got a message to open the urls, right?
If so please clarify in the description - same for RAPPOR metric.

[Steven Holte, 2015-08-12 18:31:09]

I mostly just agree with Alexie's comments here.  Particularly, if this can be
hit by command line call reusing a running chrome, or some other method (Android
Intents/DBUS messages or something) it would be good to name and describe it in
a way that identifies that.  Could be as simple as referring to "Invokations"
rather than startups.

[rkaplow, 2015-08-12 19:54:18]

Thanks for feedback - the docs are much clearer.

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
!urls_to_open.empty());
On 2015/08/12 18:10:12, Alexei Svitkine wrote:
> If you're adding this, maybe just record the url count instead of the boolean?

Done. Probably it's not super necessary but it is strictly more informative so
seems reasonable.

https://codereview.chromium.org/1283243002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:1026: for (auto& url :
urls_to_open) {
On 2015/08/12 18:10:12, Alexei Svitkine wrote:
> Nit: const auto& url or write it out completely - const GURL& url, since it's
> short.

Done.

[Alexei Svitkine (slow), 2015-08-12 20:08:09]

lgtm

[rkaplow, 2015-08-12 20:09:35]

rkaplow@chromium.org changed reviewers:
+ thestig@chromium.org

[rkaplow, 2015-08-12 20:09:51]

On 2015/08/12 20:08:09, Alexei Svitkine wrote:
> lgtm

Lei can you review the startup_browser code?

[rkaplow, 2015-08-12 22:23:55]

rkaplow@chromium.org changed reviewers:
+ pkasting@chromium.org

[rkaplow, 2015-08-12 22:24:07]

On 2015/08/12 20:09:51, rkaplow wrote:
> On 2015/08/12 20:08:09, Alexei Svitkine wrote:
> > lgtm
> 
> Lei can you review the startup_browser code?

lei on vacation today - peter can you take a look?

[Peter Kasting, 2015-08-12 22:28:23]

LGTM

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
urls_to_open.size());
Nit: To avoid a (currently disabled) warning on MSVC< you should probably pass:

static_cast<base::HistogramBase::Sample>(urls_to_open.size())

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:155: void
RecordRapporOnStartupURLs(const std::vector<GURL>& urls_to_open);
Nit: If this vector type is used enough maybe it would make sense to have a
typedef for it somewhere.

[rkaplow, 2015-08-12 22:37:40]

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
urls_to_open.size());
On 2015/08/12 22:28:23, Peter Kasting wrote:
> Nit: To avoid a (currently disabled) warning on MSVC< you should probably
pass:
> 
> static_cast<base::HistogramBase::Sample>(urls_to_open.size())

The current convention for this macro is just a number. I would think it would
make sense to switch them all at once (if we want to do this) instead of
piecemeal.

Let me know if that is ok.

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.h:155: void
RecordRapporOnStartupURLs(const std::vector<GURL>& urls_to_open);
On 2015/08/12 22:28:23, Peter Kasting wrote:
> Nit: If this vector type is used enough maybe it would make sense to have a
> typedef for it somewhere.

Acknowledged. Seems to be only 3-4 places here so seems ok to leave as is.

[Peter Kasting, 2015-08-12 22:46:11]

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
urls_to_open.size());
On 2015/08/12 22:37:40, rkaplow wrote:
> On 2015/08/12 22:28:23, Peter Kasting wrote:
> > Nit: To avoid a (currently disabled) warning on MSVC< you should probably
> pass:
> > 
> > static_cast<base::HistogramBase::Sample>(urls_to_open.size())
> 
> The current convention for this macro is just a number. I would think it would
> make sense to switch them all at once (if we want to do this) instead of
> piecemeal.

I'm not sure if I'm being clear.  Passing a number is fine.  Passing a variable
is fine if the variable can be losslessly converted to Sample (which is a
typedef for int32).  The problem is that you're passing a size_t, which is not
guaranteed to fit in an int32, and that will trigger the warning.  Hence the
cast.

Few histogram callers pass size_ts, so this cast is rarely needed.  So this
isn't a matter of trying to systematically change something throughout the
codebase.

[rkaplow, 2015-08-13 02:11:58]

Will commit tomorrow if I don't hear any other responses. Thanks for reviews.

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1283243002/diff/20001/chrome/browser/ui/start...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:330:
urls_to_open.size());
On 2015/08/12 22:46:11, Peter Kasting wrote:
> On 2015/08/12 22:37:40, rkaplow wrote:
> > On 2015/08/12 22:28:23, Peter Kasting wrote:
> > > Nit: To avoid a (currently disabled) warning on MSVC< you should probably
> > pass:
> > > 
> > > static_cast<base::HistogramBase::Sample>(urls_to_open.size())
> > 
> > The current convention for this macro is just a number. I would think it
would
> > make sense to switch them all at once (if we want to do this) instead of
> > piecemeal.
> 
> I'm not sure if I'm being clear.  Passing a number is fine.  Passing a
variable
> is fine if the variable can be losslessly converted to Sample (which is a
> typedef for int32).  The problem is that you're passing a size_t, which is not
> guaranteed to fit in an int32, and that will trigger the warning.  Hence the
> cast.
> 
> Few histogram callers pass size_ts, so this cast is rarely needed.  So this
> isn't a matter of trying to systematically change something throughout the
> codebase.

Ah thanks, that makes sense.

[rkaplow, 2015-08-13 14:31:15]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2015-08-13 14:31:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1283243002/#ps40001
(title: "Cast")

[commit-bot: I haz the power, 2015-08-13 14:31:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1283243002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1283243002/40001

[commit-bot: I haz the power, 2015-08-13 16:02:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-13 16:02:46]

Patchset 3 (id:??) landed as
https://crrev.com/97f419951b4f4301a17266a39ba3f77cc4577ff4
Cr-Commit-Position: refs/heads/master@{#343205}