downloads.query: parse numerical query properties as double, not int.

downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.

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

[lazyboy, 2016-06-24 00:26:02]

Description was changed from

==========
downloads.query: parse numerical queries properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.
==========

to

==========
downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.
==========

[lazyboy, 2016-06-24 00:26:50]

lazyboy@chromium.org changed reviewers:
+ asargent@chromium.org

[lazyboy, 2016-06-24 00:28:19]

lazyboy@chromium.org changed reviewers:
+ asanka@chromium.org

[lazyboy, 2016-06-24 00:28:19]

+asanka for chrome/browser/download/

[asargent_no_longer_on_chrome, 2016-06-24 20:32:19]

lgtm

[asanka, 2016-06-27 15:28:47]

On 2016/06/24 at 00:28:19, lazyboy wrote:
> +asanka for chrome/browser/download/

LGTM, but didn't this require any changes to download_query_unittest?

[lazyboy, 2016-06-27 17:32:38]

On 2016/06/27 15:28:47, asanka wrote:
> On 2016/06/24 at 00:28:19, lazyboy wrote:
> > +asanka for chrome/browser/download/
> 
> LGTM, but didn't this require any changes to download_query_unittest?

All of them passed when I tried. Will see how the CQ likes the change.

[asanka, 2016-06-27 17:51:15]

On 2016/06/27 at 17:32:38, lazyboy wrote:
> On 2016/06/27 15:28:47, asanka wrote:
> > On 2016/06/24 at 00:28:19, lazyboy wrote:
> > > +asanka for chrome/browser/download/
> > 
> > LGTM, but didn't this require any changes to download_query_unittest?
> 
> All of them passed when I tried. Will see how the CQ likes the change.

That's probably a sign that this code doesn't have good test coverage.  :-)
Could you add some that verify that double values work with AddFilter? Also, you
should tests with large values that don't fit in a 32-bit int.

Also, please update download_query.h. It still documents the affected values as
being of 'int' type. While GetDouble() works for ints, we should document that
'double's also work.

[lazyboy, 2016-06-27 20:47:58]

On 2016/06/27 17:51:15, asanka wrote:
> On 2016/06/27 at 17:32:38, lazyboy wrote:
> > On 2016/06/27 15:28:47, asanka wrote:
> > > On 2016/06/24 at 00:28:19, lazyboy wrote:
> > > > +asanka for chrome/browser/download/
> > > 
> > > LGTM, but didn't this require any changes to download_query_unittest?
> > 
> > All of them passed when I tried. Will see how the CQ likes the change.
> 
> That's probably a sign that this code doesn't have good test coverage.  :-)
> Could you add some that verify that double values work with AddFilter? Also,
you
> should tests with large values that don't fit in a 32-bit int.
I've added few tests TotalBytes{"","Less","Greater"}, with doubles > 2^32.
> 
> Also, please update download_query.h. It still documents the affected values
as
> being of 'int' type. While GetDouble() works for ints, we should document that
> 'double's also work.
Done.

[asanka, 2016-06-27 20:50:26]

On 2016/06/27 at 20:47:58, lazyboy wrote:
> On 2016/06/27 17:51:15, asanka wrote:
> > On 2016/06/27 at 17:32:38, lazyboy wrote:
> > > On 2016/06/27 15:28:47, asanka wrote:
> > > > On 2016/06/24 at 00:28:19, lazyboy wrote:
> > > > > +asanka for chrome/browser/download/
> > > > 
> > > > LGTM, but didn't this require any changes to download_query_unittest?
> > > 
> > > All of them passed when I tried. Will see how the CQ likes the change.
> > 
> > That's probably a sign that this code doesn't have good test coverage.  :-)
> > Could you add some that verify that double values work with AddFilter? Also,
you
> > should tests with large values that don't fit in a 32-bit int.
> I've added few tests TotalBytes{"","Less","Greater"}, with doubles > 2^32.
> > 
> > Also, please update download_query.h. It still documents the affected values
as
> > being of 'int' type. While GetDouble() works for ints, we should document
that
> > 'double's also work.
> Done.

Thanks. LGTM

[lazyboy, 2016-06-27 23:15:35]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-06-27 23:15:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2092963002/#ps40001
(title: "add one more edge case test")

[commit-bot: I haz the power, 2016-06-27 23:16:38]

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-06-27 23:48:33]

Description was changed from

==========
downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.
==========

to

==========
downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.
==========

[commit-bot: I haz the power, 2016-06-27 23:48:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-27 23:51:03]

Description was changed from

==========
downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.
==========

to

==========
downloads.query: parse numerical query properties as double, not int.

In r244404, we changed these from ints to doubles. However, we still
seem to parse these through Value::GetAsInteger. This always fails.
Change it to use Value::GetAsDouble instead.

BUG=617435
Test=See bug description for repro test case.

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

[commit-bot: I haz the power, 2016-06-27 23:51:04]

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