Refactoring data reduction proxy some methods to common

Refactoring data reduction proxy some methods to common

Certain methods, especially for protobufs, can be moved to common in
data_reduction_proxy_utils.

BUG=

Committed: https://crrev.com/c63ef5ab892e78c1279db7301d2d2aac4f959e76
Cr-Commit-Position: refs/heads/master@{#397175}

[RyanSturm, 2016-05-27 22:21:45]

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

[RyanSturm, 2016-05-27 22:21:46]

tbansal: PTAL

[tbansal1, 2016-05-27 23:01:59]

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/BUILD.gn (right):

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/BUILD.gn:44: defines = [
"USE_GOOGLE_API_KEYS" ]
Two questions:
(1) data_reduction_proxy/core/browser/BUILD.gn has 
if (!is_ios) {
    defines += [ "USE_GOOGLE_API_KEYS_FOR_AUTH_KEY" ]
  }
Is that needed here?

(2) Should we update that file to remove dependencies and defines statements?

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

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:94:
return base::Time::FromJsTime(
Is there a difference between this implementation and the one jeremyim had
before:
https://codereview.chromium.org/1509923003/diff/100001/components/data_reduct...

Can we use the other one since that was tested to work, and does not involve
this Javascript timing.

Alternatively, I would suggest using existing APIs from third_party (but keep
the tests that you added):
https://code.google.com/p/chromium/codesearch#search/&q=TimestampToTimeT&sq=p...
Then, use the functions in base/time.h to convert between time_t and base::Time.
wdyt?

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
(right):

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:46: //
Returns the |Timestamp| representation of |time|. This is accurate to the
This comment looks incorrect.

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

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc:29:
const base::Time& GetFutureTime() { return future_time_; }
const method.

[RyanSturm, 2016-05-31 20:10:52]

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/BUILD.gn (right):

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/BUILD.gn:44: defines = [
"USE_GOOGLE_API_KEYS" ]
On 2016/05/27 23:01:58, tbansal1 wrote:
> Two questions:
> (1) data_reduction_proxy/core/browser/BUILD.gn has 
> if (!is_ios) {
>     defines += [ "USE_GOOGLE_API_KEYS_FOR_AUTH_KEY" ]
>   }
> Is that needed here?
> 
> (2) Should we update that file to remove dependencies and defines statements?

1) not needed 2) still needed in core

I haven't moved all of the google_apis code from core to common, and the piece
that needs !is_ios is still in core.

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

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:94:
return base::Time::FromJsTime(
On 2016/05/27 23:01:58, tbansal1 wrote:
> Is there a difference between this implementation and the one jeremyim had
> before:
>
https://codereview.chromium.org/1509923003/diff/100001/components/data_reduct...
> 
> Can we use the other one since that was tested to work, and does not involve
> this Javascript timing.
> 
> Alternatively, I would suggest using existing APIs from third_party (but keep
> the tests that you added):
>
https://code.google.com/p/chromium/codesearch#search/&q=TimestampToTimeT&sq=p...
> Then, use the functions in base/time.h to convert between time_t and
base::Time.
> wdyt?

I couldn't find a good way to build a proto from components/data_reduction_proxy
with an imported proto from third_party, so using those utility functions will
have two definitions of Duration/Timestamp, so having our own full definitions
seems necessary. I've cleaned up the implementation to similar to jeremyim's
implmenetation.

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
(right):

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:46: //
Returns the |Timestamp| representation of |time|. This is accurate to the
On 2016/05/27 23:01:59, tbansal1 wrote:
> This comment looks incorrect.

Done.

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

https://codereview.chromium.org/2025443002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc:29:
const base::Time& GetFutureTime() { return future_time_; }
On 2016/05/27 23:01:59, tbansal1 wrote:
> const method.

Done.

[tbansal1, 2016-05-31 23:31:12]

lgtm %  2 nits.

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
(right):

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:37: void
TimeDeltatoDuration(const base::TimeDelta& time_delta, Duration* duration);
nit: It might be easier to read if t in "to" is capitalized. This would also be
consistent to how you capitalize it in DurationToTimeDelta() below. Similar
comment for other functions below that have "to".

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc
(right):

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc:74:
"Second", base::Time::FromJsTime(1000), 1, 0,
May be use UnixEpoch() to write tests since that's what server will use. See
https://codereview.chromium.org/1509923003/diff/100001/components/data_reduct....

[RyanSturm, 2016-06-01 15:41:48]

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.h
(right):

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.h:37: void
TimeDeltatoDuration(const base::TimeDelta& time_delta, Duration* duration);
On 2016/05/31 23:31:12, tbansal1 wrote:
> nit: It might be easier to read if t in "to" is capitalized. This would also
be
> consistent to how you capitalize it in DurationToTimeDelta() below. Similar
> comment for other functions below that have "to".

Done.

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc
(right):

https://codereview.chromium.org/2025443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util_unittest.cc:74:
"Second", base::Time::FromJsTime(1000), 1, 0,
On 2016/05/31 23:31:12, tbansal1 wrote:
> May be use UnixEpoch() to write tests since that's what server will use. See
>
https://codereview.chromium.org/1509923003/diff/100001/components/data_reduct....

Done.

[RyanSturm, 2016-06-01 17:08:14]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-06-01 17:08:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2025443002/#ps40001
(title: "Fixing nits")

[commit-bot: I haz the power, 2016-06-01 17:08:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2025443002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2025443002/40001

[commit-bot: I haz the power, 2016-06-01 17:54:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-01 17:56:07]

Description was changed from

==========
Refactoring data reduction proxy some methods to common

Certain methods, especially for protobufs, can be moved to common in
data_reduction_proxy_utils.

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

to

==========
Refactoring data reduction proxy some methods to common

Certain methods, especially for protobufs, can be moved to common in
data_reduction_proxy_utils.

BUG=

Committed: https://crrev.com/c63ef5ab892e78c1279db7301d2d2aac4f959e76
Cr-Commit-Position: refs/heads/master@{#397175}
==========

[commit-bot: I haz the power, 2016-06-01 17:56:08]

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