Use "Dictionary" for PositionOptions instead of Custom binding

Use "Dictionary" for PositionOptions instead of Custom binding

getCurrentPosition and watchPosition of Geolocation API use "Custom"
binding for implementing PositionOptions.
But it could be implemented using "Dictionary" instead of "Custom"
and we can remove custom binding file(V8GeolocationCustom.cpp) for that.

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

[kihong, 2014-06-17 04:34:44]

PTAL

[haraken, 2014-06-17 04:39:32]

This is great CL overall! I'm happy to take a close look if Michael is OK with
the behavior change.

Also I think you can remove a lot of code in bindings/v8/V8Callback.h that is
used only by V8GeolocationCustom.cpp.

[kihong, 2014-06-17 05:36:35]

On 2014/06/17 04:39:32, haraken wrote:
> This is great CL overall! I'm happy to take a close look if Michael is OK with
> the behavior change.
> 
> Also I think you can remove a lot of code in bindings/v8/V8Callback.h that is
> used only by V8GeolocationCustom.cpp.

I am happy to here that. Thanks Kentaro. :)
I will remove whole codes which are only used for V8GeolocationCustom.cpp after
this.

[Michael van Ouwerkerk, 2014-06-17 11:13:56]

Getting rid of these custom bindings is great! Thanks!

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:26: double maximumAge;
Why a double? m_maximumAge is an int. I'm not sure I understand this bit of V8
binding correctly.

[kihong, 2014-06-17 13:13:10]

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:26: double maximumAge;
On 2014/06/17 11:13:56, Michael van Ouwerkerk wrote:
> Why a double? m_maximumAge is an int. I'm not sure I understand this bit of V8
> binding correctly.

