Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121913002:
[Typed OM] Add CSSValue -> CSSNumberValue conversion (Closed)
|
Created:
4 years, 5 months ago by meade_UTC10 Modified:
3 years, 9 months ago Reviewers:
sashab CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed OM] Add CSSValue -> CSSNumberValue conversion
Also add support for setting/getting CSSNumberValues in
animation-iteration-count and opacity to allow testing in LayoutTests.
BUG=545318
Patch Set 1 #
Total comments: 4
Patch Set 2 : Change iteration type #
Total comments: 9
Patch Set 3 : Update TODO comments #Patch Set 4 : Add new consts to keep up with code changes. #Patch Set 5 : Also add support for opacity #Patch Set 6 : Make InlineStylePropertyMap error if wrong number #Patch Set 7 : Fix test #Patch Set 8 : sync #Patch Set 9 : sync #Messages
Total messages: 39 (22 generated)
meade@chromium.org changed reviewers: + timloh@chromium.org
On 2016/07/05 05:25:17, Eddy wrote: bug= link is wrong
How do we avoid setting negative numbers for properties that don't support it (and also non-integers for integer-only properties)? https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:47: for (const Member<CSSStyleValue> value : styleValueVector) { CSSStyleValue*? https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:63: for (const Member<const CSSValue> innerValue : cssValueList) { for (const CSSValue* innerValue : It's weird to have a Member local variable here (I guess you meant const Member<>&, but I think in this case it's nicer to just have a CSSValue*)
https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:47: for (const Member<CSSStyleValue> value : styleValueVector) { On 2016/07/05 05:50:03, Timothy Loh wrote: > CSSStyleValue*? Done. https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2121913002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:63: for (const Member<const CSSValue> innerValue : cssValueList) { On 2016/07/05 05:50:03, Timothy Loh wrote: > for (const CSSValue* innerValue : > > It's weird to have a Member local variable here (I guess you meant const > Member<>&, but I think in this case it's nicer to just have a CSSValue*) Agreed, done.
meade@chromium.org changed reviewers: + sashab@chromium.org
Hey Sasha! Want to have a look at this one? After I can send it to Elliott maybe...
https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt:4: FAIL CSSNumberValues in a list round trip when passed in as a list of CSSNumberValues assert_equals: expected "1, 5.5" but got "1 5.5" ??? Is this correct? https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:123: // TODO(meade): Figure out the correct separator. There's a lot of these TODOs to figure out the correct behavior... Should you have more ASSERTs or should we be erroring out in these cases? Is there an alternative way we could implement this so more info is passed through?
https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt:4: FAIL CSSNumberValues in a list round trip when passed in as a list of CSSNumberValues assert_equals: expected "1, 5.5" but got "1 5.5" On 2016/07/11 23:47:22, sashab wrote: > ??? Is this correct? It's a consequence of the TODO you commented on. I figured having an expectation is better than not writing a test at all... to be fixed in a future CL! https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:123: // TODO(meade): Figure out the correct separator. On 2016/07/11 23:47:22, sashab wrote: > There's a lot of these TODOs to figure out the correct behavior... Should you > have more ASSERTs or should we be erroring out in these cases? Is there an > alternative way we could implement this so more info is passed through? So these are interesting - we don't check the separator when this is used internally for calculating styles (so there is no visual effect here), but the generated text is wrong. We definitely shouldn't ASSERT or error out here, it's just not-quite-right yet. I have a plan to fix this, but it involves changing CSSProperties.in to store which separator is used. It's a decent sized change. Though, you might find that change useful for your stuff eventually too :)
https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt:4: FAIL CSSNumberValues in a list round trip when passed in as a list of CSSNumberValues assert_equals: expected "1, 5.5" but got "1 5.5" On 2016/07/27 at 07:14:29, Eddy wrote: > On 2016/07/11 23:47:22, sashab wrote: > > ??? Is this correct? > > It's a consequence of the TODO you commented on. I figured having an expectation is better than not writing a test at all... to be fixed in a future CL! That's a great idea! :D Except I think this should be using TestHarness.js. I think the best way to do this would be: 1. Split out numbers.html into its own test 2. Make a new test numbers-non-space-separator.html which fails 3. Add a failing test expectation to TestExpectations with a bug and a comment like "Expected to fail until CSSOM types know the correct separator for list properties." https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:123: // TODO(meade): Figure out the correct separator. On 2016/07/27 at 07:14:29, Eddy wrote: > On 2016/07/11 23:47:22, sashab wrote: > > There's a lot of these TODOs to figure out the correct behavior... Should you > > have more ASSERTs or should we be erroring out in these cases? Is there an > > alternative way we could implement this so more info is passed through? > > So these are interesting - we don't check the separator when this is used internally for calculating styles (so there is no visual effect here), but the generated text is wrong. We definitely shouldn't ASSERT or error out here, it's just not-quite-right yet. > I have a plan to fix this, but it involves changing CSSProperties.in to store which separator is used. It's a decent sized change. Though, you might find that change useful for your stuff eventually too :) Ok, that sounds good, but I would make the TODO more descriptive then, like: // TODO(meade): Store the correct separator for each property in CSSProperties.in and use it to choose the list type.
https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt:4: FAIL CSSNumberValues in a list round trip when passed in as a list of CSSNumberValues assert_equals: expected "1, 5.5" but got "1 5.5" On 2016/07/27 23:23:57, sashab wrote: > On 2016/07/27 at 07:14:29, Eddy wrote: > > On 2016/07/11 23:47:22, sashab wrote: > > > ??? Is this correct? > > > > It's a consequence of the TODO you commented on. I figured having an > expectation is better than not writing a test at all... to be fixed in a future > CL! > > That's a great idea! :D Except I think this should be using TestHarness.js. > > I think the best way to do this would be: > 1. Split out numbers.html into its own test > 2. Make a new test numbers-non-space-separator.html which fails > 3. Add a failing test expectation to TestExpectations with a bug and a comment > like "Expected to fail until CSSOM types know the correct separator for list > properties." Please don't add new entries to TestExpectations. This sort of expected.txt file is fine.
https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/numbers-expected.txt:4: FAIL CSSNumberValues in a list round trip when passed in as a list of CSSNumberValues assert_equals: expected "1, 5.5" but got "1 5.5" On 2016/07/28 02:11:31, Timothy Loh wrote: > On 2016/07/27 23:23:57, sashab wrote: > > On 2016/07/27 at 07:14:29, Eddy wrote: > > > On 2016/07/11 23:47:22, sashab wrote: > > > > ??? Is this correct? > > > > > > It's a consequence of the TODO you commented on. I figured having an > > expectation is better than not writing a test at all... to be fixed in a > future > > CL! > > > > That's a great idea! :D Except I think this should be using TestHarness.js. > > > > I think the best way to do this would be: > > 1. Split out numbers.html into its own test > > 2. Make a new test numbers-non-space-separator.html which fails > > 3. Add a failing test expectation to TestExpectations with a bug and a comment > > like "Expected to fail until CSSOM types know the correct separator for list > > properties." > > Please don't add new entries to TestExpectations. This sort of expected.txt file > is fine. I don't think it makes sense to split this test up just to merge it again later after making the separator change. https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2121913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:123: // TODO(meade): Figure out the correct separator. On 2016/07/27 23:23:57, sashab wrote: > On 2016/07/27 at 07:14:29, Eddy wrote: > > On 2016/07/11 23:47:22, sashab wrote: > > > There's a lot of these TODOs to figure out the correct behavior... Should > you > > > have more ASSERTs or should we be erroring out in these cases? Is there an > > > alternative way we could implement this so more info is passed through? > > > > So these are interesting - we don't check the separator when this is used > internally for calculating styles (so there is no visual effect here), but the > generated text is wrong. We definitely shouldn't ASSERT or error out here, it's > just not-quite-right yet. > > I have a plan to fix this, but it involves changing CSSProperties.in to store > which separator is used. It's a decent sized change. Though, you might find that > change useful for your stuff eventually too :) > > Ok, that sounds good, but I would make the TODO more descriptive then, like: > // TODO(meade): Store the correct separator for each property in > CSSProperties.in and use it to choose the list type. Done.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
LGTM, but looks like you need to fix some const-ness before landing :) I think styleValueToCSSValue should return a const CSSValue*? Or maybe (ideally) a const CSSValue&.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/04 03:15:25, sashab wrote: > LGTM, but looks like you need to fix some const-ness before landing :) I think > styleValueToCSSValue should return a const CSSValue*? Or maybe (ideally) a const > CSSValue&. Done. Ping Tim? Thanks!
On 2016/08/09 01:16:10, Eddy wrote: > On 2016/08/04 03:15:25, sashab wrote: > > LGTM, but looks like you need to fix some const-ness before landing :) I think > > styleValueToCSSValue should return a const CSSValue*? Or maybe (ideally) a > const > > CSSValue&. > > Done. Ping Tim? Thanks! I think you missed my previous question: How do we avoid setting negative numbers for properties that don't support it (and also non-integers for integer-only properties)?
On 2016/08/09 01:50:09, Timothy Loh wrote: > On 2016/08/09 01:16:10, Eddy wrote: > > On 2016/08/04 03:15:25, sashab wrote: > > > LGTM, but looks like you need to fix some const-ness before landing :) I > think > > > styleValueToCSSValue should return a const CSSValue*? Or maybe (ideally) a > > const > > > CSSValue&. > > > > Done. Ping Tim? Thanks! > > I think you missed my previous question: > > How do we avoid setting negative numbers for properties that don't support it > (and also non-integers for integer-only properties)? We don't. The InlineStylePropertyMap accepts whatever number, but the result will be clamped when being returned from the ComputedStylePropertyMap. I've added support for opacity + a test to show that it works. file:///usr/local/google/src/css-houdini-drafts/css-typed-om/Overview.html#cssnumbervalue in_reply_to: 5704837555552256 send_mail: 1 subject: [Typed OM] Add CSSValue -> CSSNumberValue conversion
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/17 03:34:03, Eddy wrote: > We don't. The InlineStylePropertyMap accepts whatever number, but the result > will be clamped when being returned from the ComputedStylePropertyMap. I've > added support for opacity + a test to show that it works. I'm not convinced. I think opacity here is a special case -- the spec allows any <number> but has clamping at computation time, but this is not the case for other properties. For animation-iteration-count, e.g., the spec says negative numbers are invalid (i.e. rejected at parse time). It looks like if you tried to set a negative number with this code then it won't be clamped and Timing::assertValid will fail.
meade@chromium.org changed reviewers: + rjwright@chromium.org, shans@chromium.org
I've made it so that in that situation an error is thrown. Shane: In the situation an invalid number is passed in, what should happen? In the spec we say that all numbers should be accepted when passed into the InlineStyleMap, (https://drafts.css-houdini.org/css-typed-om/#numbervalue-objects) but that can't be represented without doing one of two weird things: - making the regular inline style also do this (large, probably breaking changes elsewhere in blink) - caching the passed in StyleValue in the inline StylePropertyMap (returned value is correct when using StylePropertyMap, but not reflected in old OM)
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Typed OM] Add CSSValue -> CSSNumberValue conversion Also add support for setting/getting CSSNumberValues in animation-iteration-count to allow testing in LayoutTests. BUG=619010 ========== to ========== [Typed OM] Add CSSValue -> CSSNumberValue conversion Also add support for setting/getting CSSNumberValues in animation-iteration-count to allow testing in LayoutTests. BUG=545318 ==========
Description was changed from ========== [Typed OM] Add CSSValue -> CSSNumberValue conversion Also add support for setting/getting CSSNumberValues in animation-iteration-count to allow testing in LayoutTests. BUG=545318 ========== to ========== [Typed OM] Add CSSValue -> CSSNumberValue conversion Also add support for setting/getting CSSNumberValues in animation-iteration-count and opacity to allow testing in LayoutTests. BUG=545318 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/18 03:24:27, Timothy Loh wrote: > On 2016/08/17 03:34:03, Eddy wrote: > > We don't. The InlineStylePropertyMap accepts whatever number, but the result > > will be clamped when being returned from the ComputedStylePropertyMap. I've > > added support for opacity + a test to show that it works. > > I'm not convinced. I think opacity here is a special case -- the spec allows any > <number> but has clamping at computation time, but this is not the case for > other properties. For animation-iteration-count, e.g., the spec says negative > numbers are invalid (i.e. rejected at parse time). It looks like if you tried to > set a negative number with this code then it won't be clamped and > Timing::assertValid will fail. I also realised that this CL has really gone into the weeds - my original intention was to implement getting, not setting. I've created a new CL where I've removed all the setting logic in https://codereview.chromium.org/2274463002/. Let's do that one first.
meade@chromium.org changed reviewers: - rjwright@chromium.org, sashab@chromium.org, shans@chromium.org, timloh@chromium.org |
