Changing the TimeDeltas in DRPPageLoadTiming to Optional

Changing the TimeDeltas in DRPPageLoadTiming to Optional

To match the TimeDeltas in page_load_metrics, the
DataReductionProxyPageLoadTiming TimeDeltas are changed to
base::Optional TimeDeltas. Functionally, this will only change the case
where the TimeDelta is zero.

The constructor for DataReductionProxyPageLoadTiming is moved to a new
cc file due to the templated variables creating a complex constructor.

BUG=626040
Committed: https://crrev.com/4aaa1c3f03e522bb84b99e6bdf36a9b334865547
Cr-Commit-Position: refs/heads/master@{#405279}

[RyanSturm, 2016-07-12 22:14:24]

ryansturm@chromium.org changed reviewers:
+ tbansal@chromium.org

[RyanSturm, 2016-07-12 22:14:25]

tbansal: PTAL at * before I send it to page_load_metrics review, thanks

[tbansal1, 2016-07-12 22:58:01]

https://codereview.chromium.org/2145743002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2145743002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:59:
timing_.reset(new data_reduction_proxy::DataReductionProxyPageLoadTiming(
Add a copy constructor to the struct?

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
(right):

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:9:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
Why not do this in .cc file? It is weird to have .cc file for a struct.

[RyanSturm, 2016-07-12 23:38:16]

Patchset #2 (id:20001) has been deleted

[RyanSturm, 2016-07-13 17:05:17]

https://codereview.chromium.org/2145743002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2145743002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:59:
timing_.reset(new data_reduction_proxy::DataReductionProxyPageLoadTiming(
On 2016/07/12 22:58:01, tbansal1 wrote:
> Add a copy constructor to the struct?

Done.

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
(right):

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:9:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
On 2016/07/12 22:58:01, tbansal1 wrote:
> Why not do this in .cc file? It is weird to have .cc file for a struct.

since response_start and the others are a template type, chromium style
considers this a complex constructor, so they need to not be inlined. I'm not
really sure the best way to handle this, but other structs have their own cc
file:
https://cs.chromium.org/chromium/src/content/public/common/resource_response.cc

[tbansal1, 2016-07-13 17:41:10]

lgtm

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc
(right):

https://codereview.chromium.org/2145743002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.cc:9:
DataReductionProxyPageLoadTiming::DataReductionProxyPageLoadTiming(
On 2016/07/13 17:05:17, RyanSturm wrote:
> On 2016/07/12 22:58:01, tbansal1 wrote:
> > Why not do this in .cc file? It is weird to have .cc file for a struct.
> 
> since response_start and the others are a template type, chromium style
> considers this a complex constructor, so they need to not be inlined. I'm not
> really sure the best way to handle this, but other structs have their own cc
> file:
>
https://cs.chromium.org/chromium/src/content/public/common/resource_response.cc

Acknowledged.

[RyanSturm, 2016-07-13 17:59:04]

ryansturm@chromium.org changed reviewers:
+ bmcquade@chromium.org

[RyanSturm, 2016-07-13 17:59:04]

bmcquade: PTAL @
page_load_metrics/observers/data_reduction_proxy_metrics_observer*

[Bryan McQuade, 2016-07-13 18:02:54]

On 2016/07/13 at 17:59:04, ryansturm wrote:
> bmcquade: PTAL @
page_load_metrics/observers/data_reduction_proxy_metrics_observer*

LGTM, thanks!

[RyanSturm, 2016-07-13 18:36:24]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-07-13 18:36:57]

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-13 20:35:36]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-13 20:35:57]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 20:37:35]

Description was changed from

==========
Changing the TimeDeltas in DRPPageLoadTiming to Optional

To match the TimeDeltas in page_load_metrics, the
DataReductionProxyPageLoadTiming TimeDeltas are changed to
base::Optional TimeDeltas. Functionally, this will only change the case
where the TimeDelta is zero.

The constructor for DataReductionProxyPageLoadTiming is moved to a new
cc file due to the templated variables creating a complex constructor.

BUG=626040
==========

to

==========
Changing the TimeDeltas in DRPPageLoadTiming to Optional

To match the TimeDeltas in page_load_metrics, the
DataReductionProxyPageLoadTiming TimeDeltas are changed to
base::Optional TimeDeltas. Functionally, this will only change the case
where the TimeDelta is zero.

The constructor for DataReductionProxyPageLoadTiming is moved to a new
cc file due to the templated variables creating a complex constructor.

BUG=626040

Committed: https://crrev.com/4aaa1c3f03e522bb84b99e6bdf36a9b334865547
Cr-Commit-Position: refs/heads/master@{#405279}
==========

[commit-bot: I haz the power, 2016-07-13 20:37:36]

Patchset 3 (id:??) landed as
https://crrev.com/4aaa1c3f03e522bb84b99e6bdf36a9b334865547
Cr-Commit-Position: refs/heads/master@{#405279}