Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
105273010:
Web Animations API: Start implementation of timing input objects. (Closed)
|
Created:
7 years ago by rjwright Modified:
6 years, 11 months ago CC:
blink-reviews, shans, alancutter (OOO until 2018), Mike Lawther (Google), dstockwell, Timothy Loh, Inactive, darktears, arv+blink, Steve Block, dino_apple.com, watchdog-blink-watchlist_google.com, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionWeb Animations API: Start implementation of timing input objects.
BUG=327564
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165750
Patch Set 1 #Patch Set 2 : Change to use Dictionary instead of Object #
Total comments: 17
Patch Set 3 : Don't fail on invalid timing input #Patch Set 4 : Changed timing input interface to match spec changes. #Patch Set 5 : Change default fill mode to none for all cases #Patch Set 6 : Fix layout test #Patch Set 7 : Change default fill back to "forwards". Move timing input processing into method. #
Total comments: 8
Patch Set 8 : Handle timing input objects with custom getters that throw exceptions. (Ignore change to test for n… #Patch Set 9 : Added test cases for each timing field. #Patch Set 10 : Add custom binding for timing input and handle each case separately. #
Total comments: 10
Patch Set 11 : Update and rebase. Manually merge changes from https://codereview.chromium.org/140623003/ #Patch Set 12 : Change treatment of invalid duration input #
Total comments: 1
Patch Set 13 : Remove custom bindings #Patch Set 14 : Change isnan to std:isnan #
Messages
Total messages: 30 (0 generated)
I'd love some direction on what to do about invalid timing inputs. I'll add some test cases tomorrow. PTAL :)
I've switched to using Dictionary instead of Object for making Timing objects. I don't know if it's actually better.
https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:80: } I think it's simpler to check the bool result of Dictionary.get instead of using hasProperty. Also, I think error handling should probably just be setting the default value back: timing.startDelay = 0; if (timingInputDictionary.get("startDelay", timing.startDelay)) { if (isnan(timing.startDelay) || isinf(timing.startDelay)) timing.startDelay = 0; } If Timing already has default values, then even better would be: double startDelay = 0; if (timingInputDictionary.get("startDelay", startDelay)) { if (!(isnan(startDelay) || isinf(startDelay))) timing.startDelay = startDelay; }
https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:80: } On 2013/12/16 23:47:22, shans wrote: > I think it's simpler to check the bool result of Dictionary.get instead of using > hasProperty. > > Also, I think error handling should probably just be setting the default value > back: > > timing.startDelay = 0; > if (timingInputDictionary.get("startDelay", timing.startDelay)) { > if (isnan(timing.startDelay) || isinf(timing.startDelay)) > timing.startDelay = 0; > } > > If Timing already has default values, then even better would be: > > double startDelay = 0; > if (timingInputDictionary.get("startDelay", startDelay)) { > if (!(isnan(startDelay) || isinf(startDelay))) > timing.startDelay = startDelay; > } Ok,but these two suggestions only work in unison. If we want to catch invalid inputs we need to do 'has' then 'get'. I'm down with your approach.
We should add a comprehensive unit test to cover all the cases of weird input (for example objects with custom getters that throw exceptions). Have a look at LayoutTests/fast/dom/Geolocation/argument-types.html for inspiration. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:72: if (timingInputDictionary.hasProperty("startDelay")) { This has been renamed to 'delay'. We should rename it globally in a later patch. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:76: ASSERT(!isinf(startDelay)); We shouldn't be asserting here - a developer could pass anything as the value of startDelay. If it's an invalid value, we should just ignore it and use the default value of 0. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:95: ASSERT_NOT_REACHED(); Same thing about asserting and ignoring invalid input. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:244: return; Again, I don't think we should fail. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:246: return; This seems wrong. I think that if timingInput is not an object, we should always coerce to a number. That's what JavaScript APIs typically do. For example, you can pass a string to setTimeout and it works: "setTimeout(function() { console.log('foo'); }, '1000')". In the case that the coercion returns NaN (or a negative number), I think we should just use 0 internally. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.h (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:48: static void startAnimation(Element*, Vector<Dictionary> keyframesDictionaryVector, v8::Handle<v8::Value> = v8::Handle<v8::Value>()); You should name this new param https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.idl (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.idl:33: [RuntimeEnabled=WebAnimationsAPI] void animate(sequence<Dictionary> keyframes, any timingInput); Why do we need two overloaded versions of this method? Can't we make timingInput optional?
I've done some of that. Still no tests. I'm wondering if I should just add support for all of the fields before I do comprehensive testing. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:72: if (timingInputDictionary.hasProperty("startDelay")) { On 2013/12/17 00:05:35, Steve Block wrote: > This has been renamed to 'delay'. We should rename it globally in a later patch. Ok. Is it just this one? And is the spec up to date on this front? Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:76: ASSERT(!isinf(startDelay)); On 2013/12/17 00:05:35, Steve Block wrote: > We shouldn't be asserting here - a developer could pass anything as the value of > startDelay. If it's an invalid value, we should just ignore it and use the > default value of 0. Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:80: } On 2013/12/16 23:47:22, shans wrote: > I think it's simpler to check the bool result of Dictionary.get instead of using > hasProperty. > > Also, I think error handling should probably just be setting the default value > back: > > timing.startDelay = 0; > if (timingInputDictionary.get("startDelay", timing.startDelay)) { > if (isnan(timing.startDelay) || isinf(timing.startDelay)) > timing.startDelay = 0; > } > > If Timing already has default values, then even better would be: > > double startDelay = 0; > if (timingInputDictionary.get("startDelay", startDelay)) { > if (!(isnan(startDelay) || isinf(startDelay))) > timing.startDelay = startDelay; > } Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:95: ASSERT_NOT_REACHED(); On 2013/12/17 00:05:35, Steve Block wrote: > Same thing about asserting and ignoring invalid input. Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:244: return; On 2013/12/17 00:05:35, Steve Block wrote: > Again, I don't think we should fail. Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.cpp:246: return; On 2013/12/17 00:05:35, Steve Block wrote: > This seems wrong. I think that if timingInput is not an object, we should always > coerce to a number. That's what JavaScript APIs typically do. For example, you > can pass a string to setTimeout and it works: "setTimeout(function() { > console.log('foo'); }, '1000')". In the case that the coercion returns NaN (or a > negative number), I think we should just use 0 internally. Sounds good, but I have no idea how to do it. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.h (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.h:48: static void startAnimation(Element*, Vector<Dictionary> keyframesDictionaryVector, v8::Handle<v8::Value> = v8::Handle<v8::Value>()); On 2013/12/17 00:05:35, Steve Block wrote: > You should name this new param Done. https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... File Source/core/animation/ElementAnimation.idl (right): https://codereview.chromium.org/105273010/diff/20001/Source/core/animation/El... Source/core/animation/ElementAnimation.idl:33: [RuntimeEnabled=WebAnimationsAPI] void animate(sequence<Dictionary> keyframes, any timingInput); On 2013/12/17 00:05:35, Steve Block wrote: > Why do we need two overloaded versions of this method? Can't we make timingInput > optional? We don't, but Alan said not to make it optional. I don't know why. I tried it as optional and it worked so I'll change it back to that. Done.
On 2013/12/17 00:05:34, Steve Block wrote: > We should add a comprehensive unit test to cover all the cases of weird input > (for example objects with custom getters that throw exceptions). Have a look at > LayoutTests/fast/dom/Geolocation/argument-types.html for inspiration. > I'm not sure about testing timing objects with custom getters that throw exceptions. As far as I can tell the IDL for Dictionary doesn't use JS getters; and after that we work with Dictionaries, not JS. In contrast, the geolocation test expects an exception when the custom getter throws an exception, so whoever wrote the test knew that the getter would be used (I can't figure out where it is used though). Similarly, passing a timing argument with a valueOf implementation that throws an exception doesn't cause this code to throw an exception. Maybe when (if) we write IDL for the Timing class we could hit it (e.g. if the "delay" field expects a double but gets an object), I don't know. Including these cases now seems to test code that doesn't exist yet. Obviously we still need to test all kinds of weird inputs, but I don't think we need to test custom getters or valueOf.
https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:84: if (!isnan(iterationStart) && !isinf(iterationStart)) I'm not sure this is the correct procedure for converting to 'double'. For example, null should be treated as 0, while NaN, Infinity will throw a type error :( I guess the bindings generator will take care of this once it supports dictionaries. See: http://www.w3.org/TR/WebIDL/#es-double https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:93: bool hasIterationDurationValue = timingInputDictionary.get("duration", iterationDurationValue); Did we discover whether this will invoke a custom getter? timing = {get duration() { return 1; }} https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:94: double iterationDuration = 0; just declare this as needed on lines 98 / 103 https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:125: } timing.assertValid();
https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:84: if (!isnan(iterationStart) && !isinf(iterationStart)) On 2014/01/15 11:13:52, dstockwell wrote: > I'm not sure this is the correct procedure for converting to 'double'. For > example, null should be treated as 0, while NaN, Infinity will throw a type > error :( I guess the bindings generator will take care of this once it supports > dictionaries. > > See: http://www.w3.org/TR/WebIDL/#es-double Yeah I wasn't sure what to do about this. https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:93: bool hasIterationDurationValue = timingInputDictionary.get("duration", iterationDurationValue); On 2014/01/15 11:13:52, dstockwell wrote: > Did we discover whether this will invoke a custom getter? > > timing = {get duration() { return 1; }} Yes it does invoke the getter in this case. https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:94: double iterationDuration = 0; On 2014/01/15 11:13:52, dstockwell wrote: > just declare this as needed on lines 98 / 103 Done. https://codereview.chromium.org/105273010/diff/320001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:125: } On 2014/01/15 11:13:52, dstockwell wrote: > timing.assertValid(); Done.
So I've added some unit testing that just tests that negative values, infinity, invalid values, etc. work correctly for each of the Timing fields. Do I need to test funky stuff with objects, custom getters, etc.?
ptal
lgtm, pending a few minor comments https://codereview.chromium.org/105273010/diff/540001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8ElementCustom.cpp (right): https://codereview.chromium.org/105273010/diff/540001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8ElementCustom.cpp:42: if ((info.Length() == 1) && (info[0]->IsArray())) { The inner parentheses are not required here, just: if (info.Length() == 1 && info[0]->IsArray()) { https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:76: // fill mode "none". The default in the spec seems to be "auto" https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:95: v8::Local<v8::Value> iterationDurationValue; We shouldn't be using v8::Local in core/, but there doesn't seem to be a good way to accomplish "all strings are treated as 'auto' except" otherwise. Let's treat NaN as 'auto' for now. So just get(double&) and only set iteration duration if !isnan. // fixme that this needs to be resolved in the spec https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:99: // All strings are treated as 'auto' except strings that are numbers, e.g. '1', 'Infinity'. Looks like it should actually be that numbers less than 0 are also treated as "auto". https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... File Source/core/animation/ElementAnimation.idl (right): https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.idl:32: [Custom, RuntimeEnabled=WebAnimationsAPI] void animate(sequence<Dictionary> keyframes, optional any timingInput); If we put back the two overloads and mark both Custom, does everything still work?
https://codereview.chromium.org/105273010/diff/540001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8ElementCustom.cpp (right): https://codereview.chromium.org/105273010/diff/540001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8ElementCustom.cpp:42: if ((info.Length() == 1) && (info[0]->IsArray())) { On 2014/01/21 20:47:29, dstockwell wrote: > The inner parentheses are not required here, just: > > if (info.Length() == 1 && info[0]->IsArray()) { Done. https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... File Source/core/animation/ElementAnimation.cpp (right): https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:76: // fill mode "none". On 2014/01/21 20:47:29, dstockwell wrote: > The default in the spec seems to be "auto" Done. https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:95: v8::Local<v8::Value> iterationDurationValue; On 2014/01/21 20:47:29, dstockwell wrote: > We shouldn't be using v8::Local in core/, but there doesn't seem to be a good > way to accomplish "all strings are treated as 'auto' except" otherwise. > > Let's treat NaN as 'auto' for now. So just get(double&) and only set iteration > duration if !isnan. > > // fixme that this needs to be resolved in the spec Done. https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.cpp:99: // All strings are treated as 'auto' except strings that are numbers, e.g. '1', 'Infinity'. On 2014/01/21 20:47:29, dstockwell wrote: > Looks like it should actually be that numbers less than 0 are also treated as > "auto". Done. https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... File Source/core/animation/ElementAnimation.idl (right): https://codereview.chromium.org/105273010/diff/540001/Source/core/animation/E... Source/core/animation/ElementAnimation.idl:32: [Custom, RuntimeEnabled=WebAnimationsAPI] void animate(sequence<Dictionary> keyframes, optional any timingInput); On 2014/01/21 20:47:29, dstockwell wrote: > If we put back the two overloads and mark both Custom, does everything still > work? No. It doesn't compile. It seems to ignore the custom tag and generate code for animate.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/105273010/740001
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...
+haraken for bindings owners. ptal :)
https://codereview.chromium.org/105273010/diff/740001/Source/bindings/v8/cust... File Source/bindings/v8/custom/V8ElementCustom.cpp (right): https://codereview.chromium.org/105273010/diff/740001/Source/bindings/v8/cust... Source/bindings/v8/custom/V8ElementCustom.cpp:39: void V8Element::animateMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& info) Are you writing this method in custom bindings just because the overload resolution algorithm is not correctly implemented in the current IDL compiler? What's the difference between the auto-generated code and this custom binding code? I want to understand why you need to write this in custom bindings.
Hey Haraken,
Yes just because the generator doesn't produce correct code yet. There
seems to be two different problems.
- The generated code doesn't branch on the type of the second argument
(even though it produces code for both cases)
- The generated code doesn't handle the 1 argument case where "optional"
is used (even though it checks for info.Length() == 1)
The binding
partial interface Element {
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, optional Dictionary timingInput);
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, double timingInput);
};
Generates code in Debug/gen/blink/bindings/V8Element.cpp
static void animateMethod(const v8::FunctionCallbackInfo<v8::Value>& info)
{
if (((info.Length() == 1) && (info[0]->IsArray())) || ((info.Length()
== 2) && (info[0]->IsArray()))) {
animate1Method(info);
return;
}
if (((info.Length() == 2) && (info[0]->IsArray()))) {
animate2Method(info);
return;
}
// exception code
}
animate1Method makes a Dictionary out of info[1], but doesn't handle the 1
argument case.
animate2Method makes a double out of info[1], but it is never reached.
I tried
partial interface Element {
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, Dictionary timingInput);
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, double timingInput);
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes);
};
Which generates
static void animateMethod(const v8::FunctionCallbackInfo<v8::Value>& info)
{
if (((info.Length() == 2) && (info[0]->IsArray()))) {
animate1Method(info);
return;
}
if (((info.Length() == 2) && (info[0]->IsArray()))) {
animate2Method(info);
return;
}
if (((info.Length() == 1) && (info[0]->IsArray()))) {
animate3Method(info);
return;
}
// exception code
}
Which is still not correct. However in this case animate1Method,
animate2Method and animate3Method seem correct.
The binding
partial interface Element {
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, optional Dictionary timingInput);
};
Generates only
static void animateMethod(const v8::FunctionCallbackInfo<v8::Value>& info)
{
if (UNLIKELY(info.Length() < 1))
// throw and return
Element* imp = V8Element::toNative(info.Holder());
V8TRYCATCH_VOID(Vector<Dictionary>, keyframes,
toNativeArray<Dictionary>(info[0], 1, info.GetIsolate()));
V8TRYCATCH_VOID(Dictionary, timingInput, Dictionary(info[1],
info.GetIsolate()));
if (!timingInput.isUndefinedOrNull() && !timingInput.isObject()) {
// throwTypeError and return
}
v8SetReturnValueFast(info, ElementAnimation::animate(imp, keyframes,
timingInput), imp);
}
Which doesn't handle the 1 argument case.
I hope that isn't too much information.
On Thu, Jan 23, 2014 at 1:37 PM, <haraken@chromium.org> wrote:
>
> https://codereview.chromium.org/105273010/diff/740001/
> Source/bindings/v8/custom/V8ElementCustom.cpp
> File Source/bindings/v8/custom/V8ElementCustom.cpp (right):
>
> https://codereview.chromium.org/105273010/diff/740001/
> Source/bindings/v8/custom/V8ElementCustom.cpp#newcode39
> Source/bindings/v8/custom/V8ElementCustom.cpp:39: void
> V8Element::animateMethodCustom(const
> v8::FunctionCallbackInfo<v8::Value>& info)
>
> Are you writing this method in custom bindings just because the overload
> resolution algorithm is not correctly implemented in the current IDL
> compiler?
>
> What's the difference between the auto-generated code and this custom
> binding code?
>
> I want to understand why you need to write this in custom bindings.
>
> https://codereview.chromium.org/105273010/
>
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.
Thanks Renee!
> I tried
>
> partial interface Element {
> [RuntimeEnabled=WebAnimationsAPI] Animation
> animate(sequence<Dictionary> keyframes, Dictionary timingInput);
> [RuntimeEnabled=WebAnimationsAPI] Animation
> animate(sequence<Dictionary> keyframes, double timingInput);
> [RuntimeEnabled=WebAnimationsAPI] Animation
> animate(sequence<Dictionary> keyframes);
> };
I think the above is a correct IDL. (It's not right to use 'optional' and method
overloading at the same time.)
> Which generates
>
> static void animateMethod(const v8::FunctionCallbackInfo<v8::Value>& info)
> {
> if (((info.Length() == 2) && (info[0]->IsArray()))) {
> animate1Method(info);
> return;
> }
> if (((info.Length() == 2) && (info[0]->IsArray()))) {
> animate2Method(info);
> return;
> }
> if (((info.Length() == 1) && (info[0]->IsArray()))) {
> animate3Method(info);
> return;
> }
> // exception code
> }
>
> Which is still not correct. However in this case animate1Method,
> animate2Method and animate3Method seem correct.
This looks like a bug of the current code generator. Would it be possible to
improve code_generator_v8.pm so that it can generate the correct code?
+nbarth, what would you prefer us to do? Haraken, we can take it on and fix it later, but we're currently pushing to complete the core functionality of our API. Based on our discussion yesterday, it seems like the python IDL compiler will be finished by the time we can fix this problem. On Thu, Jan 23, 2014 at 2:34 PM, <haraken@chromium.org> wrote: > Thanks Renee! > > > I tried >> > > partial interface Element { >> [RuntimeEnabled=WebAnimationsAPI] Animation >> animate(sequence<Dictionary> keyframes, Dictionary timingInput); >> [RuntimeEnabled=WebAnimationsAPI] Animation >> animate(sequence<Dictionary> keyframes, double timingInput); >> [RuntimeEnabled=WebAnimationsAPI] Animation >> animate(sequence<Dictionary> keyframes); >> }; >> > > I think the above is a correct IDL. (It's not right to use 'optional' and > method > overloading at the same time.) > > > Which generates >> > > static void animateMethod(const v8::FunctionCallbackInfo<v8::Value>& >> info) >> { >> if (((info.Length() == 2) && (info[0]->IsArray()))) { >> animate1Method(info); >> return; >> } >> if (((info.Length() == 2) && (info[0]->IsArray()))) { >> animate2Method(info); >> return; >> } >> if (((info.Length() == 1) && (info[0]->IsArray()))) { >> animate3Method(info); >> return; >> } >> // exception code >> } >> > > Which is still not correct. However in this case animate1Method, >> animate2Method and animate3Method seem correct. >> > > This looks like a bug of the current code generator. Would it be possible > to > improve code_generator_v8.pm so that it can generate the correct code? > > > https://codereview.chromium.org/105273010/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Hi Renée,
Cleanest (and easiest) would be to change the Perl CG to handle overloads of
Dictionaries;
it *should* be a one-line fix in sub GenerateParametersCheckExpression.
The issue is that Dictionary is a non-wrapper type, so default overloading check
doesn't work for it.
If there isn't an easy way to check, then we can't handle double/Dictionary
overloads and what you suggest
(custom bindings) seems the only way to go short-term.
haraken:
How do we test if a value is a Dictionary?
(For wrapper types we use V8${type}::hasInstance)
If there's an easy test, we can put in a test for Dictionary!
> It's not right to use 'optional' and method overloading at the same time.
Optional arguments *can* be used with overloading:
http://heycam.github.io/webidl/#dfn-optionality-value
In general it's complicated, I believe the Perl CG has correct
behavior in this case, w/r/t optionality:
it's not the optionality that's the problem, it's that double/Dictionary
aren't distinguished.
Reference:
http://heycam.github.io/webidl/#idl-overloading
> haraken:
> How do we test if a value is a Dictionary?
> (For wrapper types we use V8${type}::hasInstance)
> If there's an easy test, we can put in a test for Dictionary!
v8Value->IsObject() ?
Currently, a dictionary is just a V8 object.
BTW, as a style point:
You can put [RuntimeEnabled] on the partial interface itself,
instead of each individual member.
This is more idiomatic, as it indicates that you're turning the whole partial
interface on or off.
So instead of:
partial interface Element {
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, optional Dictionary timingInput);
[RuntimeEnabled=WebAnimationsAPI] Animation
animate(sequence<Dictionary> keyframes, double timingInput);
};
...you can write:
[
RuntimeEnabled=WebAnimationsAPI,
] partial interface Element {
Animation animate(sequence<Dictionary> keyframes, optional Dictionary
timingInput);
Animation animate(sequence<Dictionary> keyframes, double timingInput);
};
(Style-wise, note the vertical list for interface extended attributes, and
trailing comma.)
Documentation:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Partia...
On 2014/01/23 04:25:04, haraken wrote:
> > haraken:
> > How do we test if a value is a Dictionary?
> > (For wrapper types we use V8${type}::hasInstance)
> > If there's an easy test, we can put in a test for Dictionary!
>
> v8Value->IsObject() ?
>
> Currently, a dictionary is just a V8 object.
Ok, I'll try adding that check in a quick CL!
On 2014/01/23 04:49:32, Nils Barth wrote: > Ok, I'll try adding that check in a quick CL! CL posted at: Add overload support for non-wrapper type vs. primitive type https://codereview.chromium.org/131973012/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/105273010/950001
Retried try job too often on mac_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rjwright@chromium.org/105273010/920002
Message was sent while issue was closed.
Change committed as 165750 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
