Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2354463002:
CSS Properties and Values API: Implement computation / computational independence (Closed)
|
Created:
4 years, 3 months ago by Timothy Loh Modified:
4 years, 3 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS Properties and Values API: Implement computation / computational independence
This patch implements basic computation and computational independence
logic for registered custom properties. For now we just handle variable
references, length types and lists thereof, while images and transform
functions containing lengths will be properly handled later.
'Computationally independent' has recently been renamed from
'computationally idempotent', so the function is updated to match.
https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-computed-values
https://drafts.css-houdini.org/css-properties-values-api/#computationally-independent
BUG=641877
Committed: https://crrev.com/1ed40ebf3aeeb391d0aa2e82c46988ad767c1c35
Cr-Commit-Position: refs/heads/master@{#420328}
Patch Set 1 #Patch Set 2 : moo #Patch Set 3 : (no change) #
Total comments: 16
Patch Set 4 : didnt add tests yet #Patch Set 5 : test #
Total comments: 11
Patch Set 6 : test more #
Total comments: 2
Patch Set 7 : patch for landing #Patch Set 8 : actually fix test :) #Dependent Patchsets: Messages
Total messages: 39 (13 generated)
Description was changed from ========== ---------------------This line is 72 characters long-------------------- CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. Computation: https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... BUG=641877 ========== to ========== ---------------------This line is 72 characters long-------------------- CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ==========
timloh@chromium.org changed reviewers: + alancutter@chromium.org, sashab@chromium.org
Description was changed from ========== ---------------------This line is 72 characters long-------------------- CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ========== to ========== CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ==========
https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/PropertyRegistration.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:22: return !toCSSVariableReferenceValue(value).variableDataValue()->needsVariableResolution(); Can variables not have percentages etc in them? It feels like this should be recursive on the variable's value. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:24: // TODO(timloh): Images and transform-function values can also contain lengths. Are there tests that fail because of this? https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:34: if (!value.isPrimitiveValue()) Just to prevent the kind of mess we currently have in CSSToStyleMap, I'd prefer this written as if (value.isPrimitiveValue()) { ... } return true; https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); Why not use state.style.zoom()?
https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/PropertyRegistration.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:19: static bool computationallyIndependent(const CSSValue& value) What happens to CSS wide keywords? Either check them or assert they're not given as input. Please add tests for them. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:22: return !toCSSVariableReferenceValue(value).variableDataValue()->needsVariableResolution(); It's very weird to be checking needsVariableResolution here. It doesn't make sense for a CSSVariableReferenceValue to not have var() references, we should be using CSSTokenStreamValues instead, please add it as a separate patch and assert in CSSVariableReferenceValue::create() that the CSSVariableData has unresolved references. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:34: if (!value.isPrimitiveValue()) On 2016/09/19 at 23:25:03, sashab wrote: > Just to prevent the kind of mess we currently have in CSSToStyleMap, I'd prefer this written as > > if (value.isPrimitiveValue()) { > ... > } > > return true; I think it's preferred to exit functions early on simple checks to avoid nested code and provide guarantees for the remainder of the function. I can't find references to this though... https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/19 at 23:25:03, sashab wrote: > Why not use state.style.zoom()? Or avoid the copy and just use style.effectiveZoom() when creating the CSSPrimitiveValue below.
https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1120: } Do we have tests for this code?
Didn't add tests for computation yet, will do that shortly. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/PropertyRegistration.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:19: static bool computationallyIndependent(const CSSValue& value) On 2016/09/20 00:15:55, alancutter wrote: > What happens to CSS wide keywords? Either check them or assert they're not given > as input. > Please add tests for them. This is tested in register-property-syntax-parsing.html already. I'll add an assert. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:22: return !toCSSVariableReferenceValue(value).variableDataValue()->needsVariableResolution(); On 2016/09/19 23:25:03, sashab wrote: > Can variables not have percentages etc in them? It feels like this should be > recursive on the variable's value. If there are variable references then this returns false, if there aren't (i.e. it's just a regular token stream) then we don't do any computation on it regardless of the contents. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:22: return !toCSSVariableReferenceValue(value).variableDataValue()->needsVariableResolution(); On 2016/09/20 00:15:55, alancutter wrote: > It's very weird to be checking needsVariableResolution here. It doesn't make > sense for a CSSVariableReferenceValue to not have var() references, we should be > using CSSTokenStreamValues instead, please add it as a separate patch and assert > in CSSVariableReferenceValue::create() that the CSSVariableData has unresolved > references. There's no CSSTokenStreamValue class -- CSSVariableReferenceValue is the only CSSValue subclass which stores a token stream. I can rename this later if you want, I don't think it makes sense to have two CSSValue subclasses that are effectively identical. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:24: // TODO(timloh): Images and transform-function values can also contain lengths. On 2016/09/19 23:25:03, sashab wrote: > Are there tests that fail because of this? Nope I'll add them when I resolve the TODO. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/PropertyRegistration.cpp:34: if (!value.isPrimitiveValue()) On 2016/09/19 23:25:03, sashab wrote: > Just to prevent the kind of mess we currently have in CSSToStyleMap, I'd prefer > this written as > > if (value.isPrimitiveValue()) { > ... > } > > return true; I'll change this, it probably made more sense before I added the other cases above. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/20 00:15:55, alancutter wrote: > On 2016/09/19 at 23:25:03, sashab wrote: > > Why not use state.style.zoom()? > > Or avoid the copy and just use style.effectiveZoom() when creating the > CSSPrimitiveValue below. I figured it'd be nicer to not have any chance of values changing slightly due to rounding at different zoom levels. I'm not sure if it really makes a difference, lmk what you think. https://codereview.chromium.org/2354463002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1120: } On 2016/09/20 00:39:44, alancutter wrote: > Do we have tests for this code? Uhh... I'll go add some :)
Computation test added. For now ems are incorrectly resolved based on the parent's font-size, will be fixed later...
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); I think we should really use the actual zoom here. If a user zooms in the page their custom properties should change values, otherwise we'll get inconsistencies and bugs. Alternatively you could add a comment like: // To not have any chance of values changing slightly due to rounding at different zoom levels, ignore the zoom here. But that just sounds like it's incorrect behavior, doesn't it?
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/22 00:29:22, sashab wrote: > I think we should really use the actual zoom here. If a user zooms in the page > their custom properties should change values, otherwise we'll get > inconsistencies and bugs. > > Alternatively you could add a comment like: > // To not have any chance of values changing slightly due to rounding at > different zoom levels, ignore the zoom here. > But that just sounds like it's incorrect behavior, doesn't it? I'm not sure what you're getting at here. If I specify left: 10px, I don't expect it to end up as 20px. Similarly, if I specify --left: 10px, I also don't expect it to end up as 20px.
On 2016/09/22 at 02:42:48, timloh wrote: > https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): > > https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); > On 2016/09/22 00:29:22, sashab wrote: > > I think we should really use the actual zoom here. If a user zooms in the page > > their custom properties should change values, otherwise we'll get > > inconsistencies and bugs. > > > > Alternatively you could add a comment like: > > // To not have any chance of values changing slightly due to rounding at > > different zoom levels, ignore the zoom here. > > But that just sounds like it's incorrect behavior, doesn't it? > > I'm not sure what you're getting at here. If I specify left: 10px, I don't expect it to end up as 20px. Similarly, if I specify --left: 10px, I also don't expect it to end up as 20px. But if you specify --my-font-size: 10px, you want it to be 20px. Do we apply zoom to font-size after the variable is substituted?
What does --x compute to in font-size: 10px; --length: 10em; --x: var(--length); ? Do we have tests for this? Is it clear in the specs what should happen here? https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> Rename as outerDiv and innerDiv and use the ids in the test names.
On 2016/09/22 02:44:30, sashab wrote: > On 2016/09/22 at 02:42:48, timloh wrote: > > > https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp > (right): > > > > > https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: > Length length = > primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); > > On 2016/09/22 00:29:22, sashab wrote: > > > I think we should really use the actual zoom here. If a user zooms in the > page > > > their custom properties should change values, otherwise we'll get > > > inconsistencies and bugs. > > > > > > Alternatively you could add a comment like: > > > // To not have any chance of values changing slightly due to rounding at > > > different zoom levels, ignore the zoom here. > > > But that just sounds like it's incorrect behavior, doesn't it? > > > > I'm not sure what you're getting at here. If I specify left: 10px, I don't > expect it to end up as 20px. Similarly, if I specify --left: 10px, I also don't > expect it to end up as 20px. > > But if you specify --my-font-size: 10px, you want it to be 20px. Do we apply > zoom to font-size after the variable is substituted? (please respond to inline comments using the tool) No, if I write 10px it should stay at 10px. Font size is no different to any other length property. If you specify font-size: 10px it always comes out of getComputedStyle as 10px. The spec even has the following example: "For example, 5px is computationally independent, as converting it into a computed value doesn’t change it at all."
On 2016/09/22 03:08:43, alancutter wrote: > What does --x compute to in > font-size: 10px; > --length: 10em; > --x: var(--length); > ? > > Do we have tests for this? Is it clear in the specs what should happen here? Added a test. I think --x should be 100px if it's registered as a length and 10em if it's not registered.
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> On 2016/09/22 03:08:43, alancutter wrote: > Rename as outerDiv and innerDiv and use the ids in the test names. Done (left the other as div1, feels a bit weird..)
LGTM once you add the comment explaining why zoom is 1 :) https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/22 at 02:42:48, Timothy Loh wrote: > On 2016/09/22 00:29:22, sashab wrote: > > I think we should really use the actual zoom here. If a user zooms in the page > > their custom properties should change values, otherwise we'll get > > inconsistencies and bugs. > > > > Alternatively you could add a comment like: > > // To not have any chance of values changing slightly due to rounding at > > different zoom levels, ignore the zoom here. > > But that just sounds like it's incorrect behavior, doesn't it? > > I'm not sure what you're getting at here. If I specify left: 10px, I don't expect it to end up as 20px. Similarly, if I specify --left: 10px, I also don't expect it to end up as 20px. Ok, add this comment then :)
lgtm after comments resolved. https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> On 2016/09/22 at 06:00:46, Timothy Loh wrote: > On 2016/09/22 03:08:43, alancutter wrote: > > Rename as outerDiv and innerDiv and use the ids in the test names. > > Done (left the other as div1, feels a bit weird..) My concern was the test names weren't descriptive enough and should state which element they're testing. I meant for div1 and div3 to be renamed. Feel free to pick better names than outerDiv and innerDiv. Preferably such that you don't need the comment below. https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/22 at 06:02:35, sashab wrote: > Ok, add this comment then :) Agreed, at the very least add the comment. I think any rounding errors here would be too negligible to worry about though. https://codereview.chromium.org/2354463002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:48: var computedStyle = getComputedStyle([div1, innerDiv][i]); This could be "for (var element of [outerDiv, innerDiv])".
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> On 2016/09/22 07:58:41, alancutter wrote: > On 2016/09/22 at 06:00:46, Timothy Loh wrote: > > On 2016/09/22 03:08:43, alancutter wrote: > > > Rename as outerDiv and innerDiv and use the ids in the test names. > > > > Done (left the other as div1, feels a bit weird..) > > My concern was the test names weren't descriptive enough and should state which > element they're testing. I meant for div1 and div3 to be renamed. Feel free to > pick better names than outerDiv and innerDiv. Preferably such that you don't > need the comment below. Picked better names... https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:1117: Length length = primitiveValue.convertToLength(state.cssToLengthConversionData().copyWithAdjustedZoom(1)); On 2016/09/22 07:58:41, alancutter wrote: > On 2016/09/22 at 06:02:35, sashab wrote: > > Ok, add this comment then :) > > Agreed, at the very least add the comment. > I think any rounding errors here would be too negligible to worry about though. Comment added. Probably negligible, but I prefer it this way. Since typed om will return lengths as floats and not 6sf strings, maybe it matters more... I dunno :/ https://codereview.chromium.org/2354463002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:48: var computedStyle = getComputedStyle([div1, innerDiv][i]); On 2016/09/22 07:58:41, alancutter wrote: > This could be "for (var element of [outerDiv, innerDiv])". Done.
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2354463002/#ps120001 (title: "patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> On 2016/09/22 at 08:18:58, Timothy Loh wrote: > On 2016/09/22 07:58:41, alancutter wrote: > > On 2016/09/22 at 06:00:46, Timothy Loh wrote: > > > On 2016/09/22 03:08:43, alancutter wrote: > > > > Rename as outerDiv and innerDiv and use the ids in the test names. > > > > > > Done (left the other as div1, feels a bit weird..) > > > > My concern was the test names weren't descriptive enough and should state which > > element they're testing. I meant for div1 and div3 to be renamed. Feel free to > > pick better names than outerDiv and innerDiv. Preferably such that you don't > > need the comment below. > > Picked better names... Tests still don't mention the element ID being tested. D:
The CQ bit was checked by timloh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, alancutter@chromium.org Link to the patchset: https://codereview.chromium.org/2354463002/#ps140001 (title: "actually fix test :)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html (right): https://codereview.chromium.org/2354463002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-properties/registered-property-computation.html:27: <div id=div3></div> On 2016/09/22 08:25:56, alancutter wrote: > On 2016/09/22 at 08:18:58, Timothy Loh wrote: > > On 2016/09/22 07:58:41, alancutter wrote: > > > On 2016/09/22 at 06:00:46, Timothy Loh wrote: > > > > On 2016/09/22 03:08:43, alancutter wrote: > > > > > Rename as outerDiv and innerDiv and use the ids in the test names. > > > > > > > > Done (left the other as div1, feels a bit weird..) > > > > > > My concern was the test names weren't descriptive enough and should state > which > > > element they're testing. I meant for div1 and div3 to be renamed. Feel free > to > > > pick better names than outerDiv and innerDiv. Preferably such that you don't > > > need the comment below. > > > > Picked better names... > > Tests still don't mention the element ID being tested. D: I fixed it now D:
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by timloh@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by timloh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ========== to ========== CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 ========== to ========== CSS Properties and Values API: Implement computation / computational independence This patch implements basic computation and computational independence logic for registered custom properties. For now we just handle variable references, length types and lists thereof, while images and transform functions containing lengths will be properly handled later. 'Computationally independent' has recently been renamed from 'computationally idempotent', so the function is updated to match. https://drafts.css-houdini.org/css-properties-values-api/#calculation-of-comp... https://drafts.css-houdini.org/css-properties-values-api/#computationally-ind... BUG=641877 Committed: https://crrev.com/1ed40ebf3aeeb391d0aa2e82c46988ad767c1c35 Cr-Commit-Position: refs/heads/master@{#420328} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1ed40ebf3aeeb391d0aa2e82c46988ad767c1c35 Cr-Commit-Position: refs/heads/master@{#420328} |
