Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(290)

Issue 216803002: Implement all shorthand property. (Closed)

Created:
6 years, 9 months ago by tasak
Modified:
6 years, 1 month ago
CC:
apavlov+blink_chromium.org, blink-reviews, dglazkov+blink, ed+blinkwatch_opera.com, Nils Barth (inactive), rwlbuis, rune+blink
Visibility:
Public.

Description

Implement all shorthand property. Spec: http://dev.w3.org/csswg/css-cascade/#all-shorthand BUG=172051 TEST=fast/css/all-shorthand-css-text.html,fast/css/all-shorthand.html,fast/css/getComputedStyle/getComputedStyle-all.html

Patch Set 1 : #

Patch Set 2 : Updated css-properties-as-js-properties-expected.txt #

Total comments: 9

Patch Set 3 : Added a new build script to generate "all" #

Patch Set 4 : Fixed parseAnimationProperty #

Total comments: 28

Patch Set 5 : Revised #

Patch Set 6 : Fixed transitions' regression #

Patch Set 7 : #

Patch Set 8 : StylePropertySerializer only expands "all" if needed #

Patch Set 9 : Added missing getComputedStyle-all.html again #

Patch Set 10 : Fixed ASSERT_NOT_REACH in linux_blink_dbg #

Total comments: 43

Patch Set 11 : Revised #

Patch Set 12 : Rebaselined inspector/console/console-format-style-whitelist #

Patch Set 13 : #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+769 lines, -84 lines) Patch
M LayoutTests/editing/style/push-down-inline-styles-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/editing/style/script-tests/push-down-inline-styles.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -2 lines 0 comments Download
A LayoutTests/fast/css/all-shorthand.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +58 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/all-shorthand-css-text.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +87 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/all-shorthand-css-text-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +53 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/all-shorthand-expected.html View 1 2 3 4 5 6 7 1 chunk +50 lines, -0 lines 1 comment Download
A LayoutTests/fast/css/getComputedStyle/getComputedStyle-all.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +30 lines, -0 lines 1 comment Download
A LayoutTests/fast/css/getComputedStyle/getComputedStyle-all-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -0 lines 0 comments Download
M LayoutTests/inspector/console/console-format-style-whitelist-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/virtual/stable/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/webexposed/css-properties-as-js-properties-expected.txt View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/build/scripts/templates/StylePropertyShorthand.h.tmpl View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/css/CSSComputedStyleDeclaration.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/css/CSSProperties.in View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSProperty.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSProperty.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +15 lines, -0 lines 0 comments Download
M Source/core/css/CSSPropertyNames.in View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSValueKeywords.in View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/css/StylePropertySerializer.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +54 lines, -0 lines 0 comments Download
M Source/core/css/StylePropertySerializer.cpp View 1 2 3 4 5 6 7 8 9 10 22 chunks +267 lines, -56 lines 7 comments Download
M Source/core/css/StylePropertyShorthandCustom.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +16 lines, -0 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +16 lines, -3 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -0 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +79 lines, -18 lines 3 comments Download
M Source/core/frame/UseCounter.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 20 (0 generated)
tasak
Would you review this CL?
6 years, 9 months ago (2014-03-28 11:14:38 UTC) #1
dglazkov
Would love for rune@ to review this. https://codereview.chromium.org/216803002/diff/40001/LayoutTests/fast/css/all-shorthand-css-text-expected.txt File LayoutTests/fast/css/all-shorthand-css-text-expected.txt (right): https://codereview.chromium.org/216803002/diff/40001/LayoutTests/fast/css/all-shorthand-css-text-expected.txt#newcode8 LayoutTests/fast/css/all-shorthand-css-text-expected.txt:8: PASS cssRules[1].cssText ...
6 years, 9 months ago (2014-03-28 16:56:03 UTC) #2
darktears
On 2014/03/28 16:56:03, dglazkov wrote: > Would love for rune@ to review this. > > ...
6 years, 9 months ago (2014-03-28 17:10:21 UTC) #3
rune
I could not find an intent to implement or ship for this in the spreadsheet ...
6 years, 8 months ago (2014-03-30 19:45:38 UTC) #4
rune
It looks like "all" will now be a property name instead of CSSValueAll for "transition-property:all". ...
6 years, 8 months ago (2014-03-30 22:00:24 UTC) #5
tasak
Thank you for reviewing. I missed that I need "intent to implement/ship". I will discuss ...
6 years, 8 months ago (2014-04-01 10:17:49 UTC) #6
tasak
CC +nbarth for build scripts.
6 years, 8 months ago (2014-04-01 10:22:06 UTC) #7
Nils Barth (inactive)
Hi Sakamoto, Some Python, Jinja, and GYP notes on the build scripts, but otherwise looks ...
6 years, 8 months ago (2014-04-03 06:27:06 UTC) #8
esprehn
https://codereview.chromium.org/216803002/diff/120001/Source/core/css/CSSComputedStyleDeclaration.cpp File Source/core/css/CSSComputedStyleDeclaration.cpp (right): https://codereview.chromium.org/216803002/diff/120001/Source/core/css/CSSComputedStyleDeclaration.cpp#newcode2871 Source/core/css/CSSComputedStyleDeclaration.cpp:2871: isInherit = isInherit && value->isInheritedValue(); What is this doing? ...
6 years, 7 months ago (2014-05-02 22:24:11 UTC) #9
dglazkov
Folks, let's do the intent to ship&implement ASAP. Peeps in the wild are looking for ...
6 years, 7 months ago (2014-05-11 17:19:50 UTC) #10
esprehn
I'm not sure this is how we want to implement this btw, expanding all into ...
6 years, 7 months ago (2014-05-11 22:33:43 UTC) #11
kenjibaheux
Sent the intent to implement and ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/VkDKCvdVKlA/yGYNtgxnsHMJ Adam chimed in with questions about the ...
6 years, 7 months ago (2014-05-12 09:18:15 UTC) #12
tasak
Thank you for reviewing. I have just uploaded a revised patch. https://codereview.chromium.org/216803002/diff/120001/Source/build/scripts/make_css_shorthands.py File Source/build/scripts/make_css_shorthands.py (right): ...
6 years, 7 months ago (2014-05-12 14:33:26 UTC) #13
tasak
Would you review this CL? I tried another approach: - Only StylePropertySerializer expands "all" property ...
6 years, 7 months ago (2014-05-23 10:24:04 UTC) #14
dglazkov
rune, esprehn -- penny for your thought.
6 years, 6 months ago (2014-05-29 22:25:49 UTC) #15
esprehn
This patch is really big, so it might take a few passes. Here's a first ...
6 years, 6 months ago (2014-05-30 00:59:22 UTC) #16
tasak
Thank you for reviewing. https://codereview.chromium.org/216803002/diff/280018/LayoutTests/fast/css/all-shorthand-css-text.html File LayoutTests/fast/css/all-shorthand-css-text.html (right): https://codereview.chromium.org/216803002/diff/280018/LayoutTests/fast/css/all-shorthand-css-text.html#newcode3 LayoutTests/fast/css/all-shorthand-css-text.html:3: <head> On 2014/05/30 00:59:22, esprehn ...
6 years, 6 months ago (2014-06-04 09:37:41 UTC) #17
tasak
Looking at inspector/console/console-format-style-whitelist.html failure, the test invokes MutablePropertySet::setProperty with "border-image-source: initial;", "border-image-slice: initial;", "border-image-width: initial;" ...
6 years, 6 months ago (2014-06-05 08:56:54 UTC) #18
tasak
Would you review this CL?
6 years, 6 months ago (2014-06-09 06:29:22 UTC) #19
esprehn
6 years, 6 months ago (2014-06-12 00:36:45 UTC) #20
This patch is really difficult to review because it's so large. Can we land this
in parts like:

1) tests but failing
2) parsing
3) land the real feature (maybe we can break this into parts somewhere?)

It's hard to know if this code is correct since the patch is so big and full of
so many special cases for assorted properties.

https://codereview.chromium.org/216803002/diff/470001/LayoutTests/fast/css/al...
File LayoutTests/fast/css/all-shorthand-expected.html (right):

https://codereview.chromium.org/216803002/diff/470001/LayoutTests/fast/css/al...
LayoutTests/fast/css/all-shorthand-expected.html:2: <html>
Leave out html, head and body.

https://codereview.chromium.org/216803002/diff/470001/LayoutTests/fast/css/ge...
File LayoutTests/fast/css/getComputedStyle/getComputedStyle-all.html (right):

https://codereview.chromium.org/216803002/diff/470001/LayoutTests/fast/css/ge...
LayoutTests/fast/css/getComputedStyle/getComputedStyle-all.html:3: <head>
ditto

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
File Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:58: static bool
canAllOverwriteProperty(const StylePropertySet::PropertyReference& all, unsigned
allIndex, const StylePropertySet::PropertyReference& property, unsigned
propertyIndex)
Can you explain the purpose of this?

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:110:
m_propertyVector.insert(position++, CSSPropertyInternal(propertyId, value,
all.isImportant(), value->isInheritedValue(), all.isImplicit()));
All these vector inserts are going to be really expensive.

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:134: return
m_propertyVector.at(index);
m_propertyVector[index] ?

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:145: if (CSSValue* value =
m_propertyVector.at(index).value())
ditto

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:158: return
m_propertyVector.at(index).value();
ditto

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:167: if (m_propertyVector.at(i).id()
== propertyId)
m_propertyVector[i].id()

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/StylePr...
Source/core/css/StylePropertySerializer.cpp:1029: bool
StylePropertySerializer::hasAllShorthand(String& result) const
has*() methods shouldn't really reaturn stuff like this, and returning a String
by way of a non-const ref is very strange.

Can we just return a String from this and return emptyString() when you would
have returned false?

I'd suggest calling it allShorthand() or something?

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/resolve...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1238: return CSSPropertyDisplay;
These all need comments.

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1267: return
static_cast<CSSPropertyID>(lastCSSProperty);
Can you comment this?

https://codereview.chromium.org/216803002/diff/470001/Source/core/css/resolve...
Source/core/css/resolver/StyleResolver.cpp:1277: void
StyleResolver::applyAllProperty(StyleResolverState& state, CSSValue* allValue)
Your patch description needs to explain what this is doing.

Powered by Google App Engine
This is Rietveld 408576698