Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
124763002:
Support IDL attributes limited to only known values. (Closed)
|
Created:
6 years, 11 months ago by sof Modified:
6 years, 10 months ago CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, adamk+blink_chromium.org, Nate Chapin, watchdog-blink-watchlist_google.com, Inactive Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSupport IDL attributes limited to only known values.
Provide the required binding layer support for IDL attributes that
reflect an enumerated content attribute that is limited to a set of
known values:
http://www.whatwg.org/specs/web-apps/current-work/#limited-to-only-known-values
That is, have the getter for such attributes check with respect to the
known value set and perform the required filtering + canonicalization
of the value returned.
R=haraken
BUG=331695
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164509
Patch Set 1 #
Total comments: 11
Patch Set 2 : Retire same-case optimization #Patch Set 3 : Rebased + switched to using '|' as ReflectOnly separator #
Total comments: 5
Patch Set 4 : Flatten generated code block #
Messages
Total messages: 28 (0 generated)
Please take a look when you next have a moment. If/when this change is accepted, follow-up work would be: - document the IDL attribute addition - make use of this mechanism to fix handling of various reflecting IDL attributes (e.g., crbug.com/321862 )
On 2014/01/05 22:40:31, sof wrote: > Please take a look when you next have a moment. > > If/when this change is accepted, follow-up work would be: > > - document the IDL attribute addition > - make use of this mechanism to fix handling of various reflecting IDL > attributes (e.g., crbug.com/321862 ) - Where is [ReflectOnly] going to be used specifically? If it's used in performance-sensitive DOM attributes, it might regress performance. - Probably we might want to use [Reflect=*] instead of introducing [ReflectOnly]. --- [Reflect=xxx] => [Reflect, ImplementedAs=xxx] --- [Reflect=xxx, ReflectOnly=yyy] => [Reflect=yyy, ImplementedAs=xxx]
On 2014/01/06 00:24:10, haraken wrote: > On 2014/01/05 22:40:31, sof wrote: > > Please take a look when you next have a moment. > > > > If/when this change is accepted, follow-up work would be: > > > > - document the IDL attribute addition > > - make use of this mechanism to fix handling of various reflecting IDL > > attributes (e.g., crbug.com/321862 ) > > - Where is [ReflectOnly] going to be used specifically? If it's used in > performance-sensitive DOM attributes, it might regress performance. > Searching for "limited to only known values" in the spec yields 22 hits; one of them being the defn & some attributes are grouped together, so about 25 attributes overal. None of these are mainstream, daily-use kind of properties, I'd argue. > - Probably we might want to use [Reflect=*] instead of introducing > [ReflectOnly]. > > --- [Reflect=xxx] => [Reflect, ImplementedAs=xxx] > --- [Reflect=xxx, ReflectOnly=yyy] => [Reflect=yyy, ImplementedAs=xxx] Not very clear that xxx is an attribute.
On 2014/01/06 06:16:28, sof wrote: > On 2014/01/06 00:24:10, haraken wrote: > > On 2014/01/05 22:40:31, sof wrote: > > > Please take a look when you next have a moment. > > > > > > If/when this change is accepted, follow-up work would be: > > > > > > - document the IDL attribute addition > > > - make use of this mechanism to fix handling of various reflecting IDL > > > attributes (e.g., crbug.com/321862 ) > > > > - Where is [ReflectOnly] going to be used specifically? If it's used in > > performance-sensitive DOM attributes, it might regress performance. > > > > Searching for "limited to only known values" in the spec yields 22 hits; one of > them being the defn & some attributes are grouped together, so about 25 > attributes overal. None of these are mainstream, daily-use kind of properties, > I'd argue. > > > - Probably we might want to use [Reflect=*] instead of introducing > > [ReflectOnly]. > > > > --- [Reflect=xxx] => [Reflect, ImplementedAs=xxx] > > --- [Reflect=xxx, ReflectOnly=yyy] => [Reflect=yyy, ImplementedAs=xxx] > > Not very clear that xxx is an attribute. Thanks, makes sense.
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestObject.idl:78: [Reflect, ReflectOnly=Per Paal Espen] attribute DOMString limitedToOnlyAttribute; Shall we use ReflectOnly=Per|Paal|Pspen instead of space splitting? https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; It looks strange that you're checking if(resultValue == "unique") first and then if(equalIgnoringCase(resultValue, "unique")).
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestObject.idl:78: [Reflect, ReflectOnly=Per Paal Espen] attribute DOMString limitedToOnlyAttribute; On 2014/01/06 06:23:53, haraken wrote: > > Shall we use ReflectOnly=Per|Paal|Pspen instead of space splitting? I thought I could from reading the IDLExtendedAttributes.txt description for "Attr=*", but "|" with more than 2 values isn't allowed by the parser. It looks odd to use space, I agree, should we make the parser more lenient? (assuming that there isn't a way to express this already.) https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; On 2014/01/06 06:23:53, haraken wrote: > > It looks strange that you're checking if(resultValue == "unique") first and then > if(equalIgnoringCase(resultValue, "unique")). The reason for it is that most attribute values do preserve the expected case, so only fall into the case of checking & normalizing the value reported if it doesn't. Perhaps that's an optimization not worth bothering with?
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestObject.idl:78: [Reflect, ReflectOnly=Per Paal Espen] attribute DOMString limitedToOnlyAttribute; On 2014/01/06 06:40:51, sof wrote: > On 2014/01/06 06:23:53, haraken wrote: > > > > Shall we use ReflectOnly=Per|Paal|Pspen instead of space splitting? > > I thought I could from reading the IDLExtendedAttributes.txt description for > "Attr=*", but "|" with more than 2 values isn't allowed by the parser. ?? To agree with haraken, we can't use spaces here. | should work with arbitrarily many values. Let me check this.
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; On 2014/01/06 06:40:51, sof wrote: > On 2014/01/06 06:23:53, haraken wrote: > > > > It looks strange that you're checking if(resultValue == "unique") first and > then > > if(equalIgnoringCase(resultValue, "unique")). > > The reason for it is that most attribute values do preserve the expected case, > so only fall into the case of checking & normalizing the value reported if it > doesn't. Perhaps that's an optimization not worth bothering with? ?? Do you want to accept "Unique"? - If you want to accept "Unique", you shouldn't check if(resultValue == "unique") first. - If you don't want to accept "Unique", you don't need to check if(equalIgnoringCase(resultValue, "unique")).
Parser bug is real, fix posted. https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestObject.idl:78: [Reflect, ReflectOnly=Per Paal Espen] attribute DOMString limitedToOnlyAttribute; Thanks for catching this; this is a bug, which I've posted a fix for: Perl IDL parser: allow extended attributes to have more than 2 values https://codereview.chromium.org/124943002/ In the initial Python rewrite the grammar explicitly limited extended attribute value lists to length 2, as that was sufficient for actual use, but I removed the limit later (someone complained, and it's clearer that way anyway). I didn't update the Perl then though b/c no-one was using it yet.
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; On 2014/01/06 07:00:37, haraken wrote: > On 2014/01/06 06:40:51, sof wrote: > > On 2014/01/06 06:23:53, haraken wrote: > > > > > > It looks strange that you're checking if(resultValue == "unique") first and > > then > > > if(equalIgnoringCase(resultValue, "unique")). > > > > The reason for it is that most attribute values do preserve the expected case, > > so only fall into the case of checking & normalizing the value reported if it > > doesn't. Perhaps that's an optimization not worth bothering with? > > ?? > Do you want to accept "Unique"? > The attribute might be set to "Unique", but the canonical representation is "unique". Attribute values are case insensitive. > - If you want to accept "Unique", you shouldn't check if(resultValue == > "unique") first. > If 99/100 the user doesn't bother with case insensitivity and supplies "unique", why so? > - If you don't want to accept "Unique", you don't need to check > if(equalIgnoringCase(resultValue, "unique")). See above.
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; ah, sorry, understood. However, I think this looks like a premature optimization to me. We might want to remove the optimization and see how the perf bots look. If we do need the optimization, we should improve equalIgnoringCase() instead of adding optimization to the caller side.
Will wait until the IDL parser fix lands & rebase + switch to using "|" as separator then. https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... File Source/bindings/tests/results/V8TestObject.cpp (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/result... Source/bindings/tests/results/V8TestObject.cpp:883: resultValue = "unique"; On 2014/01/06 08:07:33, haraken wrote: > > ah, sorry, understood. > > However, I think this looks like a premature optimization to me. We might want > to remove the optimization and see how the perf bots look. > > If we do need the optimization, we should improve equalIgnoringCase() instead of > adding optimization to the caller side. Done; the other half of this is that the string value isn't reused by not doing so.
https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... File Source/bindings/tests/idls/TestObject.idl (right): https://codereview.chromium.org/124763002/diff/1/Source/bindings/tests/idls/T... Source/bindings/tests/idls/TestObject.idl:78: [Reflect, ReflectOnly=Per Paal Espen] attribute DOMString limitedToOnlyAttribute; On 2014/01/06 07:12:49, Nils Barth wrote: > Thanks for catching this; this is a bug, which I've posted a fix for: > Perl IDL parser: allow extended attributes to have more than 2 values > https://codereview.chromium.org/124943002/ > > In the initial Python rewrite the grammar explicitly limited extended attribute > value lists to length 2, as that was sufficient for actual use, but I removed > the limit later (someone complained, and it's clearer that way anyway). I didn't > update the Perl then though b/c no-one was using it yet. Thanks; rebased this CL on top of it + switched to using "|".
LGTM https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... File Source/bindings/scripts/code_generator_v8.pm (right): https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... Source/bindings/scripts/code_generator_v8.pm:6270: AddToImplIncludes("wtf/text/WTFString.h"); This won't be needed. https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... Source/bindings/scripts/code_generator_v8.pm:6283: ${indent}if (!resultValue.isEmpty()) { This empty check won't be needed.
https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... File Source/bindings/scripts/code_generator_v8.pm (right): https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... Source/bindings/scripts/code_generator_v8.pm:6283: ${indent}if (!resultValue.isEmpty()) { On 2014/01/06 13:13:02, haraken wrote: > > This empty check won't be needed. It is useful to check for early though (rather than spin through N names & have it bottom out in the unknown value case.)
On 2014/01/06 13:15:56, sof wrote: > https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... > File Source/bindings/scripts/code_generator_v8.pm (right): > > https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... > Source/bindings/scripts/code_generator_v8.pm:6283: ${indent}if > (!resultValue.isEmpty()) { > On 2014/01/06 13:13:02, haraken wrote: > > > > This empty check won't be needed. > > It is useful to check for early though (rather than spin through N names & have > it bottom out in the unknown value case.) Then: if (resultValue.IsEmpty()) { resultValue = ""; } else if (equalIgnoringCase(resultValue, "foo")) { ...; } ? A slight benefit of doing this is that you can remove $prefix from the code generator.
https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... File Source/bindings/scripts/code_generator_v8.pm (right): https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... Source/bindings/scripts/code_generator_v8.pm:6270: AddToImplIncludes("wtf/text/WTFString.h"); On 2014/01/06 13:13:02, haraken wrote: > > This won't be needed. Done. https://codereview.chromium.org/124763002/diff/200001/Source/bindings/scripts... Source/bindings/scripts/code_generator_v8.pm:6283: ${indent}if (!resultValue.isEmpty()) { On 2014/01/06 13:15:56, sof wrote: > On 2014/01/06 13:13:02, haraken wrote: > > > > This empty check won't be needed. > > It is useful to check for early though (rather than spin through N names & have > it bottom out in the unknown value case.) Done.
LGTM, thanks!
On 2014/01/06 13:48:41, haraken wrote: > LGTM, thanks! Thank you. Sorry for the extra bother; I've prepared some documentation for ReflectOnly -- who do I need to ask for permissions to update http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ?
On 2014/01/06 14:05:13, sof wrote: > On 2014/01/06 13:48:41, haraken wrote: > > LGTM, thanks! > > Thank you. > > Sorry for the extra bother; I've prepared some documentation for ReflectOnly -- > who do I need to ask for permissions to update > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? You can edit yourself :)
On 2014/01/06 14:07:06, haraken wrote: > On 2014/01/06 14:05:13, sof wrote: > > On 2014/01/06 13:48:41, haraken wrote: > > > LGTM, thanks! > > > > Thank you. > > > > Sorry for the extra bother; I've prepared some documentation for ReflectOnly > -- > > who do I need to ask for permissions to update > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? > > You can edit yourself :) oh, permission. Aren't you a chromium committer?
On 2014/01/06 14:07:51, haraken wrote: > On 2014/01/06 14:07:06, haraken wrote: > > On 2014/01/06 14:05:13, sof wrote: > > > On 2014/01/06 13:48:41, haraken wrote: > > > > LGTM, thanks! > > > > > > Thank you. > > > > > > Sorry for the extra bother; I've prepared some documentation for ReflectOnly > > -- > > > who do I need to ask for permissions to update > > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? > > > > You can edit yourself :) > > oh, permission. Aren't you a chromium committer? Probably it's better to ask it in IRC.
On 2014/01/06 14:08:45, haraken wrote: > On 2014/01/06 14:07:51, haraken wrote: > > On 2014/01/06 14:07:06, haraken wrote: > > > On 2014/01/06 14:05:13, sof wrote: > > > > On 2014/01/06 13:48:41, haraken wrote: > > > > > LGTM, thanks! > > > > > > > > Thank you. > > > > > > > > Sorry for the extra bother; I've prepared some documentation for > ReflectOnly > > > -- > > > > who do I need to ask for permissions to update > > > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? > > > > > > You can edit yourself :) > > > > oh, permission. Aren't you a chromium committer? > > Probably it's better to ask it in IRC. right, will do. i have committer status.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/124763002/310001
On 2014/01/06 14:11:59, sof wrote: > On 2014/01/06 14:08:45, haraken wrote: > > On 2014/01/06 14:07:51, haraken wrote: > > > On 2014/01/06 14:07:06, haraken wrote: > > > > On 2014/01/06 14:05:13, sof wrote: > > > > > On 2014/01/06 13:48:41, haraken wrote: > > > > > > LGTM, thanks! > > > > > > > > > > Thank you. > > > > > > > > > > Sorry for the extra bother; I've prepared some documentation for > > ReflectOnly > > > > -- > > > > > who do I need to ask for permissions to update > > > > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? > > > > > > > > You can edit yourself :) > > > > > > oh, permission. Aren't you a chromium committer? > > > > Probably it's better to ask it in IRC. > > right, will do. i have committer status. nevermind, a 2nd sign-in attempt just now succeeded & i can edit. yay :)
On 2014/01/06 14:14:52, sof wrote: > On 2014/01/06 14:11:59, sof wrote: > > On 2014/01/06 14:08:45, haraken wrote: > > > On 2014/01/06 14:07:51, haraken wrote: > > > > On 2014/01/06 14:07:06, haraken wrote: > > > > > On 2014/01/06 14:05:13, sof wrote: > > > > > > On 2014/01/06 13:48:41, haraken wrote: > > > > > > > LGTM, thanks! > > > > > > > > > > > > Thank you. > > > > > > > > > > > > Sorry for the extra bother; I've prepared some documentation for > > > ReflectOnly > > > > > -- > > > > > > who do I need to ask for permissions to update > > > > > > http://www.chromium.org/blink/webidl/blink-idl-extended-attributes ? > > > > > > > > > > You can edit yourself :) > > > > > > > > oh, permission. Aren't you a chromium committer? > > > > > > Probably it's better to ask it in IRC. > > > > right, will do. i have committer status. > > nevermind, a 2nd sign-in attempt just now succeeded & i can edit. yay :) http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Reflec...
Message was sent while issue was closed.
Change committed as 164509
Message was sent while issue was closed.
Python side is: IDL compiler: [ReflectOnly] https://codereview.chromium.org/145773004/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
