[M54] Merge DesktopEngagementService fixes.

[M54] Merge DesktopEngagementService fixes.

This merges the following 3 CLs:

Discount for inactivity timeout for recording session length

When session ends because of timeout we should subtract the
waiting time for more accurate session length.

BUG=645538
Committed: https://crrev.com/1d1e6805b10cf2101d4a926a4c9c19ffb9c66749
Cr-Commit-Position: refs/heads/master@{#418396}

Fix DesktopEngagementServiceTest.TestTimeoutDiscount unittests

Fixing the flaky unittest by correcting the wrong measurement units.
This CL also enables the test for mac bots.

BUG=645538, 646758
Committed: https://crrev.com/68de0eb04f00eed4467bb743eccaec1b92dba132
Cr-Commit-Position: refs/heads/master@{#419000}

[DesktopSessionDurationTracker] Remove visibility-switch timeout from session length.

This aims to remove the artificial spike at 3 seconds in the Session.TotalDuration metric.

BUG=652253
Committed: https://crrev.com/8f546cb788ad68d0f2de3ea047a0166a35884b33
Cr-Commit-Position: refs/heads/master@{#422501}

NOTRY=true
NOPRESUBMIT=true
BUG=645538

[chrisha, 2016-10-12 15:25:52]

chrisha@chromium.org changed reviewers:
+ asvitkine@chromium.org, bustamante@chromium.org

[chrisha, 2016-10-12 15:25:52]

This is the code I'd like to merge to M54, as talked about yesterday.

Given that this was manually applied (skipping over the massive rename patch),
I'd love a second pair of eyes on this Alexei. Confirmed that this builds
locally and passes unittests.

Should I add a "merge request" label on the tracking bug, or create a new bug?

[chrisha, 2016-10-12 15:26:05]

The CQ bit was checked by chrisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 15:26:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 15:26:25]

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

[commit-bot: I haz the power, 2016-10-12 15:26:26]

Dry run: CLs for remote refs other than refs/pending/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[chrisha, 2016-10-12 15:27:28]

Description was changed from

==========
[M54] Merge DesktopEngagementService fixes.

This merges the following 3 CLs:

Discount for inactivity timeout for recording session length

When session ends because of timeout we should subtract the
waiting time for more accurate session length.

BUG=645538
Committed: https://crrev.com/1d1e6805b10cf2101d4a926a4c9c19ffb9c66749
Cr-Commit-Position: refs/heads/master@{#418396}

Fix DesktopEngagementServiceTest.TestTimeoutDiscount unittests

Fixing the flaky unittest by correcting the wrong measurement units.
This CL also enables the test for mac bots.

BUG=645538, 646758
Committed: https://crrev.com/68de0eb04f00eed4467bb743eccaec1b92dba132
Cr-Commit-Position: refs/heads/master@{#419000}

[DesktopSessionDurationTracker] Remove visibility-switch timeout from session
length.

This aims to remove the artificial spike at 3 seconds in the
Session.TotalDuration metric.

BUG=652253
Committed: https://crrev.com/8f546cb788ad68d0f2de3ea047a0166a35884b33
Cr-Commit-Position: refs/heads/master@{#422501}

BUG=645538
==========

to

==========
[M54] Merge DesktopEngagementService fixes.

This merges the following 3 CLs:

Discount for inactivity timeout for recording session length

When session ends because of timeout we should subtract the
waiting time for more accurate session length.

BUG=645538
Committed: https://crrev.com/1d1e6805b10cf2101d4a926a4c9c19ffb9c66749
Cr-Commit-Position: refs/heads/master@{#418396}

Fix DesktopEngagementServiceTest.TestTimeoutDiscount unittests

Fixing the flaky unittest by correcting the wrong measurement units.
This CL also enables the test for mac bots.

BUG=645538, 646758
Committed: https://crrev.com/68de0eb04f00eed4467bb743eccaec1b92dba132
Cr-Commit-Position: refs/heads/master@{#419000}

[DesktopSessionDurationTracker] Remove visibility-switch timeout from session
length.

This aims to remove the artificial spike at 3 seconds in the
Session.TotalDuration metric.

BUG=652253
Committed: https://crrev.com/8f546cb788ad68d0f2de3ea047a0166a35884b33
Cr-Commit-Position: refs/heads/master@{#422501}

NOTRY=true
NOPRESUBMIT=true
BUG=645538
==========

[Alexei Svitkine (slow), 2016-10-12 15:34:38]

lgtm

https://codereview.chromium.org/2412133002/diff/1/chrome/browser/metrics/desk...
File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc
(right):

https://codereview.chromium.org/2412133002/diff/1/chrome/browser/metrics/desk...
chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:18:
const base::TimeDelta kZeroTime = base::TimeDelta::FromSeconds(0);
Hmm, I'm actually surprised this didn't introduce a static initializer in the
original CL, which we generally don't permit. Anyway, no concerns here since
it's merging the same code that landed, but I'm curious why this didn't make
bots yell at us.

[chrisha, 2016-10-12 17:36:21]

https://codereview.chromium.org/2412133002/diff/1/chrome/browser/metrics/desk...
File chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc
(right):

https://codereview.chromium.org/2412133002/diff/1/chrome/browser/metrics/desk...
chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc:18:
const base::TimeDelta kZeroTime = base::TimeDelta::FromSeconds(0);
On 2016/10/12 15:34:38, Alexei Svitkine (slow) wrote:
> Hmm, I'm actually surprised this didn't introduce a static initializer in the
> original CL, which we generally don't permit. Anyway, no concerns here since
> it's merging the same code that landed, but I'm curious why this didn't make
> bots yell at us.

Due to fancy newfangled constexpr semantics, this is now safe to do:

https://cs.chromium.org/chromium/src/base/time/time.h?q=base::TimeDelta&sq=pa...

[Alexei Svitkine (slow), 2016-10-12 18:29:27]

Ah neat, thanks for the pointer. :)

On Wed, Oct 12, 2016 at 1:36 PM, <chrisha@chromium.org> wrote:

>
> https://codereview.chromium.org/2412133002/diff/1/chrome/
> browser/metrics/desktop_engagement/desktop_engagement_service.cc
> File
> chrome/browser/metrics/desktop_engagement/desktop_engagement_service.cc
> (right):
>
> https://codereview.chromium.org/2412133002/diff/1/chrome/
> browser/metrics/desktop_engagement/desktop_engagement_service.cc#newcode18
> chrome/browser/metrics/desktop_engagement/desktop_
> engagement_service.cc:18:
> const base::TimeDelta kZeroTime = base::TimeDelta::FromSeconds(0);
> On 2016/10/12 15:34:38, Alexei Svitkine (slow) wrote:
> > Hmm, I'm actually surprised this didn't introduce a static initializer
> in the
> > original CL, which we generally don't permit. Anyway, no concerns here
> since
> > it's merging the same code that landed, but I'm curious why this
> didn't make
> > bots yell at us.
>
> Due to fancy newfangled constexpr semantics, this is now safe to do:
>
> https://cs.chromium.org/chromium/src/base/time/time.h?
> q=base::TimeDelta&sq=package:chromium&l=115
>
> https://codereview.chromium.org/2412133002/
>

-- 
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.

[chrisha, 2016-10-19 15:09:53]

Closing this issue. The code made it into M55, so we'll simply wait for results
there.