Replace Dictionary with PositionOptions in geolocation/.

Replace Dictionary with PositionOptions in geolocation/.

Some geolocation apis take PositionOptions parameter using Dictionary class.
However, it is better to use IDL dictionary instead of Dictionary class.

BUG=403150
TEST=existing tests
R=bashi@chromium.org,haraken@chromium.org

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

[zino, 2014-11-20 14:09:52]

Dear all,

Please take a look.

Thank you.

[haraken, 2014-11-20 16:02:07]

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/GeoNotifier.h (right):

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeoNotifier.h:57: const PositionOptions m_options;

bashi-san: Just to confirm, is it safe to keep a dictionary value in an on-heap
object?

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/PositionOptions.cpp (left):

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/PositionOptions.cpp:36: m_maximumAge = maximumAge;

Do we have test cases for the edge case behavior (i.e., corner case behavior of
clamped values) of |timeout| and |maximumAge|? If not, we might want to add test
cases to layout tests.

[zino, 2014-11-20 17:27:15]

Thank you for review.

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/PositionOptions.cpp (left):

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/PositionOptions.cpp:36: m_maximumAge = maximumAge;
On 2014/11/20 16:02:07, haraken wrote:
> 
> Do we have test cases for the edge case behavior (i.e., corner case behavior
of
> clamped values) of |timeout| and |maximumAge|? If not, we might want to add
test
> cases to layout tests.

We already have tests.


|maximumAge|:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


|timeout|:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[bashi, 2014-11-20 23:10:05]

non-owner LGTM.

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
File Source/modules/geolocation/GeoNotifier.h (right):

https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
Source/modules/geolocation/GeoNotifier.h:57: const PositionOptions m_options;
On 2014/11/20 16:02:07, haraken wrote:
> 
> bashi-san: Just to confirm, is it safe to keep a dictionary value in an
on-heap
> object?

Yes, for now. But we might want to make dictionaries a thin wrapper of a
v8::Local at some point.

I'd recommend not to hold dictionaries on heap unless it is really needed.
(Again, it's fine for now).

[haraken, 2014-11-20 23:15:44]

On 2014/11/20 23:10:05, bashi1 wrote:
> non-owner LGTM.
> 
>
https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
> File Source/modules/geolocation/GeoNotifier.h (right):
> 
>
https://codereview.chromium.org/745503002/diff/20001/Source/modules/geolocati...
> Source/modules/geolocation/GeoNotifier.h:57: const PositionOptions m_options;
> On 2014/11/20 16:02:07, haraken wrote:
> > 
> > bashi-san: Just to confirm, is it safe to keep a dictionary value in an
> on-heap
> > object?
> 
> Yes, for now. But we might want to make dictionaries a thin wrapper of a
> v8::Local at some point.
> 
> I'd recommend not to hold dictionaries on heap unless it is really needed.
> (Again, it's fine for now).

Makes sense, LGTM.

[zino, 2014-11-20 23:47:14]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-11-20 23:48:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/745503002/20001

[commit-bot: I haz the power, 2014-11-21 01:07:57]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185735