Support for [Clamp] and [EnforceRange] to IDL dictionary.

Support for [Clamp] and [EnforceRange] to IDL dictionary.

We should be able to use [Clamp][1] and [EnforceRange][2] on dictionary
as well as interface or method. They will be used in some dictionaries such as
Geolocation API[3], Web Crypto API[4] and so on.

[1] http://www.w3.org/TR/WebIDL/#Clamp
[2] http://www.w3.org/TR/WebIDL/#EnforceRange
[3] http://dev.w3.org/geo/api/spec-source.html#position-options
[4] http://www.w3.org/TR/WebCryptoAPI/#dfn-RsaKeyGenParams


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

[zino, 2014-11-16 04:14:43]

Patchset #1 (id:1) has been deleted

[zino, 2014-11-16 04:14:51]

Patchset #1 (id:20001) has been deleted

[zino, 2014-11-16 04:15:01]

Patchset #1 (id:40001) has been deleted

[zino, 2014-11-16 04:15:09]

Patchset #1 (id:60001) has been deleted

[zino, 2014-11-16 04:15:18]

Patchset #1 (id:80001) has been deleted

[zino, 2014-11-16 04:15:39]

Patchset #1 (id:100001) has been deleted

[zino, 2014-11-16 04:17:14]

jinho.bang@samsung.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[zino, 2014-11-16 04:34:43]

Dear all,

Please take a look.

Thank you.

[haraken, 2014-11-17 00:22:19]

bashi-san is more familiar with the code.

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:33: if
(dictionary.getKey("{{member.name}}", {{member.name}}Value) &&
!isUndefinedOrNull({{member.name}}Value)) {

Is getWithUndefinedOrNullCheck still needed? It's confusing to have two ways to
get a dictionary value: getWithUndefinedOrNullCheck and 'getKey &&
isUndefinedOrNull'?

[bashi, 2014-11-17 02:20:45]

Thank you for the CL!

I believe we have a case in which we need [Clamp] support for dictionary (I
forgot the name of interface though...). Could you elaborate the motivation of
this CL in "description"?

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:30: v8::TryCatch block;
I think you can fix this FIXME in this CL or a follow-up CL:)

v8_value_to_local_cpp_value can handle ExceptionState.

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:33: if
(dictionary.getKey("{{member.name}}", {{member.name}}Value) &&
!isUndefinedOrNull({{member.name}}Value)) {
On 2014/11/17 00:22:19, haraken wrote:
> 
> Is getWithUndefinedOrNullCheck still needed? It's confusing to have two ways
to
> get a dictionary value: getWithUndefinedOrNullCheck and 'getKey &&
> isUndefinedOrNull'?

Also, we can remove Dictionary here if you use v8_value_to_local_cpp_value.
Dictionary::getKey() is just a wrapper of getting a property from a V8 object.
You can add a static function instead (which will take v8::Local<v8::Object> and
String, returning a v8::Local<v8::Value>.

[bashi, 2014-11-19 11:17:21]

Hi zino,

Are you still working on this? If not, I'll willing to take over your change.

[zino, 2014-11-19 11:22:20]

On 2014/11/19 11:17:21, bashi1 wrote:
> Hi zino,
> 
> Are you still working on this? If not, I'll willing to take over your change.

Hi Bashi,

Could you please wait for me?
I'm working on this now. (Sorry for delay because of crash from my changes..)

[bashi, 2014-11-19 13:16:51]

On 2014/11/19 11:22:20, zino wrote:
> On 2014/11/19 11:17:21, bashi1 wrote:
> > Hi zino,
> > 
> > Are you still working on this? If not, I'll willing to take over your
change.
> 
> Hi Bashi,
> 
> Could you please wait for me?
> I'm working on this now. (Sorry for delay because of crash from my changes..)

Sure. No hurry, I just wanted to make sure you are still interested in this CL
:)

[zino, 2014-11-19 18:12:19]

Thank you for review :)

I've uploaded new patch set.
Please take a look.

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:30: v8::TryCatch block;
On 2014/11/17 02:20:45, bashi1 wrote:
> I think you can fix this FIXME in this CL or a follow-up CL:)
> 
> v8_value_to_local_cpp_value can handle ExceptionState.

I'm trying to fix this but I'm not familiar with this part.
How can I handle the ExceptionState without v8::TryCatch.
Please see Patch Set 4 (not Set 5) and
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:33: if
(dictionary.getKey("{{member.name}}", {{member.name}}Value) &&
!isUndefinedOrNull({{member.name}}Value)) {
On 2014/11/17 02:20:45, bashi1 wrote:
> On 2014/11/17 00:22:19, haraken wrote:
> > 
> > Is getWithUndefinedOrNullCheck still needed? It's confusing to have two ways
> to
> > get a dictionary value: getWithUndefinedOrNullCheck and 'getKey &&
> > isUndefinedOrNull'?
> 
> Also, we can remove Dictionary here if you use v8_value_to_local_cpp_value.
> Dictionary::getKey() is just a wrapper of getting a property from a V8 object.
> You can add a static function instead (which will take v8::Local<v8::Object>
and
> String, returning a v8::Local<v8::Value>.

Done.

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (left):

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:55: return;
This is my mistake.. :-(

[bashi, 2014-11-20 02:18:02]

non-owner LGTM with one comment that should be addressed.

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/724733002/diff/120001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:30: v8::TryCatch block;
On 2014/11/19 18:12:19, zino wrote:
> On 2014/11/17 02:20:45, bashi1 wrote:
> > I think you can fix this FIXME in this CL or a follow-up CL:)
> > 
> > v8_value_to_local_cpp_value can handle ExceptionState.
> 
> I'm trying to fix this but I'm not familiar with this part.
> How can I handle the ExceptionState without v8::TryCatch.
> Please see Patch Set 4 (not Set 5) and
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

Hmm, let me investigate it later. I think I can fix it. This CL shouldn't affect
custom elements tests so please leave this as is. Sorry for the noise.

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (left):

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:55: return;
On 2014/11/19 18:12:19, zino wrote:
> This is my mistake.. :-(

I'm not sure why you removed this return. Early exit when an error occurs is a
good thing, no?

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:28: 
Remove unnecessary blank line.

[zino, 2014-11-20 02:40:43]

Thank you for review :)

I'm waiting for owner's approval.

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
File Source/bindings/templates/dictionary_v8.cpp (left):

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/templat...
Source/bindings/templates/dictionary_v8.cpp:55: return;
On 2014/11/20 02:18:02, bashi1 wrote:
> On 2014/11/19 18:12:19, zino wrote:
> > This is my mistake.. :-(
> 
> I'm not sure why you removed this return. Early exit when an error occurs is a
> good thing, no?

Yep, you're right! It's just my mistake :-P

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/724733002/diff/200001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestDictionary.cpp:28: 
On 2014/11/20 02:18:02, bashi1 wrote:
> Remove unnecessary blank line.

Done.

[haraken, 2014-11-20 13:05:40]

Sorry about the review delay. LGTM.

[zino, 2014-11-20 14:06:36]

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

[commit-bot: I haz the power, 2014-11-20 14:07:08]

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

[commit-bot: I haz the power, 2014-11-20 14:14:49]

Committed patchset #6 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185667