Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(222)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 747093002: Modify names of PipelineStatus histograms (Closed)

Created:
6 years, 1 month ago by prabhur1
Modified:
6 years ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Modify names of PipelineStatus histograms R=dalecurtis BUG=435370 Committed: https://crrev.com/dc15c40eee58d869650a919878644e8d8faaf4da Cr-Commit-Position: refs/heads/master@{#305309}

Patch Set 1 #

Total comments: 1

Patch Set 2 : Fix CL comments #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -16 lines) Patch
M content/browser/media/media_internals.cc View 1 1 chunk +7 lines, -7 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +10 lines, -9 lines 3 comments Download

Messages

Total messages: 11 (3 generated)
prabhur1
These histograms were added last week. We are ok if there is data loss due ...
6 years, 1 month ago (2014-11-21 19:12:13 UTC) #2
Ilya Sherman
LGTM
6 years, 1 month ago (2014-11-21 20:25:41 UTC) #3
DaleCurtis
lgtm % up to you nit. https://codereview.chromium.org/747093002/diff/1/content/browser/media/media_internals.cc File content/browser/media/media_internals.cc (right): https://codereview.chromium.org/747093002/diff/1/content/browser/media/media_internals.cc#newcode319 content/browser/media/media_internals.cc:319: UMA_HISTOGRAM_ENUMERATION("Media.PipelineStatus.AudioVideo.Others", Other?
6 years, 1 month ago (2014-11-21 20:43:09 UTC) #4
prabhur1
Thanks for the quick reviews! Checking in.
6 years, 1 month ago (2014-11-21 21:48:59 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/747093002/20001
6 years, 1 month ago (2014-11-21 21:51:00 UTC) #7
commit-bot: I haz the power
Committed patchset #2 (id:20001)
6 years, 1 month ago (2014-11-21 22:53:12 UTC) #8
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/dc15c40eee58d869650a919878644e8d8faaf4da Cr-Commit-Position: refs/heads/master@{#305309}
6 years, 1 month ago (2014-11-21 22:53:49 UTC) #9
ddorwin
6 years ago (2014-11-25 01:14:21 UTC) #11
Message was sent while issue was closed. 
Comments on the descriptions, which doesn't affect the actual UMAs.

https://codereview.chromium.org/747093002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/747093002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58012: label="PipelineStatus for
hardware decoded AV streams with H264
nit: This is inconsistent with the VPx ones.
nit: Technically, "H.264".

https://codereview.chromium.org/747093002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/747093002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58007: +      label="PipelineStatus for
the codecs that dont have an explicit metric."/>
don't

https://codereview.chromium.org/747093002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58009: +      label="PipelineStatus for
AV streams with VP8 Software decoder."/>
nit: _s_oftware
nit: Probably missing "decoded" before "with". Though if we can make these
labels fit on one line, that's good.

Powered by Google App Engine
This is Rietveld 408576698