Web MIDI: add a new UMA entry for the final result code

Web MIDI: add a new UMA entry for the final result code

This change adds a new UMA entry for the final MidiManager
result code on browser shutdown.

To report the result correctly, I assigned new numbers for
the result code. As a side effect, this patch can fix an
existing problem that the result code for NOT_INITIALIZED
was not propagated to renderer side correctly.

BUG=465661
Committed: https://crrev.com/eb09fc13169f631cfd610af9fabc3a9e3311ec1a
Cr-Commit-Position: refs/heads/master@{#338148}

[Takashi Toyoshima, 2015-07-06 14:57:39]

Patchset #1 (id:1) has been deleted

[Takashi Toyoshima, 2015-07-06 15:47:58]

Patchset #1 (id:20001) has been deleted

[Takashi Toyoshima, 2015-07-06 16:17:16]

Patchset #1 (id:40001) has been deleted

[Takashi Toyoshima, 2015-07-06 16:22:48]

toyoshim@chromium.org changed reviewers:
+ yhirano@chromium.org

[Takashi Toyoshima, 2015-07-06 16:23:10]

Can you take a look?

[yhirano, 2015-07-07 04:11:39]

> Original code had a problem that NOT_INITIALIZED was not propagated to
renderer side correctly.

Is the issue related to bug 465661? Or is there another bug entry?

[Takashi Toyoshima, 2015-07-07 06:17:23]

It was a separate issue that I find by chance in the way to add the new UMA.
Maybe I should change the subject to be aligned with the bug ID.

[Takashi Toyoshima, 2015-07-07 06:21:09]

How about the new description?

[yhirano, 2015-07-07 07:05:21]

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.cc:20: : initialized_(false),
result_(Result::NOT_SUPPORTED) {
Perhaps not related with this cl, but should this be NOT_INITIALIZED?

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.h:113: // with MIDI_NOT_SUPPORTED on the caller thread.
A derived class for a
rename?

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.h:173: // completed. Otherwise keeps MIDI_NOT_SUPPORTED.
rename?

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h
File media/midi/result.h (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h#new...
media/midi/result.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
2015

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h#new...
media/midi/result.h:18: // assigned value.
tools/metrics/histograms/histograms.xml contains MidiResult
s/MidiResult/Result

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17281: +    The final result code of
MidiManager on browser shutdown.
[optional] I like "final status".

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:61335: +  <int value="1"
label="Succeeded"/>
[optional] successfully initialized?

[Takashi Toyoshima, 2015-07-09 05:02:54]

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.cc:20: : initialized_(false),
result_(Result::NOT_SUPPORTED) {
Oh, good catch. MidiManager set NOT_SUPPORTED in StartInitialization for
unsupported platforms, so this should be NOT_INITIALIZED.

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.h:113: // with MIDI_NOT_SUPPORTED on the caller thread.
A derived class for a
On 2015/07/07 07:05:21, yhirano wrote:
> rename?

Done.

https://codereview.chromium.org/1217853007/diff/80001/media/midi/midi_manager...
media/midi/midi_manager.h:173: // completed. Otherwise keeps MIDI_NOT_SUPPORTED.
On 2015/07/07 07:05:21, yhirano wrote:
> rename?

Done.

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h
File media/midi/result.h (right):

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h#new...
media/midi/result.h:1: // Copyright 2014 The Chromium Authors. All rights
reserved.
On 2015/07/07 07:05:21, yhirano wrote:
> 2015

Done.

https://codereview.chromium.org/1217853007/diff/80001/media/midi/result.h#new...
media/midi/result.h:18: // assigned value.
tools/metrics/histograms/histograms.xml contains MidiResult
On 2015/07/07 07:05:21, yhirano wrote:
> s/MidiResult/Result

Done.

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:17281: +    The final result code of
MidiManager on browser shutdown.
On 2015/07/07 07:05:21, yhirano wrote:
> [optional] I like "final status".

Done.

https://codereview.chromium.org/1217853007/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:61335: +  <int value="1"
label="Succeeded"/>
On 2015/07/07 07:05:21, yhirano wrote:
> [optional] successfully initialized?

Done.

[Takashi Toyoshima, 2015-07-09 05:03:17]

toyoshim@chromium.org changed reviewers:
+ isherman@chromium.org

[Takashi Toyoshima, 2015-07-09 05:03:56]

toyoshim@chromium.org changed reviewers:
+ palmer@chromium.org
- isherman@chromium.org

[Takashi Toyoshima, 2015-07-09 05:04:34]

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

[yhirano, 2015-07-09 05:05:45]

lgtm

[Takashi Toyoshima, 2015-07-09 05:06:39]

+asvitkine for tools/metrics/histograms/histograms.xml
Relevant UMA change is in media/midi/midi_manager.cc.

+palmer for content/common/media/midi_messages.h
This is almost mechanical change.

[Alexei Svitkine (slow), 2015-07-09 17:11:27]

https://codereview.chromium.org/1217853007/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.cc:16: const char kMidiResultOnShutdown[] =
"Midi.ResultOnShutdown";
Nit: If it's only used in one place, it's fine to inline the histogram name into
the macro.

https://codereview.chromium.org/1217853007/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1217853007/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:17374: +<histogram
name="Midi.ResultOnShutdown" enum="MidiResult">
This seems to be introducing a new top-level prefix. Can it re-use an existing
one instead?

For example, Media.Midi.ResultOnShutdown?

[Takashi Toyoshima, 2015-07-09 17:43:30]

done

https://codereview.chromium.org/1217853007/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/120001/media/midi/midi_manage...
media/midi/midi_manager.cc:16: const char kMidiResultOnShutdown[] =
"Midi.ResultOnShutdown";
I prefer to use const variable to represent a magic number or string. But
anyway, I'll follow your suggestion here.

https://codereview.chromium.org/1217853007/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1217853007/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:17374: +<histogram
name="Midi.ResultOnShutdown" enum="MidiResult">
On 2015/07/09 17:11:27, Alexei Svitkine wrote:
> This seems to be introducing a new top-level prefix. Can it re-use an existing
> one instead?
> 
> For example, Media.Midi.ResultOnShutdown?

Done.

https://codereview.chromium.org/1217853007/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.cc:22: static_cast<int>(Result::MAX) + 1);
Hum... presubmit check has a special rule for
UMA_HISTOGRAM_ENUMERATION("Media.*", ...), and it requires "+1" here.

[Alexei Svitkine (slow), 2015-07-09 18:05:27]

lgtm

https://codereview.chromium.org/1217853007/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1217853007/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.cc:22: static_cast<int>(Result::MAX) + 1);
On 2015/07/09 17:43:30, Takashi Toyoshima (chromium) wrote:
> Hum... presubmit check has a special rule for
> UMA_HISTOGRAM_ENUMERATION("Media.*", ...), and it requires "+1" here.

Yeah, the media convention is to do it this way, which differs from the rest of
the codebase. I see you updated the enum, so this is fine.

[palmer, 2015-07-09 18:42:37]

IPC LGTM.

[Takashi Toyoshima, 2015-07-09 19:48:08]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-07-09 19:48:08]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1217853007/#ps140001
(title: "review #16")

[commit-bot: I haz the power, 2015-07-09 19:49:23]

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

[commit-bot: I haz the power, 2015-07-09 21:15:03]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-07-09 21:16:20]

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