This value is a "[Clamp] unsigned long" therefore I suppose to be input larger
than unsigned. That's why I use double for
this.(http://dev.w3.org/geo/api/spec-source.html#position_options_interface)

[Inactive, 2014-06-17 13:49:13]

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
PositionOptions::create(const Dictionary& options)
Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe / leak
prone.

[sof, 2014-06-17 14:05:05]

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
PositionOptions::create(const Dictionary& options)
On 2014/06/17 13:49:13, Chris Dumez wrote:
> Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe /
leak
> prone.

geolocation/ is using Oilpan by default.

[kihong, 2014-06-17 14:46:18]

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
PositionOptions::create(const Dictionary& options)
On 2014/06/17 13:49:13, Chris Dumez wrote:
> Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe /
leak
> prone.

As sof said geolocation is using Oilpan already, but it's not a reason why I am
using raw pointer.

Basically legacy geolocation API implementation have used so many raw pointers.
And PositionOptions raw pointers are used by some other functions also.
I agree with your opinion that it's unsafe, but I don't think it is in this
patch's scope.
That's why I'm using raw pointer.

And I stacked a bug for that(http://crbug.com/385638) and I will change them
soon.

[kihong, 2014-06-18 03:57:01]

On 2014/06/17 14:46:18, kihong wrote:
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> File Source/modules/geolocation/PositionOptions.cpp (right):
> 
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
> PositionOptions::create(const Dictionary& options)
> On 2014/06/17 13:49:13, Chris Dumez wrote:
> > Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe /
> leak
> > prone.
> 
> As sof said geolocation is using Oilpan already, but it's not a reason why I
am
> using raw pointer.
> 
> Basically legacy geolocation API implementation have used so many raw
pointers.
> And PositionOptions raw pointers are used by some other functions also.
> I agree with your opinion that it's unsafe, but I don't think it is in this
> patch's scope.
> That's why I'm using raw pointer.
> 
> And I stacked a bug for that(http://crbug.com/385638) and I will change them
> soon.

PositionOptions raw pointer is already considered by "GeoNotifier" for garbage
collection.
Therefore raw pointer looks proper here.
And Stacked bug(http://crbug.com/385638) is meaningless. I will close it.

[Inactive, 2014-06-18 12:22:44]

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

https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
PositionOptions::create(const Dictionary& options)
On 2014/06/17 14:46:18, kihong wrote:
> On 2014/06/17 13:49:13, Chris Dumez wrote:
> > Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe /
> leak
> > prone.
> 
> As sof said geolocation is using Oilpan already, but it's not a reason why I
am
> using raw pointer.
> 
> Basically legacy geolocation API implementation have used so many raw
pointers.
> And PositionOptions raw pointers are used by some other functions also.
> I agree with your opinion that it's unsafe, but I don't think it is in this
> patch's scope.
> That's why I'm using raw pointer.
> 
> And I stacked a bug for that(http://crbug.com/385638) and I will change them
> soon.

If this is how oilpan code looks when done porting, I have some concerns.
Passing around raw pointers like this looks unsafe and leak-prone. I thought
oilpan was going to pass around "handles".

[haraken, 2014-06-18 12:29:01]

On 2014/06/18 12:22:44, Chris Dumez wrote:
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> File Source/modules/geolocation/PositionOptions.cpp (right):
> 
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
> PositionOptions::create(const Dictionary& options)
> On 2014/06/17 14:46:18, kihong wrote:
> > On 2014/06/17 13:49:13, Chris Dumez wrote:
> > > Are we returning a raw pointer because of oilpan? IMHO, this looks unsafe
/
> > leak
> > > prone.
> > 
> > As sof said geolocation is using Oilpan already, but it's not a reason why I
> am
> > using raw pointer.
> > 
> > Basically legacy geolocation API implementation have used so many raw
> pointers.
> > And PositionOptions raw pointers are used by some other functions also.
> > I agree with your opinion that it's unsafe, but I don't think it is in this
> > patch's scope.
> > That's why I'm using raw pointer.
> > 
> > And I stacked a bug for that(http://crbug.com/385638) and I will change them
> > soon.
> 
> If this is how oilpan code looks when done porting, I have some concerns.
> Passing around raw pointers like this looks unsafe and leak-prone. I thought
> oilpan was going to pass around "handles".

Oilpan uses:

- a Member for a pointer from an on-heap object to an on-heap object.
- a Persistent for a pointer from an off-heap object to an on-heap object.
- a raw pointer for a pointer on the stack.

Oilpan's GC does conservative scanning on the stack and finds all on-stack
pointers conservatively.

> I have some concerns. Passing around raw pointers like this looks unsafe and
leak-prone.

What's your concern specifically?

[Inactive, 2014-06-19 13:43:37]

On 2014/06/18 12:29:01, haraken wrote:
> On 2014/06/18 12:22:44, Chris Dumez wrote:
> >
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> > File Source/modules/geolocation/PositionOptions.cpp (right):
> > 
> >
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> > Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
> > PositionOptions::create(const Dictionary& options)
> > On 2014/06/17 14:46:18, kihong wrote:
> > > On 2014/06/17 13:49:13, Chris Dumez wrote:
> > > > Are we returning a raw pointer because of oilpan? IMHO, this looks
unsafe
> /
> > > leak
> > > > prone.
> > > 
> > > As sof said geolocation is using Oilpan already, but it's not a reason why
I
> > am
> > > using raw pointer.
> > > 
> > > Basically legacy geolocation API implementation have used so many raw
> > pointers.
> > > And PositionOptions raw pointers are used by some other functions also.
> > > I agree with your opinion that it's unsafe, but I don't think it is in
this
> > > patch's scope.
> > > That's why I'm using raw pointer.
> > > 
> > > And I stacked a bug for that(http://crbug.com/385638) and I will change
them
> > > soon.
> > 
> > If this is how oilpan code looks when done porting, I have some concerns.
> > Passing around raw pointers like this looks unsafe and leak-prone. I thought
> > oilpan was going to pass around "handles".
> 
> Oilpan uses:
> 
> - a Member for a pointer from an on-heap object to an on-heap object.
> - a Persistent for a pointer from an off-heap object to an on-heap object.
> - a raw pointer for a pointer on the stack.
> 
> Oilpan's GC does conservative scanning on the stack and finds all on-stack
> pointers conservatively.
> 
> > I have some concerns. Passing around raw pointers like this looks unsafe and
> leak-prone.
> 
> What's your concern specifically?

Will *all* object types be garbage collected? If not, I think this makes memory
management difficult because the developer needs to take care of freeing some
object types and not others. If they are all garbage collected (eventually) then
this is less of an issue and this is just something I'll have to get used to
(passing raw pointers around, not caring about memory ownership). At least
during the transition period, having some types garbage collected (like
PositionOptions) and others not (like Node) is confusing IMHO.

[haraken, 2014-06-19 13:48:44]

On 2014/06/19 13:43:37, Chris Dumez wrote:
> On 2014/06/18 12:29:01, haraken wrote:
> > On 2014/06/18 12:22:44, Chris Dumez wrote:
> > >
> >
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> > > File Source/modules/geolocation/PositionOptions.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/336343002/diff/1/Source/modules/geolocation/P...
> > > Source/modules/geolocation/PositionOptions.cpp:13: PositionOptions*
> > > PositionOptions::create(const Dictionary& options)
> > > On 2014/06/17 14:46:18, kihong wrote:
> > > > On 2014/06/17 13:49:13, Chris Dumez wrote:
> > > > > Are we returning a raw pointer because of oilpan? IMHO, this looks
> unsafe
> > /
> > > > leak
> > > > > prone.
> > > > 
> > > > As sof said geolocation is using Oilpan already, but it's not a reason
why
> I
> > > am
> > > > using raw pointer.
> > > > 
> > > > Basically legacy geolocation API implementation have used so many raw
> > > pointers.
> > > > And PositionOptions raw pointers are used by some other functions also.
> > > > I agree with your opinion that it's unsafe, but I don't think it is in
> this
> > > > patch's scope.
> > > > That's why I'm using raw pointer.
> > > > 
> > > > And I stacked a bug for that(http://crbug.com/385638) and I will change
> them
> > > > soon.
> > > 
> > > If this is how oilpan code looks when done porting, I have some concerns.
> > > Passing around raw pointers like this looks unsafe and leak-prone. I
thought
> > > oilpan was going to pass around "handles".
> > 
> > Oilpan uses:
> > 
> > - a Member for a pointer from an on-heap object to an on-heap object.
> > - a Persistent for a pointer from an off-heap object to an on-heap object.
> > - a raw pointer for a pointer on the stack.
> > 
> > Oilpan's GC does conservative scanning on the stack and finds all on-stack
> > pointers conservatively.
> > 
> > > I have some concerns. Passing around raw pointers like this looks unsafe
and
> > leak-prone.
> > 
> > What's your concern specifically?
> 
> Will *all* object types be garbage collected? If not, I think this makes
memory
> management difficult because the developer needs to take care of freeing some
> object types and not others. If they are all garbage collected (eventually)
then
> this is less of an issue and this is just something I'll have to get used to
> (passing raw pointers around, not caring about memory ownership). At least
> during the transition period, having some types garbage collected (like
> PositionOptions) and others not (like Node) is confusing IMHO.

Eventually we're planning to move almost all RefCounted objects to the heap. At
least all DOM objects that have IDL interfaces will be moved to the heap. Most
(but not all) OwnPtrs will be moved to the heap as well.

I do agree that the transition period is a pain, and we're trying to make the
transition period as short as possible :)

[kihong, 2014-06-19 15:01:08]

OK... I think discussion regarding oilpan is almost finished in this phase. :-)
Michael, could you take a look at this patch please?

[haraken, 2014-06-19 15:18:36]

On 2014/06/19 15:01:08, kihong wrote:
> OK... I think discussion regarding oilpan is almost finished in this phase.
:-)
> Michael, could you take a look at this patch please?

I'm sorry for interrupting this thread with oilpan... Either way, removing
custom bindings for GeoLocation is great, so I'm happy to take a close look once
Michael is happy with this CL.

Christophie: We can discuss more about oilpan in this bug
(https://code.google.com/p/chromium/issues/detail?id=340522) if you have
concerns :)

[kihong, 2014-06-19 15:34:29]

On 2014/06/19 15:18:36, haraken wrote:
> On 2014/06/19 15:01:08, kihong wrote:
> > OK... I think discussion regarding oilpan is almost finished in this phase.
> :-)
> > Michael, could you take a look at this patch please?
> 
> I'm sorry for interrupting this thread with oilpan... Either way, removing
> custom bindings for GeoLocation is great, so I'm happy to take a close look
once
> Michael is happy with this CL.
> 
> Christophie: We can discuss more about oilpan in this bug
> (https://code.google.com/p/chromium/issues/detail?id=340522) if you have
> concerns :)

Don't worry about it and this is very good change to me understanding oilpan
also.
As you said, if Chris has more concerns, IMHO, it's better to discuss in proper
bug.
Thank you very much for your understanding and reviewing. Kentaro. :)

[Inactive, 2014-06-19 15:38:37]

On 2014/06/19 15:34:29, kihong wrote:
> On 2014/06/19 15:18:36, haraken wrote:
> > On 2014/06/19 15:01:08, kihong wrote:
> > > OK... I think discussion regarding oilpan is almost finished in this
phase.
> > :-)
> > > Michael, could you take a look at this patch please?
> > 
> > I'm sorry for interrupting this thread with oilpan... Either way, removing
> > custom bindings for GeoLocation is great, so I'm happy to take a close look
> once
> > Michael is happy with this CL.
> > 
> > Christophie: We can discuss more about oilpan in this bug
> > (https://code.google.com/p/chromium/issues/detail?id=340522) if you have
> > concerns :)
> 
> Don't worry about it and this is very good change to me understanding oilpan
> also.
> As you said, if Chris has more concerns, IMHO, it's better to discuss in
proper
> bug.
> Thank you very much for your understanding and reviewing. Kentaro. :)

Yes, sorry for hijacking Kihong's issue :)

[kihong, 2014-06-19 15:40:23]

On 2014/06/19 15:38:37, Chris Dumez wrote:
> On 2014/06/19 15:34:29, kihong wrote:
> > On 2014/06/19 15:18:36, haraken wrote:
> > > On 2014/06/19 15:01:08, kihong wrote:
> > > > OK... I think discussion regarding oilpan is almost finished in this
> phase.
> > > :-)
> > > > Michael, could you take a look at this patch please?
> > > 
> > > I'm sorry for interrupting this thread with oilpan... Either way, removing
> > > custom bindings for GeoLocation is great, so I'm happy to take a close
look
> > once
> > > Michael is happy with this CL.
> > > 
> > > Christophie: We can discuss more about oilpan in this bug
> > > (https://code.google.com/p/chromium/issues/detail?id=340522) if you have
> > > concerns :)
> > 
> > Don't worry about it and this is very good change to me understanding oilpan
> > also.
> > As you said, if Chris has more concerns, IMHO, it's better to discuss in
> proper
> > bug.
> > Thank you very much for your understanding and reviewing. Kentaro. :)
> 
> Yes, sorry for hijacking Kihong's issue :)

