Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1591453005:
Add metrics regarding concurrent audible tabs in Chromium. (Closed)
|
Created:
4 years, 11 months ago by mlamouri (slow - plz ping) Modified:
4 years, 11 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics regarding concurrent audible tabs in Chromium.
These metrics are keeping track of:
- whether there is another audible tab when a tab becomes audible;
- the maximum number of concurrent audible tab in a session;
- how long there are 2 or more audible tabs at the same time.
It is also recording when a tab gain or loses audible status.
BUG=578049
Committed: https://crrev.com/44e4ef42ba9104b06ae212042452dfc81d89032b
Cr-Commit-Position: refs/heads/master@{#370435}
Patch Set 1 #
Total comments: 35
Patch Set 2 : review comments and test fixes #Patch Set 3 : user action and tick #
Total comments: 6
Patch Set 4 : nits #
Total comments: 17
Patch Set 5 : review comments #Messages
Total messages: 41 (17 generated)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/1
mlamouri@chromium.org changed reviewers: + dalecurtis@chromium.org, isherman@chromium.org, jochen@chromium.org
Can you PTAL at: isherman@: tools/metrics/histograms/histograms.xml dalecurtis@: content/browser/media/audible_metrics.cc content/browser/media/audible_metrics.h content/browser/media/audible_metrics_unittest.cc content/browser/media/audio_stream_monitor.cc content/browser/media/audio_stream_monitor.h content/browser/media/media_web_contents_observer.cc content/browser/media/media_web_contents_observer.h jochen@: content/browser/web_contents/web_contents_impl.cc Thanks :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
Some drive-by nits https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:19: // It does register three different information: nit: s/%/It reports three different metrics/ ? https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:16: static WebContents* WEB_CONTENTS_0 = reinterpret_cast<WebContents*>(0x00); nit: perhaps you meant something like "const WebContents* const" ? https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:23: AudibleMetricsTest() = default; nit: is this needed? won't it have a default ctor without this? https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:33: clock_ = nullptr; nit: this is probably not needed https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:292: // No record because concurrent audbile tabs still running. nit: s/audbile/audible + one more occurrence below https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.h:57: // false as soon as the WebContents stop producing sound. hm, won't this create some noise in the metrics? should we count music playlist as one long audible tab or each track as a separate event? https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18208: + It is recording the maximum audible tab everytime it increases. In other nit: s/tab/tab count/ https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18216: + Records for how long multiple tabs have been audible at the same time. The nit: s/multiple tabs/more than one tab
dalecurtis@chromium.org changed reviewers: - avayvod@chromium.org
Nice cleanup and tests! Are you concerned about cases like multiple open streams in a single tab? That's a common use case for games, music generators, etc. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:29: void UpdateAudibleWebContentsState(WebContents* web_contents, bool audible); const all usage of WebContents* in this class. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:37: base::Time concurrent_web_contents_start_time_; TimeTicks https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:39: scoped_ptr<base::Clock> clock_; TickClock to avoid time going backwards issues https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:42: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:16: static WebContents* WEB_CONTENTS_0 = reinterpret_cast<WebContents*>(0x00); const these if possible https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:38: AudibleMetrics& audible_metrics() { Don't return non-const& -- use a pointer instead (unless this is now allowed as part of the blink-chromium style guide changes) https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:51: }; DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:57: AudibleMetrics* audible_metrics = new AudibleMetrics(); Just use a scoped_ptr? https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:115: "Media.Audible.ConcurrentTabsWhenStarting")); I think the histogram macros prevent you from using a shared public variable, but for all of these tests you should just make the names top-level consts. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/media... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/media... content/browser/media/media_web_contents_observer.cc:23: static base::LazyInstance<AudibleMetrics> g_audible_metrics = ::Leaky
Comments applied. PTAL. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:16: static WebContents* WEB_CONTENTS_0 = reinterpret_cast<WebContents*>(0x00); On 2016/01/15 at 19:02:03, DaleCurtis wrote: > const these if possible Switched to const. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:23: AudibleMetricsTest() = default; On 2016/01/15 at 19:01:20, whywhat wrote: > nit: is this needed? won't it have a default ctor without this? I need the default ctor. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:33: clock_ = nullptr; On 2016/01/15 at 19:01:20, whywhat wrote: > nit: this is probably not needed |clock_| is owned by |audible_metrics_|, I prefer to reset the pointer because after TearDown() it's pointing in an invalid memory space. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:38: AudibleMetrics& audible_metrics() { On 2016/01/15 at 19:02:03, DaleCurtis wrote: > Don't return non-const& -- use a pointer instead (unless this is now allowed as part of the blink-chromium style guide changes) Switched to a pointer. Might be allowed now but lets stay consistent ;) https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:51: }; On 2016/01/15 at 19:02:03, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:57: AudibleMetrics* audible_metrics = new AudibleMetrics(); On 2016/01/15 at 19:02:03, DaleCurtis wrote: > Just use a scoped_ptr? It looks a bit odd now but why not. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:115: "Media.Audible.ConcurrentTabsWhenStarting")); On 2016/01/15 at 19:02:03, DaleCurtis wrote: > I think the histogram macros prevent you from using a shared public variable, but for all of these tests you should just make the names top-level consts. Done. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics_unittest.cc:292: // No record because concurrent audbile tabs still running. On 2016/01/15 at 19:01:20, whywhat wrote: > nit: s/audbile/audible + one more occurrence below Done. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audio... File content/browser/media/audio_stream_monitor.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audio... content/browser/media/audio_stream_monitor.h:57: // false as soon as the WebContents stop producing sound. On 2016/01/15 at 19:01:20, whywhat wrote: > hm, won't this create some noise in the metrics? should we count music playlist as one long audible tab or each track as a separate event? Sure, this is going to have an effect on playlist. Though, it's better to use `IsCurrentlyAudible` instead of `WasRecentlyAudible` because otherwise, we will end up with false negative of concurrent audio output. For example, if I go pause example.com to play example2.com, we would record a concurrent sound which should really not be recorded as is. In the case of playlists, I think we will still see a user with a large number of concurrent sound. It will be chunks of 3-5 minutes instead of 30+ minutes but I think we should be able to read the data nonetheless. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/media... File content/browser/media/media_web_contents_observer.cc (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/media... content/browser/media/media_web_contents_observer.cc:23: static base::LazyInstance<AudibleMetrics> g_audible_metrics = On 2016/01/15 at 19:02:03, DaleCurtis wrote: > ::Leaky Done. https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18208: + It is recording the maximum audible tab everytime it increases. In other On 2016/01/15 at 19:01:20, whywhat wrote: > nit: s/tab/tab count/ Done. https://codereview.chromium.org/1591453005/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:18216: + Records for how long multiple tabs have been audible at the same time. The On 2016/01/15 at 19:01:20, whywhat wrote: > nit: s/multiple tabs/more than one tab Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
web_contents_impl.cc lgtm
I think you missed a batch of comments on the first patch set. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:37: base::Time concurrent_web_contents_start_time_; On 2016/01/15 19:02:03, DaleCurtis wrote: > TimeTicks Don't see any resolution or discussion of this comment or the TickClock one below it?
mlamouri@chromium.org changed reviewers: + mpearson@chromium.org - isherman@chromium.org
Dale, I've applied the comments I missed. Sorry about that and thank you for your patience :) Also, I've added actions metric recording. Swapping metrics review from isherman@ to mpearson@ because we discussed this. PTAL. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... File content/browser/media/audible_metrics.h (right): https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:29: void UpdateAudibleWebContentsState(WebContents* web_contents, bool audible); On 2016/01/15 at 19:02:03, DaleCurtis wrote: > const all usage of WebContents* in this class. Done. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:37: base::Time concurrent_web_contents_start_time_; On 2016/01/19 at 18:36:53, DaleCurtis wrote: > On 2016/01/15 19:02:03, DaleCurtis wrote: > > TimeTicks > > Don't see any resolution or discussion of this comment or the TickClock one below it? I think I missed the first file entirely. Sorry about that. I happened to have applied all comments but that one but it's just because I saw them while re-reading the patch :) Should be fixed now. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:39: scoped_ptr<base::Clock> clock_; On 2016/01/15 at 19:02:03, DaleCurtis wrote: > TickClock to avoid time going backwards issues Done. https://codereview.chromium.org/1591453005/diff/1/content/browser/media/audib... content/browser/media/audible_metrics.h:42: }; On 2016/01/15 at 19:02:03, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/40001
lgtm % some minor nits. https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... File content/browser/media/audible_metrics_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audible_metrics_unittest.cc:77: scoped_ptr<AudibleMetrics> audible_metrics; This does nothing, you want audible_metrics(new AudibleMetrics()); https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... File content/browser/media/audio_stream_monitor_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audio_stream_monitor_unittest.cc:104: void ExpectRecentlyAudbileChangeNotification(bool new_recently_audible) { Audible https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audio_stream_monitor_unittest.cc:117: void ExpectCurrentlyAudbileChangeNotification(bool new_audible) { Audible
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/60001
All comments applied. mpearson@, PTAL at tools/metrics/ and metrics usage. https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... File content/browser/media/audible_metrics_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audible_metrics_unittest.cc:77: scoped_ptr<AudibleMetrics> audible_metrics; On 2016/01/19 at 19:32:57, DaleCurtis wrote: > This does nothing, you want audible_metrics(new AudibleMetrics()); Stupid me :) https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... File content/browser/media/audio_stream_monitor_unittest.cc (right): https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audio_stream_monitor_unittest.cc:104: void ExpectRecentlyAudbileChangeNotification(bool new_recently_audible) { On 2016/01/19 at 19:32:57, DaleCurtis wrote: > Audible Done. https://codereview.chromium.org/1591453005/diff/40001/content/browser/media/a... content/browser/media/audio_stream_monitor_unittest.cc:117: void ExpectCurrentlyAudbileChangeNotification(bool new_audible) { On 2016/01/19 at 19:32:57, DaleCurtis wrote: > Audible Done.
Description was changed from ========== Add metrics regarding concurrent audible tabs in Chromium. These metrics are keeping track of: - whether there is another audible tab when a tab becomes audible; - the maximum number of concurrent audible tab in a session; - how long there are 2 or more audible tabs at the same time. BUG=578049 ========== to ========== Add metrics regarding concurrent audible tabs in Chromium. These metrics are keeping track of: - whether there is another audible tab when a tab becomes audible; - the maximum number of concurrent audible tab in a session; - how long there are 2 or more audible tabs at the same time. It is also recording when a tab gain or loses audible status. BUG=578049 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
a number of minor comments below. Have you tested this interactively using about:histograms? If not, you should. --mark https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... File content/browser/media/audible_metrics.cc (right): https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... content/browser/media/audible_metrics.cc:43: 0, 10, 11); the min bucket should be 1. (We always have an "underflow" bucket that contains 0.) https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... content/browser/media/audible_metrics.cc:59: 1, 10, 10); nit: second 10 here might as well be 11 for consistency with the above call. (in practice, it won't make much difference.) https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7723: <description>A new tab became audible.</description> omit "new"; it's slightly confusing and unnecessary. (Unless you are only emitting when a tab becomes audible for the first time. E.g., if a tab X starts playing sound, stops playing sound, then starts again. In that case, I expect you to record two user actions. If you only record one, we need to revise this description.) https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18258: +<histogram name="Media.Audible.ConcurrentTabsInSession"> Given when this histogram is emitted, it should have Maximum in the name somewhere. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18261: + Records how many tabs have been audible at the same time during the session. nit: have been -> are (or were) https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18263: + other words, a session with N concurrent audible tabs will record for [1;N]. nit: for [1,N]. -> each number 1 through N exactly once. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18270: + Records for how long more than one tab has been audible at the same time. nit: omit "for" https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18270: + Records for how long more than one tab has been audible at the same time. nit: has been -> is (or was) https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18272: + starts when two tabs are audible at the same time and stops when it is back two tabs are audible -> the user goes from one to two tabs audible
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/80001
mpearson@, I have applied your comments. PTAL. And thanks for about:histograms. I used to use logging in order to do some manual testing. about:histograms is significantly better :) https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... File content/browser/media/audible_metrics.cc (right): https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... content/browser/media/audible_metrics.cc:43: 0, 10, 11); On 2016/01/20 at 00:19:16, Mark P wrote: > the min bucket should be 1. > (We always have an "underflow" bucket that contains 0.) Done. https://codereview.chromium.org/1591453005/diff/60001/content/browser/media/a... content/browser/media/audible_metrics.cc:59: 1, 10, 10); On 2016/01/20 at 00:19:15, Mark P wrote: > nit: second 10 here might as well be 11 for consistency with the above call. > (in practice, it won't make much difference.) Done. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:7723: <description>A new tab became audible.</description> On 2016/01/20 at 00:19:16, Mark P wrote: > omit "new"; it's slightly confusing and unnecessary. > (Unless you are only emitting when a tab becomes audible for the first time. E.g., if a tab X starts playing sound, stops playing sound, then starts again. In that case, I expect you to record two user actions. If you only record one, we need to revise this description.) Good catch, I didn't mean to record only once per tab but when a tab becomes audible. Removed "new". https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18258: +<histogram name="Media.Audible.ConcurrentTabsInSession"> On 2016/01/20 at 00:19:16, Mark P wrote: > Given when this histogram is emitted, it should have Maximum in the name somewhere. Done. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18261: + Records how many tabs have been audible at the same time during the session. On 2016/01/20 at 00:19:16, Mark P wrote: > nit: have been -> are > (or were) Done. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18263: + other words, a session with N concurrent audible tabs will record for [1;N]. On 2016/01/20 at 00:19:16, Mark P wrote: > nit: for [1,N]. -> each number 1 through N exactly once. Done. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18270: + Records for how long more than one tab has been audible at the same time. On 2016/01/20 at 00:19:16, Mark P wrote: > nit: has been -> is > (or was) > nit: omit "for" Done for both. https://codereview.chromium.org/1591453005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18272: + starts when two tabs are audible at the same time and stops when it is back On 2016/01/20 at 00:19:16, Mark P wrote: > two tabs are audible -> the user goes from one to two tabs audible Done with s/user/browser/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1591453005/#ps80001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591453005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591453005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics regarding concurrent audible tabs in Chromium. These metrics are keeping track of: - whether there is another audible tab when a tab becomes audible; - the maximum number of concurrent audible tab in a session; - how long there are 2 or more audible tabs at the same time. It is also recording when a tab gain or loses audible status. BUG=578049 ========== to ========== Add metrics regarding concurrent audible tabs in Chromium. These metrics are keeping track of: - whether there is another audible tab when a tab becomes audible; - the maximum number of concurrent audible tab in a session; - how long there are 2 or more audible tabs at the same time. It is also recording when a tab gain or loses audible status. BUG=578049 Committed: https://crrev.com/44e4ef42ba9104b06ae212042452dfc81d89032b Cr-Commit-Position: refs/heads/master@{#370435} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/44e4ef42ba9104b06ae212042452dfc81d89032b Cr-Commit-Position: refs/heads/master@{#370435} |
