Fix the end time bound in WebHistoryService to match the API

Fix the end time bound in WebHistoryService to match the API

Change the exclusive end time bound in WebHistoryService to be inclusive,
as required by the history.google.com API.

BUG=674165
Review-Url: https://codereview.chromium.org/2621143004
Cr-Commit-Position: refs/heads/master@{#443245}
Committed: https://chromium.googlesource.com/chromium/src/+/39f5b80c5358f69f8407557990beed449fa03e74

[msramek, 2017-01-12 11:46:07]

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

[commit-bot: I haz the power, 2017-01-12 11:46:24]

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, 2017-01-12 12:48:29]

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

[commit-bot: I haz the power, 2017-01-12 12:48:32]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-12 13:56:48]

Description was changed from

==========
Fix the end time bound in WebHistoryService to match the API

Change the exclusive end time bound in WebHistoryService to be inclusive,
as required by the history.google.com API.

Note that we only do this if the time bound is explicitly specified; if it
is not, and is thus interpreted as base::Time::Now(), we leave it as is,
since the current behavior is desirable - to fetch all history items until
now (including this moment).

BUG=674165
==========

to

==========
Fix the end time bound in WebHistoryService to match the API

Change the exclusive end time bound in WebHistoryService to be inclusive,
as required by the history.google.com API.

BUG=674165
==========

[msramek, 2017-01-12 14:11:52]

msramek@chromium.org changed reviewers:
+ sdefresne@chromium.org

[msramek, 2017-01-12 14:11:53]

Hi Sylvain,

Could you have a look?

As discussed in crbug.com/674165, this is mostly just a matter of calling the
API precisely. It doesn't really fix anything, since the bug is difficult to
trigger, but it shouldn't break anything either. There is also nothing to test
here, as this is an implementation detail of the server.

Thanks,
Martin

[sdefresne, 2017-01-12 14:32:13]

lgtm with some nitpicking on how to compute the end_time safely

https://codereview.chromium.org/2621143004/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.cc (right):

https://codereview.chromium.org/2621143004/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.cc:283:
std::min(base::Time::FromInternalValue(options.EffectiveEndTime() - 1),
The documentation of base::Time discourage using the internal value to do
arithmetic on it. What about instead using base::TimeDelta::FromMicroseconds(1)
to construct a TimeDelta and do the arithmetic safely?

base::Time end_time =
  std::min(
      base::Time::FromInternalValue(options.EffectiveEndTime()) -
          base::TimeDelta::FromMicroseconds(1),
      base::Time::Now());

I also noticed that QueryOptions::EffectiveEndTime() returns
std::numeric_limits<int64_t>::max() if end_time is not set. Should we really do
decrement by 1us then? What about instead the following:

base::Time end_time = options.end_time.is_null()
    ? base::Time::Now()
    : std::min(options.end_time - base::TimeDelta::FromMicroseconds(1),
               base::Time::Now());

[msramek, 2017-01-12 15:16:25]

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

[msramek, 2017-01-12 15:17:01]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-12 15:17:02]

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

[msramek, 2017-01-12 15:18:31]

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

[commit-bot: I haz the power, 2017-01-12 15:18:54]

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

[msramek, 2017-01-12 15:18:58]

https://codereview.chromium.org/2621143004/diff/1/components/history/core/bro...
File components/history/core/browser/web_history_service.cc (right):

https://codereview.chromium.org/2621143004/diff/1/components/history/core/bro...
components/history/core/browser/web_history_service.cc:283:
std::min(base::Time::FromInternalValue(options.EffectiveEndTime() - 1),
On 2017/01/12 14:32:13, sdefresne wrote:
> The documentation of base::Time discourage using the internal value to do
> arithmetic on it. What about instead using
base::TimeDelta::FromMicroseconds(1)
> to construct a TimeDelta and do the arithmetic safely?
> 
> base::Time end_time =
>   std::min(
>       base::Time::FromInternalValue(options.EffectiveEndTime()) -
>           base::TimeDelta::FromMicroseconds(1),
>       base::Time::Now());
> 
> I also noticed that QueryOptions::EffectiveEndTime() returns
> std::numeric_limits<int64_t>::max() if end_time is not set. Should we really
do
> decrement by 1us then? What about instead the following:
> 
> base::Time end_time = options.end_time.is_null()
>     ? base::Time::Now()
>     : std::min(options.end_time - base::TimeDelta::FromMicroseconds(1),
>                base::Time::Now());

Thanks, I missed that. Changed the arithmetic to TimeDelta.

The second point is probably not so important, as MAXINT64 and MAXINT64-1 are
both always going to be rounded down to base::Time::Now() except in one short
moment in far future. But it's still a valid nitpick, so let me fix that for
future generations :-)

[msramek, 2017-01-12 15:23:18]

The CQ bit was unchecked by msramek@chromium.org

[msramek, 2017-01-12 15:23:23]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-12 15:23:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2621143004/#ps40001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2017-01-12 15:23:33]

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, 2017-01-12 16:07:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484234603984250,
"parent_rev": "66c78e9b567b973bd9de347295c59fef47cb28c0", "commit_rev":
"39f5b80c5358f69f8407557990beed449fa03e74"}

[commit-bot: I haz the power, 2017-01-12 16:07:57]

Description was changed from

==========
Fix the end time bound in WebHistoryService to match the API

Change the exclusive end time bound in WebHistoryService to be inclusive,
as required by the history.google.com API.

BUG=674165
==========

to

==========
Fix the end time bound in WebHistoryService to match the API

Change the exclusive end time bound in WebHistoryService to be inclusive,
as required by the history.google.com API.

BUG=674165

Review-Url: https://codereview.chromium.org/2621143004
Cr-Commit-Position: refs/heads/master@{#443245}
Committed:
https://chromium.googlesource.com/chromium/src/+/39f5b80c5358f69f8407557990be...
==========

[commit-bot: I haz the power, 2017-01-12 16:07:59]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/39f5b80c5358f69f8407557990be...