Change value type of timeout and maximumAge in PositionOptions

Change value type of timeout and maximumAge in PositionOptions

Regarding Geolocation API recommendation, The type of timeout and maximumAge
are changed to unsigned int from int, therefore those values could be over
int range.(http://www.w3.org/TR/geolocation-API/#position_options_interface)

BUG=368184
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174668

[Michael van Ouwerkerk, 2014-05-13 16:15:16]

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
File LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js (right):

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:57: // Update the
mock service to report an error.
Fix this comment, the line below does not set an error.

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:70: // Update the
mock service to report an error.
Actually, this is strange... maximum-age-expected.txt indicates that this test
gets no error. What's going on here?

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right):

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:93: // Wrap to int32 and force
non-negative to match behavior of window.setTimeout.
Update this comment.

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:94:
options->setTimeout(std::max<unsigned>(0, timeoutUint32->Value()));
The specification specifies [Clamp] so that overflow does not become 0, but max
value. Is that correctly implemented here? Is there a test for it?

See
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Clamp-...

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:119: // Wrap to int32 and
force non-negative to match behavior of window.setTimeout.
Update comment.

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
File Source/modules/geolocation/PositionOptions.h (right):

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.h:49: ASSERT(timeout >= 0);
Would this assert ever trigger on an unsigned int?

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.h:62: ASSERT(age >= 0);
Same here.

[kihong, 2014-05-14 03:44:24]

Thank you for reviewing, Michael.

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
File LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js (right):

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:57: // Update the
mock service to report an error.
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Fix this comment, the line below does not set an error.

Done.

https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geoloca...
LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:70: // Update the
mock service to report an error.
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Actually, this is strange... maximum-age-expected.txt indicates that this test
> gets no error. What's going on here?

As you mentioned, this is slightly strange there might be a timing issue for
checking error, but after adding testOverSignedIntMaximumAge() there is no
timing issue for adding error, So I changed test case properly.

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right):

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:93: // Wrap to int32 and force
non-negative to match behavior of window.setTimeout.
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Update this comment.

Done.

https://codereview.chromium.org/285673002/diff/1/Source/bindings/v8/custom/V8...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:119: // Wrap to int32 and
force non-negative to match behavior of window.setTimeout.
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Update comment.

Done.

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
File Source/modules/geolocation/PositionOptions.h (right):

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.h:49: ASSERT(timeout >= 0);
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Would this assert ever trigger on an unsigned int?

Done.

https://codereview.chromium.org/285673002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.h:62: ASSERT(age >= 0);
On 2014/05/13 16:15:17, Michael van Ouwerkerk wrote:
> Same here.

Done.

[Michael van Ouwerkerk, 2014-05-15 11:01:47]

Thanks for the changes Kihong!

Just some minor nits in the comments.

