Log ErrorCode of Geoposition to UMA.

Log ErrorCode of Geoposition to UMA.

BUG=None

[Michael van Ouwerkerk, 2014-04-04 15:44:29]

Tim, could you please take a look at this as feature reviewer?

Ilya, could you please take a look at this as histograms owner?

[Ilya Sherman, 2014-04-04 21:55:46]

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:154:
Geoposition::ERROR_CODE_LAST);
I recommend against re-using an existing enumeration for metrics purposes,
especially since the existing enum is tied to an external specification.  Enums
used for histograms are required to be append-only over the lifetime of the
code, which is hard to guarantee in this case.  I'd recommend instead defining a
new enumeration specifically for metrics, with a simple conversion function to
translate from one to the other.

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:154:
Geoposition::ERROR_CODE_LAST);
For enumerated histograms, the final argument should be one-past-the-end of the
enum, rather than at the end of the enum.  This property allows you to add more
error codes over time, by separating out the overflow bucket from the bucket
that is currently the last one in the histogram.

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

https://codereview.chromium.org/226103003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:7095: +<histogram
name="Geolocation.GeolocationDispatcherHostImpl.ErrorCode"
"GeolocationDispatcherHostImpl" is not a very semantically contentful string in
the context of a metrics dashboard, nor is it super likely to remain unchanged
over the long term.  Is there perhaps a more conceptually clear name that you
could use for this metric?  Perhaps something like
"Geolocation.RendererRequest.ErrorCode"?

[Michael van Ouwerkerk, 2014-04-07 14:02:34]

Thanks Ilya! Please take another look.

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:154:
Geoposition::ERROR_CODE_LAST);
On 2014/04/04 21:55:47, Ilya Sherman wrote:
> I recommend against re-using an existing enumeration for metrics purposes,
> especially since the existing enum is tied to an external specification. 
Enums
> used for histograms are required to be append-only over the lifetime of the
> code, which is hard to guarantee in this case.  I'd recommend instead defining
a
> new enumeration specifically for metrics, with a simple conversion function to
> translate from one to the other.

Done.

https://codereview.chromium.org/226103003/diff/1/content/browser/geolocation/...
content/browser/geolocation/geolocation_dispatcher_host.cc:154:
Geoposition::ERROR_CODE_LAST);
On 2014/04/04 21:55:47, Ilya Sherman wrote:
> For enumerated histograms, the final argument should be one-past-the-end of
the
> enum, rather than at the end of the enum.  This property allows you to add
more
> error codes over time, by separating out the overflow bucket from the bucket
> that is currently the last one in the histogram.

Done.

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

https://codereview.chromium.org/226103003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:7095: +<histogram
name="Geolocation.GeolocationDispatcherHostImpl.ErrorCode"
On 2014/04/04 21:55:47, Ilya Sherman wrote:
> "GeolocationDispatcherHostImpl" is not a very semantically contentful string
in
> the context of a metrics dashboard, nor is it super likely to remain unchanged
> over the long term.  Is there perhaps a more conceptually clear name that you
> could use for this metric?  Perhaps something like
> "Geolocation.RendererRequest.ErrorCode"?

Done.

[Ilya Sherman, 2014-04-07 23:54:11]

Thanks.  LGTM with a nit:

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:62: NOTREACHED();
Please remove the default case, so that the compiler can issue a warning if a
case is forgotten.

[timvolodine, 2014-04-08 12:35:56]

lgtm

feature good ;)

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:36: // Geoposition
could not be determined
nit: period at the eol.

[Michael van Ouwerkerk, 2014-04-08 13:01:40]

Thanks for the reviews!

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
File content/browser/geolocation/geolocation_dispatcher_host.cc (right):

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:36: // Geoposition
could not be determined
On 2014/04/08 12:35:57, timvolodine wrote:
> nit: period at the eol.

Done.

https://codereview.chromium.org/226103003/diff/40001/content/browser/geolocat...
content/browser/geolocation/geolocation_dispatcher_host.cc:62: NOTREACHED();
On 2014/04/07 23:54:12, Ilya Sherman wrote:
> Please remove the default case, so that the compiler can issue a warning if a
> case is forgotten.

Done.

[Michael van Ouwerkerk, 2014-04-08 13:01:45]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-04-08 13:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/226103003/8...

[commit-bot: I haz the power, 2014-04-08 15:08:30]

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

[commit-bot: I haz the power, 2014-04-08 15:08:30]

Couldn't make sense out of svn commit message:
Sending        content/browser/geolocation/geolocation_dispatcher_host.cc
Sending        tools/metrics/histograms/histograms.xml
Transmitting file data ..
Committed revision 262412.

Warning: post-commit hook failed (exit code 2) with output:
/opt/shared_hooks/post-commit.d/10_mailer.py:137: DeprecationWarning: The popen2
module is deprecated.  Use the subprocess module.
  import popen2
Post-commit mail timed out for r262412. Your commit succeeded,
but no mail was sent.