Never mind, Chris.
I always thank you for your active reviewing. :)

[Michael van Ouwerkerk, 2014-06-20 08:47:25]

Very nice, thank you Kihong. My apologies for the delay in reviewing.

Please do follow up with a cleanup patch for V8Callback.h because it mentions
V8GeolocationCustom.cpp which will be deleted now.

lgtm

[kihong, 2014-06-20 08:50:17]

On 2014/06/20 08:47:25, Michael van Ouwerkerk wrote:
> Very nice, thank you Kihong. My apologies for the delay in reviewing.
> 
> Please do follow up with a cleanup patch for V8Callback.h because it mentions
> V8GeolocationCustom.cpp which will be deleted now.
> 
> lgtm

Thank you Michael.
I will do that very soon. :)

[haraken, 2014-06-23 01:08:50]

Sorry about the review delay.

This CL changes existing behavior for corner cases where null/undefined/empty
values are passed to GeoLocation APIs. However, given that the new behavior
aligns with other DOM methods and Michael is fine with the changes in layout
tests, the CL LGTM.

[kihong, 2014-06-23 02:09:08]

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

[commit-bot: I haz the power, 2014-06-23 02:09:16]

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

[commit-bot: I haz the power, 2014-06-23 03:11:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/13060)

[commit-bot: I haz the power, 2014-06-23 03:47:58]

Change committed as 176719