I think after this is all submitted, we should look at deleting these custom
bindings as they are a pain. They are very old, and no longer necessary. They
can mostly be generated from IDL now.

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
File LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js (right):

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:84:
internals.setGeolocationPositionUnavailableError(document, mockMessage);
I understand now... this is a non-fatal error so the cached position will be
used:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
File
LayoutTests/fast/dom/Geolocation/script-tests/timeout-over-max-of-unisgned.js
(right):

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
LayoutTests/fast/dom/Geolocation/script-tests/timeout-over-max-of-unisgned.js:1:
description("Tests that when timeout value is over than maximum of unsigned, the
success callback is called as expected.");
s/over than/over/

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
File LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned-expected.txt
(right):

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned-expected.txt:1:
Tests that when timeout value is over than maximum of unsigned, the success
callback is called as expected.
s/over than/over/

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
File LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned.html (right):

https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned.html:7: <script
src="script-tests/timeout-over-max-of-unisgned.js"></script>
s/unisgned/unsigned/

[Nils Barth (inactive), 2014-05-16 00:32:30]

On 2014/05/15 11:01:47, Michael van Ouwerkerk wrote:
> Thanks for the changes Kihong!
> 
> Just some minor nits in the comments.
> 
> I think after this is all submitted, we should look at deleting these custom
> bindings as they are a pain. They are very old, and no longer necessary. They
> can mostly be generated from IDL now.

In fact, could you just try setting [Clamp] in Geolocation.idl instead of
[Custom] and see if it works?
(That's all this is doing.)

Also, in Geolocation.idl could you:
1. Link to the dictionary bug
I.e., change:
// FIXME: should be: PositionOptions options (need PositionOptions.idl)
...to:
// FIXME: should be: PositionOptions options: need PositionOptions.idl and
dictionary http:/crbug.com/321462

2. Link to PositionOptions
http://www.w3.org/TR/geolocation-API/#position-options

3. Alphabetize the extended attributes

Thanks!

[kihong, 2014-05-16 04:38:00]

Thanks for reviewing Michael.

> I think after this is all submitted, we should look at deleting these custom
> bindings as they are a pain. They are very old, and no longer necessary. They
> can mostly be generated from IDL now.

Sure, I will take care of it.

>
https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
> LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:84:
> internals.setGeolocationPositionUnavailableError(document, mockMessage);
> I understand now... this is a non-fatal error so the cached position will be
> used:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yes

>
https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
>
LayoutTests/fast/dom/Geolocation/script-tests/timeout-over-max-of-unisgned.js:1:
> description("Tests that when timeout value is over than maximum of unsigned,
the
> success callback is called as expected.");
> s/over than/over/

Done

>
https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
> LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned-expected.txt:1:
> Tests that when timeout value is over than maximum of unsigned, the success
> callback is called as expected.
> s/over than/over/

Done

>
https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
> File LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned.html
(right):
> 
>
https://codereview.chromium.org/285673002/diff/20001/LayoutTests/fast/dom/Geo...
> LayoutTests/fast/dom/Geolocation/timeout-over-max-of-unisgned.html:7: <script
> src="script-tests/timeout-over-max-of-unisgned.js"></script>
> s/unisgned/unsigned/

Done

[kihong, 2014-05-16 04:42:46]

On 2014/05/16 00:32:30, Nils Barth wrote:
> On 2014/05/15 11:01:47, Michael van Ouwerkerk wrote:
> > Thanks for the changes Kihong!
> > 
> > Just some minor nits in the comments.
> > 
> > I think after this is all submitted, we should look at deleting these custom
> > bindings as they are a pain. They are very old, and no longer necessary.
They
> > can mostly be generated from IDL now.
> 
> In fact, could you just try setting [Clamp] in Geolocation.idl instead of
> [Custom] and see if it works?
> (That's all this is doing.)

This is not working simply.
because Geolocation.cpp doesn't take care of exception and 
I think [Clamp] need to be adopted to OpsitionOptions.idl instead of
Geolocation.idl.

> 
> Also, in Geolocation.idl could you:
> 1. Link to the dictionary bug
> I.e., change:
> // FIXME: should be: PositionOptions options (need PositionOptions.idl)
> ...to:
> // FIXME: should be: PositionOptions options: need PositionOptions.idl and
> dictionary http:/crbug.com/321462
> 
> 2. Link to PositionOptions
> http://www.w3.org/TR/geolocation-API/#position-options
> 
> 3. Alphabetize the extended attributes

I will take care of this with other patches for aligning Geolocation API to the
latest spec.

Thanks Nils :)

[Nils Barth (inactive), 2014-05-16 05:04:22]

On 2014/05/16 04:42:46, kihong wrote:
> > In fact, could you just try setting [Clamp] in Geolocation.idl instead of
> > [Custom] and see if it works?
> > (That's all this is doing.)
> 
> This is not working simply.
> because Geolocation.cpp doesn't take care of exception and 
> I think [Clamp] need to be adopted to PositionOptions.idl instead of
> Geolocation.idl.

OIC: PositionOptions is custom, and this makes Geolocation custom.

PositionOptions is a dictionary, so properly it should actually be implemented
as
v8/PositionOptions.{h,cpp}
or
v8/custom/V8PositionOptions.{h,cpp}
(inheriting from DictionaryBase or something),
just like
v8/Dictionary.{h,cpp}
...in which case only PositionOptions has Blink-side code:
the actual Geolocation bindings are generic.

However, it's probably better to just implement IDL dictionary;
this CL is fine.

> I will take care of this with other patches for aligning Geolocation API to
the
> latest spec.

Cool! (^.-)b

[Michael van Ouwerkerk, 2014-05-16 10:59:07]

lgtm Thanks Kihong!

[Inactive, 2014-05-22 13:21:07]

https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right):

https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:88: unsigned timeout;
Could we use toUInt32(timeoutValue, Clamp, es) from V8Bindings.h?

https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:122: unsigned maximumAge;
Ditto.

[kihong, 2014-05-23 05:03:11]

https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right):

https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8GeolocationCustom.cpp:88: unsigned timeout;
On 2014/05/22 13:21:07, Chris Dumez wrote:
> Could we use toUInt32(timeoutValue, Clamp, es) from V8Bindings.h?

Done, that's much simpler than now.
Thanks chris :)

[haraken, 2014-05-23 05:19:29]

LGTM

[kihong, 2014-05-23 06:00:29]

The CQ bit was checked by kihong.kwon@samsung.com

[commit-bot: I haz the power, 2014-05-23 06:01:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/285673002/60001

[commit-bot: I haz the power, 2014-05-23 07:16:51]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8494)

[kihong, 2014-05-23 08:20:03]

The CQ bit was unchecked by kihong.kwon@samsung.com

[kihong, 2014-05-23 08:20:04]

The CQ bit was checked by kihong.kwon@samsung.com

[commit-bot: I haz the power, 2014-05-23 08:20:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/285673002/60001

[commit-bot: I haz the power, 2014-05-23 10:26:35]

Change committed as 174668