Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2142983002:
Add desktop engagement metrics. (Closed)
|
Created:
4 years, 5 months ago by gayane -on leave until 09-2017 Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd desktop engagement metrics.
Add desktop engagement metrics which records Chrome session length.
This metrics takes into account Chrome visibility, presence of audio and
user interaction to start and end sessions.
Parts of this CL are derived from the following CLs:
https://codereview.chromium.org/2127143002/ by chrisha@
https://codereview.chromium.org/2101743002/ by asvitkine@
https://codereview.chromium.org/2102263002/ by pmonette@
BUG=615080
Committed: https://crrev.com/c4053604844bed762906984ac304f82e2db787d5
Cr-Commit-Position: refs/heads/master@{#409603}
Patch Set 1 #Patch Set 2 : sync #Patch Set 3 : fix unittests #
Total comments: 12
Patch Set 4 : audio tests #
Total comments: 2
Patch Set 5 : audio tests refactor #
Total comments: 8
Patch Set 6 #
Total comments: 30
Patch Set 7 : address comments and fix mac bot #
Total comments: 22
Patch Set 8 #Patch Set 9 : fixes #
Total comments: 16
Patch Set 10 #
Total comments: 12
Patch Set 11 : DesktopEngagementService implements audio observer #
Total comments: 4
Patch Set 12 #
Total comments: 7
Patch Set 13 : remove unused import #Patch Set 14 : explicitly check for chromeos #
Total comments: 1
Patch Set 15 : fix build files and unittests #Patch Set 16 : sync #Patch Set 17 : fix chromeos build #
Total comments: 2
Patch Set 18 #Messages
Total messages: 94 (53 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Desktop engagement metrics ========== to ========== Desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Patched from chrisha@ CL https://codereview.chromium.org/2127143002/ Patched from asvitkine@ CL https://codereview.chromium.org/2101743002/ Patched from pmonette@ CL https://codereview.chromium.org/2102263002/ BUG=615080 ==========
gayane@chromium.org changed reviewers: + asvitkine@chromium.org, chrisha@chromium.org, pmonette@chromium.org
Patchset #3 (id:80001) has been deleted
Please have a look
Cool, just a few nits. Could you also make sure all classes have a small comment describing it? https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/audible_contents_tracker.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/audible_contents_tracker.cc:7: #include <set> Nit: Already included in header https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_visibility_observer.cc:16: const int kVisibilityGapTolerance = 5; // seconds, can make it a finch param. constexpr base::TimeDelta kVisibilityGapTolerance = base::TimeDelta::FromSeconds(5); The constant is only used in one function, maybe move it there? https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement_service.cc:16: const int kActivityIntervalMinutes = 5; Same.
avi@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.cc:219: DesktopEngagementObserver::CreateForWebContents(web_contents); Alphabetical order, plz?
Comments for all classes and functions would be nice. Also, AudibleContentsTracker could use a unittest :) https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_visibility_observer.cc:36: ChromeVisibilityObserver* ChromeVisibilityObserver::GetInstance() { Does this need to be a singleton? Maybe this can simply be owned by the DesktopEngagementService, like the AudibleContentsTracker? https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:80: static void NotifyBrowserNoLongerActive(Browser* browser); Comment?
Can you make a sub-directory under chrome/browser/metrics - e.g. chrome/browser/metrics/desktop_engagement - and put all the new classes you have in that subdirectory?
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by gayane@chromium.org
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
The CQ bit was unchecked by gayane@chromium.org
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
The CQ bit was unchecked by gayane@chromium.org
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:180001) has been deleted
Addressed all the comments. Moved new files under desktop_engagement folder and added a unittest for Audio tracker https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/audible_contents_tracker.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/audible_contents_tracker.cc:7: #include <set> On 2016/07/13 16:06:24, Patrick Monette wrote: > Nit: Already included in header Done. https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_visibility_observer.cc:16: const int kVisibilityGapTolerance = 5; // seconds, can make it a finch param. On 2016/07/13 16:06:24, Patrick Monette wrote: > constexpr base::TimeDelta kVisibilityGapTolerance = > base::TimeDelta::FromSeconds(5); > > The constant is only used in one function, maybe move it there? Logic changed a bit. Please have a look/ https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/chrome_visibility_observer.cc:36: ChromeVisibilityObserver* ChromeVisibilityObserver::GetInstance() { On 2016/07/14 13:58:48, chrisha (slow) wrote: > Does this need to be a singleton? Maybe this can simply be owned by the > DesktopEngagementService, like the AudibleContentsTracker? Agreed. It made more sense before. I have removed the singlton https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement_service.cc:16: const int kActivityIntervalMinutes = 5; On 2016/07/13 16:06:24, Patrick Monette wrote: > Same. I have changed this logic a bit. Please have a look. https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:80: static void NotifyBrowserNoLongerActive(Browser* browser); On 2016/07/14 13:58:48, chrisha (slow) wrote: > Comment? Done. https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... File chrome/browser/ui/tab_helpers.cc (right): https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... chrome/browser/ui/tab_helpers.cc:219: DesktopEngagementObserver::CreateForWebContents(web_contents); On 2016/07/13 18:40:58, Avi wrote: > Alphabetical order, plz? Done.
On 2016/07/20 20:22:03, gayane wrote: > Addressed all the comments. Moved new files under desktop_engagement folder and > added a unittest for Audio tracker > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > File chrome/browser/metrics/audible_contents_tracker.cc (right): > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > chrome/browser/metrics/audible_contents_tracker.cc:7: #include <set> > On 2016/07/13 16:06:24, Patrick Monette wrote: > > Nit: Already included in header > > Done. > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > File chrome/browser/metrics/chrome_visibility_observer.cc (right): > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > chrome/browser/metrics/chrome_visibility_observer.cc:16: const int > kVisibilityGapTolerance = 5; // seconds, can make it a finch param. > On 2016/07/13 16:06:24, Patrick Monette wrote: > > constexpr base::TimeDelta kVisibilityGapTolerance = > > base::TimeDelta::FromSeconds(5); > > > > The constant is only used in one function, maybe move it there? > > Logic changed a bit. Please have a look/ > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > chrome/browser/metrics/chrome_visibility_observer.cc:36: > ChromeVisibilityObserver* ChromeVisibilityObserver::GetInstance() { > On 2016/07/14 13:58:48, chrisha (slow) wrote: > > Does this need to be a singleton? Maybe this can simply be owned by the > > DesktopEngagementService, like the AudibleContentsTracker? > > Agreed. It made more sense before. I have removed the singlton > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > File chrome/browser/metrics/desktop_engagement_service.cc (right): > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/metrics... > chrome/browser/metrics/desktop_engagement_service.cc:16: const int > kActivityIntervalMinutes = 5; > On 2016/07/13 16:06:24, Patrick Monette wrote: > > Same. > > I have changed this logic a bit. Please have a look. > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_list.h (right): > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/brow... > chrome/browser/ui/browser_list.h:80: static void > NotifyBrowserNoLongerActive(Browser* browser); > On 2016/07/14 13:58:48, chrisha (slow) wrote: > > Comment? > > Done. > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... > File chrome/browser/ui/tab_helpers.cc (right): > > https://codereview.chromium.org/2142983002/diff/100001/chrome/browser/ui/tab_... > chrome/browser/ui/tab_helpers.cc:219: > DesktopEngagementObserver::CreateForWebContents(web_contents); > On 2016/07/13 18:40:58, Avi wrote: > > Alphabetical order, plz? > > Done. Tab helpers lgtm
https://codereview.chromium.org/2142983002/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/200001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:39: static MockAudibleContentsObserver* observer_; Couldn't these be owned by the test fixture, and allocated/destroyed in SetUp/TearDown? Cleaner than using singletons here, IMO.
chrisha@ please have a look. https://codereview.chromium.org/2142983002/diff/200001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/200001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:39: static MockAudibleContentsObserver* observer_; On 2016/07/20 21:49:26, chrisha (slow) wrote: > Couldn't these be owned by the test fixture, and allocated/destroyed in > SetUp/TearDown? Cleaner than using singletons here, IMO. Done.
https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:17: MockAudibleContentsObserver() { new metrics::AudibleContentsTracker(this); } The AudibleContentsTracker isn't owned by anyone and gets leaked? https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:38: MockAudibleContentsObserver* GetObserver() { return observer_.get(); } Just make this an accessor? Mock.... observer() const { return observer_.get(); } https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:39: I would do something like: std::unique_ptr<MockAudibleContentsObserver> observer_; std::unique_ptr<metrics::AudibleContentsTracker> tracker_; void SetUp() override { observer_ = new ...; tracker_ = new ...(observer_); } void TearDown() override { tracker_.reset(); observer_.reset(); } https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:41: std::unique_ptr<MockAudibleContentsObserver> observer_ = nullptr; No std::unique_ptr<metrics::AudibleContentsTracker> ?
Addresses chrisha@ comments for audio test https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:17: MockAudibleContentsObserver() { new metrics::AudibleContentsTracker(this); } On 2016/07/22 19:54:56, chrisha (slow) wrote: > The AudibleContentsTracker isn't owned by anyone and gets leaked? Moved to AudibleContentsTrackerTest https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:38: MockAudibleContentsObserver* GetObserver() { return observer_.get(); } On 2016/07/22 19:54:56, chrisha (slow) wrote: > Just make this an accessor? > > Mock.... observer() const { return observer_.get(); } Done. https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:39: On 2016/07/22 19:54:56, chrisha (slow) wrote: > I would do something like: > > std::unique_ptr<MockAudibleContentsObserver> observer_; > std::unique_ptr<metrics::AudibleContentsTracker> tracker_; > > void SetUp() override { > observer_ = new ...; > tracker_ = new ...(observer_); > } > > void TearDown() override { > tracker_.reset(); > observer_.reset(); > } Done. https://codereview.chromium.org/2142983002/diff/220001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:41: std::unique_ptr<MockAudibleContentsObserver> observer_ = nullptr; On 2016/07/22 19:54:56, chrisha (slow) wrote: > No std::unique_ptr<metrics::AudibleContentsTracker> ? Done.
Did a first pass and mostly style / comments. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:16: for (const auto browser : *browser_list) Nit: See recent chromium-dev discussion, basically the guidance is to explicitly put a * on pointer types when using auto. However, in this case it should be short enough to just spell out the type, so please do that. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:44: if (web_contents->WasRecentlyAudible()) { Nit: No {}'s https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:71: bool removed = (audible_contents_.erase(web_contents) == 1); Add a comment about this logic please. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:3: // found in the LICENSE file. Add an empty line after this please. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:12: namespace { Empty line after this. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:52: }; Nit: Add DISALLOW_ here too. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No (c) in new files. Fix throughout. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:19: static void Initialize(); Add a comment. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:22: virtual void SendVisibilityChangeEvent(bool active); Please add a short comment above all of these methods that are not overrides. Explain why you use virtual (for tests?0. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:50: } // namespace Add a new line above this. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:154: DLOG(ERROR) << "Logging session length of " << delta.InSeconds() Let's change these to DVLOG()'s, so chromium developers don't get spammed with these locally. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:20: static void Initialize(); Comment on all the functions please and the member variables. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service_unittest.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service_unittest.cc:17: Nit: You can omit newlines for these simple accessors. Also, use hacker_style. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser_list_observer.h:23: virtual void OnBrowserNoLongerActive(Browser* browser) {} Add a comment.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm happy with the AudibleContents stuff, and don't have anything to add beyond Alexei's comments. lgtm https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:44: InProcessBrowserTest::TearDown(); nit: Do this first, to keep setup and teardown symmetric.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addresses all the comments. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:16: for (const auto browser : *browser_list) On 2016/07/26 19:20:36, Alexei Svitkine (slow) wrote: > Nit: See recent chromium-dev discussion, basically the guidance is to explicitly > put a * on pointer types when using auto. However, in this case it should be > short enough to just spell out the type, so please do that. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:44: if (web_contents->WasRecentlyAudible()) { On 2016/07/26 19:20:36, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.cc:71: bool removed = (audible_contents_.erase(web_contents) == 1); On 2016/07/26 19:20:36, Alexei Svitkine (slow) wrote: > Add a comment about this logic please. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:3: // found in the LICENSE file. On 2016/07/26 19:20:36, Alexei Svitkine (slow) wrote: > Add an empty line after this please. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:12: namespace { On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Empty line after this. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:44: InProcessBrowserTest::TearDown(); On 2016/07/26 20:29:50, chrisha (slow) wrote: > nit: Do this first, to keep setup and teardown symmetric. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:52: }; On 2016/07/26 19:20:36, Alexei Svitkine (slow) wrote: > Nit: Add DISALLOW_ here too. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > No (c) in new files. Fix throughout. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:19: static void Initialize(); On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Add a comment. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:22: virtual void SendVisibilityChangeEvent(bool active); On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Please add a short comment above all of these methods that are not overrides. > Explain why you use virtual (for tests?0. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:50: } // namespace On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Add a new line above this. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:154: DLOG(ERROR) << "Logging session length of " << delta.InSeconds() On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Let's change these to DVLOG()'s, so chromium developers don't get spammed with > these locally. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:20: static void Initialize(); On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Comment on all the functions please and the member variables. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service_unittest.cc (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service_unittest.cc:17: On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Nit: You can omit newlines for these simple accessors. Also, use hacker_style. Done. https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list_observer.h (right): https://codereview.chromium.org/2142983002/diff/240001/chrome/browser/ui/brow... chrome/browser/ui/browser_list_observer.h:23: virtual void OnBrowserNoLongerActive(Browser* browser) {} On 2016/07/26 19:20:37, Alexei Svitkine (slow) wrote: > Add a comment. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks, another round of comments. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:55: if (BrowserList::GetInstance()->empty()) { Add a comment explaining this logic. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:71: } Add an empty line below this. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:26: virtual void SendVisibilityChangeEvent(bool active); I think having virtual functions that are private is confusing, even though it does work in C++. I suggest making them protected instead. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:21: bool IsActive() { return is_active_; } bool is_active() const Same for other accessors in your tests. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:26: bool is_active_; Add an empty line below this. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:21: : public metrics::AudibleContentsTracker::Observer { Please make things consistent re: using metrics namespace or not. I suggest putting all the new classes in the namespace. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:46: }; This seems overly complicated. Why not just have DesktopEngagementService be the metrics::AudibleContentsTracker::Observer? Then you don't need this whole block of code with extra statics - they can just be members of DesktopEngagementService. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:156: << " seconds."; Nit: Align <<'s Or just git cl format, which I think does the right thing. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:159: base::TimeDelta::FromHours(24), kNumTimeSlices); Looks like the metric on android uses a different macro, here: UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", duration); https://cs.chromium.org/chromium/src/chrome/browser/android/metrics/uma_sessi... I suggest just using the same macro as there so that the buckets are the same when not aggregating by platform. Please also add a comment to both here and the other file mentioning the other call site, so if someone decides to update the macro used, they'll know of the other location. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:23: // Returns true if the |DesktopEngagementService| instance has been created. Please add empty lines between functions. Fix throughout. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:601: if (chrome::GetLastActiveBrowser() != browser_.get()) { Do we need to remove this check? I remember when I tested this locally, if we had this check, then what could happen is that NotifyBrowserNoLongerActive() could be called when you switched to a different app and then when you switched back, this check would still evaluate to them being equal and SetLastActive would never be called. So I think we *do* need to update this to remove the check. I assume doing so would make it equivalent to what happens on other platforms, so presumably it shouldn't cause issues, but you might want to check the history. ... which I just glanced at and seems the if statement was added here: https://codereview.chromium.org/1105423003 ... and later was found to be problematic and modified here: https://codereview.chromium.org/1370753003 So my reading of the above was the original intention was to limit the -saveWindowPositionIfNeeded call below but it proved problematic and so we were left with this artifact if statement that no longer did what it was added for. So I think removing it here is fine, but please add a comment explaining why we need to call it unconditionally.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed asvitkine@ comments. chrisha@ could you have one more look. I changed the audio part. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:55: if (BrowserList::GetInstance()->empty()) { On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > Add a comment explaining this logic. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:71: } On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > Add an empty line below this. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.h:26: virtual void SendVisibilityChangeEvent(bool active); On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > I think having virtual functions that are private is confusing, even though it > does work in C++. I suggest making them protected instead. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:21: bool IsActive() { return is_active_; } On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > bool is_active() const > > Same for other accessors in your tests. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:26: bool is_active_; On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > Add an empty line below this. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:21: : public metrics::AudibleContentsTracker::Observer { On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > Please make things consistent re: using metrics namespace or not. I suggest > putting all the new classes in the namespace. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:46: }; On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > This seems overly complicated. Why not just have DesktopEngagementService be the > metrics::AudibleContentsTracker::Observer? Then you don't need this whole block > of code with extra statics - they can just be members of > DesktopEngagementService. Moved this implementation to AudibleContentsTracker https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:156: << " seconds."; On 2016/07/26 22:00:40, Alexei Svitkine (slow) wrote: > Nit: Align <<'s > > Or just git cl format, which I think does the right thing. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:159: base::TimeDelta::FromHours(24), kNumTimeSlices); On 2016/07/26 22:00:41, Alexei Svitkine (slow) wrote: > Looks like the metric on android uses a different macro, here: > > UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", duration); > > https://cs.chromium.org/chromium/src/chrome/browser/android/metrics/uma_sessi... > > I suggest just using the same macro as there so that the buckets are the same > when not aggregating by platform. > > Please also add a comment to both here and the other file mentioning the other > call site, so if someone decides to update the macro used, they'll know of the > other location. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:23: // Returns true if the |DesktopEngagementService| instance has been created. On 2016/07/26 22:00:41, Alexei Svitkine (slow) wrote: > Please add empty lines between functions. Fix throughout. Done. https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2142983002/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:601: if (chrome::GetLastActiveBrowser() != browser_.get()) { On 2016/07/26 22:00:41, Alexei Svitkine (slow) wrote: > Do we need to remove this check? > > I remember when I tested this locally, if we had this check, then what could > happen is that NotifyBrowserNoLongerActive() could be called when you switched > to a different app and then when you switched back, this check would still > evaluate to them being equal and SetLastActive would never be called. > > So I think we *do* need to update this to remove the check. I assume doing so > would make it equivalent to what happens on other platforms, so presumably it > shouldn't cause issues, but you might want to check the history. > > ... which I just glanced at and seems the if statement was added here: > > https://codereview.chromium.org/1105423003 > > ... and later was found to be problematic and modified here: > > https://codereview.chromium.org/1370753003 > > So my reading of the above was the original intention was to limit the > -saveWindowPositionIfNeeded call below but it proved problematic and so we were > left with this artifact if statement that no longer did what it was added for. > > So I think removing it here is fine, but please add a comment explaining why we > need to call it unconditionally. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)
lgtm % remaining comments https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:65: // This metric is recorded separately for desktops in Nit: "for desktops" -> "on desktop" Add a "Note: " at the front of the comment. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:798: // Start the DesktopEngagementService. This comment doesn't say much that the call below doesn't. Remove it. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:25: bool IsAudioPlaying() { return is_audio_playing_; } is_audio_playing() const https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:21: bool IsActive() const { return is_active_; } is_active() https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_observer.h (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_observer.h:19: // Class for tracking user interaction with the browser. Please make explain this more. You can omit "Class for" since that doesn't add anything. Mention that this is used to monitor input events from web contents and notify the desktop engagement service. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:125: // This metric is recorded separately for Android in Nit: "Note: " https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:127: UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", delta); Please update the histograms.xml description of this metric as it currently says "on mobile" and also add some more owners. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:47: // Used for marking current state of the user engagement. Data members should not be protected, per the style guide. ("Note that data members should be private."). If these need to be accessed by tests, add protected accessors functions or ForTesting() functions.
Thanks. Addressed all the comments. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/android... File chrome/browser/android/metrics/uma_session_stats.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/android... chrome/browser/android/metrics/uma_session_stats.cc:65: // This metric is recorded separately for desktops in On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > Nit: "for desktops" -> "on desktop" > > Add a "Note: " at the front of the comment. Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:798: // Start the DesktopEngagementService. On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > This comment doesn't say much that the call below doesn't. Remove it. Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker_browsertest.cc:25: bool IsAudioPlaying() { return is_audio_playing_; } On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > is_audio_playing() const Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer_browsertest.cc:21: bool IsActive() const { return is_active_; } On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > is_active() Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_observer.h (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_observer.h:19: // Class for tracking user interaction with the browser. On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > Please make explain this more. > > You can omit "Class for" since that doesn't add anything. Mention that this is > used to monitor input events from web contents and notify the desktop engagement > service. Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:125: // This metric is recorded separately for Android in On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > Nit: "Note: " Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:127: UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", delta); On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > Please update the histograms.xml description of this metric as it currently says > "on mobile" and also add some more owners. Done. https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/300001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:47: // Used for marking current state of the user engagement. On 2016/07/28 18:37:22, Alexei Svitkine (slow) wrote: > Data members should not be protected, per the style guide. ("Note that data > members should be private."). > > If these need to be accessed by tests, add protected accessors functions or > ForTesting() functions. moved the functions to public.
Sorry, had another glance and noticed a few more things. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:33: virtual void OnAudioEnd(); Make this a pure virtual class and just have DesktopEngagementService implement this observer class. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:39: void ChromeVisibilityObserver::Initialize() { Sorry, I missed this before, but can you change this and audible tracker be members of DesktopEngagementService? Then, they don't need to be globals or have Initialize() functions and can just be members of that class. (Don't even need to have them be allocated with new, can just be direct members.)
Sorry, had another glance and noticed a few more things. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:33: virtual void OnAudioEnd(); Make this a pure virtual class and just have DesktopEngagementService implement this observer class. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:39: void ChromeVisibilityObserver::Initialize() { Sorry, I missed this before, but can you change this and audible tracker be members of DesktopEngagementService? Then, they don't need to be globals or have Initialize() functions and can just be members of that class. (Don't even need to have them be allocated with new, can just be direct members.)
https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:270: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) Will this also work for ChromeOS? https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:799: #endif // defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) ubernit: I find the "#endif // blah" comments useful when the #if is a ways away (10s of lines or more), but distracting for such small one line conditionals. YMMV. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:26: ~Observer() {} virtual ~Observer() as this is an abstract base class. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:33: virtual void OnAudioEnd(); On 2016/07/28 21:31:45, Alexei Svitkine (slow) wrote: > Make this a pure virtual class and just have DesktopEngagementService implement > this observer class. +1
Moved the implementation of audio observer to DesktopEngagementService https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:270: #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) On 2016/07/29 15:21:25, chrisha (slow) wrote: > Will this also work for ChromeOS? Not sure, maybe there will be need for extra Chrome OS specific work. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:799: #endif // defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_LINUX) On 2016/07/29 15:21:25, chrisha (slow) wrote: > ubernit: I find the "#endif // blah" comments useful when the #if is a ways > away (10s of lines or more), but distracting for such small one line > conditionals. YMMV. Done. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:26: ~Observer() {} On 2016/07/29 15:21:26, chrisha (slow) wrote: > virtual ~Observer() as this is an abstract base class. Done. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:33: virtual void OnAudioEnd(); On 2016/07/28 21:31:45, Alexei Svitkine (slow) wrote: > Make this a pure virtual class and just have DesktopEngagementService implement > this observer class. Done. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/audible_contents_tracker.h:33: virtual void OnAudioEnd(); On 2016/07/29 15:21:25, chrisha (slow) wrote: > On 2016/07/28 21:31:45, Alexei Svitkine (slow) wrote: > > Make this a pure virtual class and just have DesktopEngagementService > implement > > this observer class. > > +1 Acknowledged. https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc (right): https://codereview.chromium.org/2142983002/diff/320001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/chrome_visibility_observer.cc:39: void ChromeVisibilityObserver::Initialize() { On 2016/07/28 21:31:45, Alexei Svitkine (slow) wrote: > Sorry, I missed this before, but can you change this and audible tracker be > members of DesktopEngagementService? Then, they don't need to be globals or have > Initialize() functions and can just be members of that class. (Don't even need > to have them be allocated with new, can just be direct members.) Done.
LGTM % comments Thanks! https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:87: visibility_observer_(), Nit: You can omit this, the no argument constructor will just be run implicitly. https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:37: // AudibleContentsTracker::Observer Nit: You can make these private since we don't expect other callers besides the observer to call these.
No comments beyond Alexei's nits. lgtm! Happy to see this land!
Addressed the nits https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc (right): https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:87: visibility_observer_(), On 2016/07/29 19:58:08, Alexei Svitkine (slow) wrote: > Nit: You can omit this, the no argument constructor will just be run implicitly. Done. https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h (right): https://codereview.chromium.org/2142983002/diff/340001/chrome/browser/metrics... chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h:37: // AudibleContentsTracker::Observer On 2016/07/29 19:58:08, Alexei Svitkine (slow) wrote: > Nit: You can make these private since we don't expect other callers besides the > observer to call these. Done.
gayane@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please have a look at following changes for owners review. chrome/browser/chrome_browser_main.cc chrome/browser/ui/* chrome/chrome_browser.gypi chrome/chrome_tests.gypi chrome/chrome_tests_unit.gypi
General comments about CL descriptions: - The CL description becomes a git commit message. Try to write it in a git-friendly manner: SummaryLine\n\nDetailedDescription - Wrap at 72 chars I also don't understand the "Patched from" bits. Are you trying to give others credit for writing parts of this CL? https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:271: #include "chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h" Just put this with the !OS_ANDROID #includes on line 260? (There's no / barely any iOS in chrome/) https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:81: static void NotifyBrowserNoLongerActive(Browser* browser); Do you really need this, or can you infer a given |browser| is no longer active when SetLastActive() gets called for a different |browser| ? https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:601: // Set this window as active even if the previously active windows was the Funny enough, before https://codereview.chromium.org/1105423003 the code was written like this, but without any of the comments here and at line 605.
Description was changed from ========== Desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Patched from chrisha@ CL https://codereview.chromium.org/2127143002/ Patched from asvitkine@ CL https://codereview.chromium.org/2101743002/ Patched from pmonette@ CL https://codereview.chromium.org/2102263002/ BUG=615080 ========== to ========== Add desktop engagement metrics. Adding desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Patched from chrisha@ CL https://codereview.chromium.org/2127143002/ Patched from asvitkine@ CL https://codereview.chromium.org/2101743002/ Patched from pmonette@ CL https://codereview.chromium.org/2102263002/ BUG=615080 ==========
Thanks for the review. Fixed the description. For he patched CLs - yes i have added it for giving credit to others https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:271: #include "chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h" On 2016/08/01 17:29:02, Lei Zhang wrote: > Just put this with the !OS_ANDROID #includes on line 260? (There's no / barely > any iOS in chrome/) how about ChromeOS, this shouldn't work for ChromeOS either https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:81: static void NotifyBrowserNoLongerActive(Browser* browser); On 2016/08/01 17:29:02, Lei Zhang wrote: > Do you really need this, or can you infer a given |browser| is no longer active > when SetLastActive() gets called for a different |browser| ? I need this because when user removes the focus from a browser and doesn't activate any other instance of a browser (like moving to other application) then the user session should end. https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_controller.mm:601: // Set this window as active even if the previously active windows was the On 2016/08/01 17:29:02, Lei Zhang wrote: > Funny enough, before https://codereview.chromium.org/1105423003 the code was > written like this, but without any of the comments here and at line 605. Yes, there is also this CL https://codereview.chromium.org/1370753003 following that one which I think left the check for no reason anymore and in our case we needed to remove the check so that when browser loses the focus the notification is always sent.
Description was changed from ========== Add desktop engagement metrics. Adding desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Patched from chrisha@ CL https://codereview.chromium.org/2127143002/ Patched from asvitkine@ CL https://codereview.chromium.org/2101743002/ Patched from pmonette@ CL https://codereview.chromium.org/2102263002/ BUG=615080 ========== to ========== Add desktop engagement metrics. Adding desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ==========
Description was changed from ========== Add desktop engagement metrics. Adding desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ========== to ========== Add desktop engagement metrics. Add desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ==========
I edited the CL description some more, to fit 72 columns, and to more clearly give credit to others. https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:271: #include "chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h" On 2016/08/02 17:13:44, gayane wrote: > On 2016/08/01 17:29:02, Lei Zhang wrote: > > Just put this with the !OS_ANDROID #includes on line 260? (There's no / barely > > any iOS in chrome/) > how about ChromeOS, this shouldn't work for ChromeOS either So "desktop" in my mind includes ChromeOS, and https://crbug.com/615080 is marked as Win/Mac/Linux/CrOS. Still don't want ChromeOS? If that's the case, then you need to explicitly check that OS_CHROMEOS is not defined, because OS_LINUX is defined on ChromeOS.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Thanks! You are right ChromeOS should be within desktop its just its different enough that needs separate work. I added a check for ChromeOS and removed ChromeOS label from the bug. On Tue, Aug 2, 2016 at 1:31 PM <thestig@chromium.org> wrote: > I edited the CL description some more, to fit 72 columns, and to more > clearly > give credit to others. > > > > > https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_main.cc (right): > > > https://codereview.chromium.org/2142983002/diff/360001/chrome/browser/chrome_... > chrome/browser/chrome_browser_main.cc:271: #include > "chrome/browser/metrics/desktop_engagement/desktop_engagement_service.h" > On 2016/08/02 17:13:44, gayane wrote: > > On 2016/08/01 17:29:02, Lei Zhang wrote: > > > Just put this with the !OS_ANDROID #includes on line 260? (There's > no / barely > > > any iOS in chrome/) > > how about ChromeOS, this shouldn't work for ChromeOS either > > So "desktop" in my mind includes ChromeOS, and https://crbug.com/615080 > is marked as Win/Mac/Linux/CrOS. > > Still don't want ChromeOS? If that's the case, then you need to > explicitly check that OS_CHROMEOS is not defined, because OS_LINUX is > defined on ChromeOS. > > https://codereview.chromium.org/2142983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
FWIW, I think we should support ChromeOS as well and thought we were intentionally doing so with the OS_LINUX check.
The thing we weren't sure about is what happens if a user interacts with ChromeOS but not Chrome windows - e.g. using the file manager or other apps. I feel like we should test that case and figure out what we want to do there, before enabling this. And it seemed we can land the initial version without support for CrOS and investigate CrOS support later. On Tue, Aug 2, 2016 at 1:52 PM, <chrisha@chromium.org> wrote: > FWIW, I think we should support ChromeOS as well and thought we were > intentionally doing so with the OS_LINUX check. > > https://codereview.chromium.org/2142983002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Okay, makes sense. Worth documenting on the tracking bug. On Tue, 2 Aug 2016 at 13:56 Alexei Svitkine <asvitkine@chromium.org> wrote: > The thing we weren't sure about is what happens if a user interacts with > ChromeOS but not Chrome windows - e.g. using the file manager or other > apps. I feel like we should test that case and figure out what we want to > do there, before enabling this. And it seemed we can land the initial > version without support for CrOS and investigate CrOS support later. > > On Tue, Aug 2, 2016 at 1:52 PM, <chrisha@chromium.org> wrote: > >> FWIW, I think we should support ChromeOS as well and thought we were >> intentionally doing so with the OS_LINUX check. >> >> https://codereview.chromium.org/2142983002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Bots are red... https://codereview.chromium.org/2142983002/diff/400001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2142983002/diff/400001/chrome/chrome_browser.... chrome/chrome_browser.gypi:1397: 'browser/metrics/desktop_engagement/audible_contents_tracker.cc', If this is now non-ChromeOS, the new files should be moved into the chrome_browser_desktop_sources list above. Adjust other .gyp files accordingly.
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gayane@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All bots fixed now
lgtm https://codereview.chromium.org/2142983002/diff/460001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2142983002/diff/460001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:2645: ['OS=="android" or OS=="ios" or chromeos == 1', { You can invert this, and then move profile_statistics_browsertest.cc into your new list. Ditto in chrome/test/BUILD.gn.
Inverted logic for profile_statistics_browsertest.cc in build files. https://codereview.chromium.org/2142983002/diff/460001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2142983002/diff/460001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:2645: ['OS=="android" or OS=="ios" or chromeos == 1', { On 2016/08/03 17:55:44, Lei Zhang wrote: > You can invert this, and then move profile_statistics_browsertest.cc into your > new list. Ditto in chrome/test/BUILD.gn. Done.
++lgtm
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, asvitkine@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2142983002/#ps480001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add desktop engagement metrics. Add desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ========== to ========== Add desktop engagement metrics. Add desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Add desktop engagement metrics. Add desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 ========== to ========== Add desktop engagement metrics. Add desktop engagement metrics which records Chrome session length. This metrics takes into account Chrome visibility, presence of audio and user interaction to start and end sessions. Parts of this CL are derived from the following CLs: https://codereview.chromium.org/2127143002/ by chrisha@ https://codereview.chromium.org/2101743002/ by asvitkine@ https://codereview.chromium.org/2102263002/ by pmonette@ BUG=615080 Committed: https://crrev.com/c4053604844bed762906984ac304f82e2db787d5 Cr-Commit-Position: refs/heads/master@{#409603} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/c4053604844bed762906984ac304f82e2db787d5 Cr-Commit-Position: refs/heads/master@{#409603} |
