Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(24)

Issue 285673002: Change value type of timeout and maximumAge in PositionOptions (Closed)

Created:
6 years, 7 months ago by kihong
Modified:
6 years, 7 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, mvanouwerkerk+watch_chromium.org, arv+blink, timvolodine, abarth-chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 13

Patch Set 2 : #

Total comments: 4

Patch Set 3 : #

Total comments: 3

Patch Set 4 : #

Messages

Total messages: 18 (0 generated)
Michael van Ouwerkerk
https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js File LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js (right): https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js#newcode57 LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:57: // Update the mock service to report an error. ...
6 years, 7 months ago (2014-05-13 16:15:16 UTC) #1
kihong
Thank you for reviewing, Michael. https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js File LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js (right): https://codereview.chromium.org/285673002/diff/1/LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js#newcode57 LayoutTests/fast/dom/Geolocation/script-tests/maximum-age.js:57: // Update the mock ...
6 years, 7 months ago (2014-05-14 03:44:24 UTC) #2
Michael van Ouwerkerk
Thanks for the changes Kihong! Just some minor nits in the comments. I think after ...
6 years, 7 months ago (2014-05-15 11:01:47 UTC) #3
Nils Barth (inactive)
On 2014/05/15 11:01:47, Michael van Ouwerkerk wrote: > Thanks for the changes Kihong! > > ...
6 years, 7 months ago (2014-05-16 00:32:30 UTC) #4
kihong
Thanks for reviewing Michael. > I think after this is all submitted, we should look ...
6 years, 7 months ago (2014-05-16 04:38:00 UTC) #5
kihong
On 2014/05/16 00:32:30, Nils Barth wrote: > On 2014/05/15 11:01:47, Michael van Ouwerkerk wrote: > ...
6 years, 7 months ago (2014-05-16 04:42:46 UTC) #6
Nils Barth (inactive)
On 2014/05/16 04:42:46, kihong wrote: > > In fact, could you just try setting [Clamp] ...
6 years, 7 months ago (2014-05-16 05:04:22 UTC) #7
Michael van Ouwerkerk
lgtm Thanks Kihong!
6 years, 7 months ago (2014-05-16 10:59:07 UTC) #8
Inactive
https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custom/V8GeolocationCustom.cpp File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right): https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custom/V8GeolocationCustom.cpp#newcode88 Source/bindings/v8/custom/V8GeolocationCustom.cpp:88: unsigned timeout; Could we use toUInt32(timeoutValue, Clamp, es) from ...
6 years, 7 months ago (2014-05-22 13:21:07 UTC) #9
kihong
https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custom/V8GeolocationCustom.cpp File Source/bindings/v8/custom/V8GeolocationCustom.cpp (right): https://codereview.chromium.org/285673002/diff/40001/Source/bindings/v8/custom/V8GeolocationCustom.cpp#newcode88 Source/bindings/v8/custom/V8GeolocationCustom.cpp:88: unsigned timeout; On 2014/05/22 13:21:07, Chris Dumez wrote: > ...
6 years, 7 months ago (2014-05-23 05:03:11 UTC) #10
haraken
LGTM
6 years, 7 months ago (2014-05-23 05:19:29 UTC) #11
kihong
The CQ bit was checked by kihong.kwon@samsung.com
6 years, 7 months ago (2014-05-23 06:00:29 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/285673002/60001
6 years, 7 months ago (2014-05-23 06:01:14 UTC) #13
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are ...
6 years, 7 months ago (2014-05-23 07:16:51 UTC) #14
kihong
The CQ bit was unchecked by kihong.kwon@samsung.com
6 years, 7 months ago (2014-05-23 08:20:03 UTC) #15
kihong
The CQ bit was checked by kihong.kwon@samsung.com
6 years, 7 months ago (2014-05-23 08:20:04 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kihong.kwon@samsung.com/285673002/60001
6 years, 7 months ago (2014-05-23 08:20:36 UTC) #17
commit-bot: I haz the power
6 years, 7 months ago (2014-05-23 10:26:35 UTC) #18
Message was sent while issue was closed.
Change committed as 174668

Powered by Google App Engine
This is Rietveld 408576698