Update PageLoadTiming to use base::Optional

Update PageLoadTiming to use base::Optional

Until now, we've used base::TimeDelta.is_zero() as a signal that
a given timing is unset.

This is technically incorrect, as a TimeDelta can be set to the
zero value.

This mostly presented issues that made writing tests more complex,
but also addresses real issues where a metric reports a zero time
delta that would have previously been ignored but will now be
included in our metrics.

BUG=623556
Committed: https://crrev.com/17bcb076bdfb2d7c27b973518c8488a0c885ef1a
Cr-Commit-Position: refs/heads/master@{#404493}

[Bryan McQuade, 2016-07-01 18:50:50]

Description was changed from

==========
NOT FOR REVIEW: update PageLoadTiming to use base::Optional

BUG=
==========

to

==========
Update PageLoadTiming to use base::Optional

BUG=623556
==========

[Bryan McQuade, 2016-07-06 16:54:39]

bmcquade@chromium.org changed reviewers:
+ mushan@google.com, ryansturm@chromium.org, shivanisha@chromium.org

[Bryan McQuade, 2016-07-06 16:57:21]

Description was changed from

==========
Update PageLoadTiming to use base::Optional

BUG=623556
==========

to

==========
Update PageLoadTiming to use base::Optional

Until now, we've used base::TimeDelta.is_zero() as a signal that
a given timing is unset.

This is technically incorrect, as a TimeDelta can be set to the
zero value.

This mostly presented issues that made writing tests more complex,
but also addresses real issues where a metric reports a zero time
delta that would have previously been ignored but will now be
included in our metrics.

BUG=623556
==========

[Bryan McQuade, 2016-07-06 16:57:26]

shivanisha, PTAL for overall change
ryansturm, PTAL for changes to data saver metric tracking
mushan, PTAL for changes to GWS metric tracking

[RyanSturm, 2016-07-06 17:10:51]

data saver metrics lgtm % nits

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:135:
response_start = timing.response_start.value();
nit: add braces for these since the if is spanning 2 lines now.

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:146:
// TODO(ryansturm): consider changing DataReductionProxyPageLoadTiming to take
I opened a bug, can you add crbug.com/626040 to the comment

[mushan, 2016-07-07 03:07:14]

mushan@chromium.org changed reviewers:
+ mushan@chromium.org

[mushan, 2016-07-07 03:07:16]

lgtm

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:211:
base::TimeDelta time_to_abort) {
Can we make time_to_interaction and time_to_abort optional here?

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:211:
base::TimeDelta time_to_abort) {
Can we make time_to_interaction and time_to_abort optional here?

[mushan, 2016-07-07 03:14:21]

mushan@chromium.org changed reviewers:
- mushan@google.com

[mushan, 2016-07-07 03:14:22]

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:505:
time_to_abort)) {
The condition of first_user_interaction_after_paint_ not existing is logically a
part of WasAbortedBeforeInteraction. Passing in the optional value and check in
that function might be better?

[shivanisha, 2016-07-07 15:38:57]

Thanks for the change.
LGTM with some comments.

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:255:
if (timing.parse_start) {
nit: Early return from here if !timing.parse_start

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:281:
if (timing.parse_stop) {
nit: Early return from here if !timing.parse_stop

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:66: 
Would the operator definition be a good fit for base/optional.h?

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_util.cc (right):

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_util.cc:31: 
Can we remove WasStartedInForegroundEventInForeground since we are using
WasStartedInForegroundOptionalEventInForeground in all places now?
We can also remove the TODO comment above this function.

[Bryan McQuade, 2016-07-08 18:39:36]

Thanks! Comments addressed.

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:135:
response_start = timing.response_start.value();
On 2016/07/06 at 17:10:51, RyanSturm wrote:
> nit: add braces for these since the if is spanning 2 lines now.

Done (and for other cases below)

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:146:
// TODO(ryansturm): consider changing DataReductionProxyPageLoadTiming to take
On 2016/07/06 at 17:10:51, RyanSturm wrote:
> I opened a bug, can you add crbug.com/626040 to the comment

Thanks! Comment updated.

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:255:
if (timing.parse_start) {
On 2016/07/07 at 15:38:57, shivanisha wrote:
> nit: Early return from here if !timing.parse_start

Good call, done

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:281:
if (timing.parse_stop) {
On 2016/07/07 at 15:38:57, shivanisha wrote:
> nit: Early return from here if !timing.parse_stop

Done

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:505:
time_to_abort)) {
On 2016/07/07 at 03:14:22, mushan wrote:
> The condition of first_user_interaction_after_paint_ not existing is logically
a part of WasAbortedBeforeInteraction. Passing in the optional value and check
in that function might be better?

Good point - migrated this check to WasAbortedBeforeInteraction. Thanks!

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:66: 
On 2016/07/07 at 15:38:57, shivanisha wrote:
> Would the operator definition be a good fit for base/optional.h?

I had wanted to do this as well. I asked the base/OWNERS about this - they said
they only want to include things in base/optional.h that are part of
std::optional, so we can port from base::Optional to std::optional when it's
available in the Chromium build. They suggested doing this for now. There's a
bit more discussion on this in https://codereview.chromium.org/2109063006 (which
I closed without committing, based on feedback from base owners).

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_util.cc (right):

https://codereview.chromium.org/2111073003/diff/100001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_util.cc:31: 
On 2016/07/07 at 15:38:57, shivanisha wrote:
> Can we remove WasStartedInForegroundEventInForeground since we are using
WasStartedInForegroundOptionalEventInForeground in all places now?
> We can also remove the TODO comment above this function.

Ah, great, done, thanks!

[Bryan McQuade, 2016-07-08 19:54:41]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-07-08 19:54:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
shivanisha@chromium.org, ryansturm@chromium.org, mushan@chromium.org
Link to the patchset: https://codereview.chromium.org/2111073003/#ps120002
(title: "remove comment")

[commit-bot: I haz the power, 2016-07-08 19:55:04]

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-07-08 21:21:18]

Committed patchset #8 (id:120002)

[commit-bot: I haz the power, 2016-07-08 21:21:29]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 21:24:13]

Description was changed from

==========
Update PageLoadTiming to use base::Optional

Until now, we've used base::TimeDelta.is_zero() as a signal that
a given timing is unset.

This is technically incorrect, as a TimeDelta can be set to the
zero value.

This mostly presented issues that made writing tests more complex,
but also addresses real issues where a metric reports a zero time
delta that would have previously been ignored but will now be
included in our metrics.

BUG=623556
==========

to

==========
Update PageLoadTiming to use base::Optional

Until now, we've used base::TimeDelta.is_zero() as a signal that
a given timing is unset.

This is technically incorrect, as a TimeDelta can be set to the
zero value.

This mostly presented issues that made writing tests more complex,
but also addresses real issues where a metric reports a zero time
delta that would have previously been ignored but will now be
included in our metrics.

BUG=623556

Committed: https://crrev.com/17bcb076bdfb2d7c27b973518c8488a0c885ef1a
Cr-Commit-Position: refs/heads/master@{#404493}
==========

[commit-bot: I haz the power, 2016-07-08 21:24:14]

Patchset 8 (id:??) landed as
https://crrev.com/17bcb076bdfb2d7c27b973518c8488a0c885ef1a
Cr-Commit-Position: refs/heads/master@{#404493}