Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227503002:
Allow use of @apply and custom properties in InlineStylePropertyMap::getProperties() (Closed)
|
Created:
4 years, 4 months ago by meade_UTC10 Modified:
4 years, 4 months ago 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. |
DescriptionAllow use of @apply and custom properties in InlineStylePropertyMap::getProperties()
BUG=631368
Committed: https://crrev.com/05df3156828407c7a51f8af78d4ba27764435114
Cr-Commit-Position: refs/heads/master@{#411011}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add layout test #Patch Set 3 : Remove extra if statement #
Total comments: 4
Patch Set 4 : Make @apply string static local #Patch Set 5 : Fix static vars #
Total comments: 4
Patch Set 6 : Remove extra DCHECK #Patch Set 7 : Fix static vars with brackets #Patch Set 8 : Remove extra DCHECK #
Dependent Patchsets: Messages
Total messages: 30 (12 generated)
meade@chromium.org changed reviewers: + sashab@chromium.org
lgtm https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:63: } else if (propertyID >= firstCSSProperty && propertyID <= lastUnresolvedCSSProperty) { This should really be a function... But it's used everywhere atm :'( So this is fine for now.
On 2016/08/08 at 05:30:38, sashab wrote: > lgtm > > https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): > > https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:63: } else if (propertyID >= firstCSSProperty && propertyID <= lastUnresolvedCSSProperty) { > This should really be a function... But it's used everywhere atm :'( So this is fine for now. Oops!! Add a layout test pls :D
Added a layout test :) https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:63: } else if (propertyID >= firstCSSProperty && propertyID <= lastUnresolvedCSSProperty) { On 2016/08/08 05:30:38, sashab wrote: > This should really be a function... But it's used everywhere atm :'( So this is > fine for now. Oops, that shouldn't even be there! Removed.
lgtm https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:64: result.append(getPropertyNameString(propertyID)); Maybe add the if-statement back as a DCHECK? :)
meade@chromium.org changed reviewers: + timloh@chromium.org
lgtm
The CQ bit was checked by meade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
This is adding duplicate @apply, I don't think we're supposed to do that? https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:62: result.append("@apply"); This is mallocing a string for every @apply rule, I think you want a static string instead. DEFINE_STATIC_LOCAL(const String, "@apply", kAtApply) or something. This also adds duplicate @apply entries into the return value, I don't think it's supposed to ever have duplicates.
The CQ bit was unchecked by meade@chromium.org
https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:62: result.append("@apply"); On 2016/08/10 00:39:43, esprehn wrote: > This is mallocing a string for every @apply rule, I think you want a static > string instead. > > DEFINE_STATIC_LOCAL(const String, "@apply", kAtApply) > > or something. > > This also adds duplicate @apply entries into the return value, I don't think > it's supposed to ever have duplicates. Done. https://codereview.chromium.org/2227503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:64: result.append(getPropertyNameString(propertyID)); On 2016/08/09 01:14:36, sashab wrote: > Maybe add the if-statement back as a DCHECK? :) Done.
lgtm https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:65: result.append(kAtApply); Hmm, I also wonder if we're supposed to add the name of the apply or just skip @apply? What does the spec say? This patch is probably okay to land, but the more I think about this having an @apply value show up in the return value of getProperties() is very weird. https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:69: DCHECK(propertyID >= firstCSSProperty && propertyID <= lastUnresolvedCSSProperty); getPropertyNameString actually contains this assert https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/CSSPropertyName...
https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp (right): https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:65: result.append(kAtApply); On 2016/08/10 04:01:06, esprehn wrote: > Hmm, I also wonder if we're supposed to add the name of the apply or just skip > @apply? What does the spec say? This patch is probably okay to land, but the > more I think about this having an @apply value show up in the return value of > getProperties() is very weird. The spec doesn't say anything at all about @apply yet. I've filed https://github.com/w3c/css-houdini-drafts/issues/276. https://codereview.chromium.org/2227503002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/InlineStylePropertyMap.cpp:69: DCHECK(propertyID >= firstCSSProperty && propertyID <= lastUnresolvedCSSProperty); On 2016/08/10 04:01:06, esprehn wrote: > getPropertyNameString actually contains this assert > > https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/CSSPropertyName... Removed.
The CQ bit was checked by meade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, timloh@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2227503002/#ps100001 (title: "Remove extra DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by meade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org, timloh@chromium.org, sashab@chromium.org Link to the patchset: https://codereview.chromium.org/2227503002/#ps140001 (title: "Remove extra DCHECK")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meade@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.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Allow use of @apply and custom properties in InlineStylePropertyMap::getProperties() BUG=631368 ========== to ========== Allow use of @apply and custom properties in InlineStylePropertyMap::getProperties() BUG=631368 Committed: https://crrev.com/05df3156828407c7a51f8af78d4ba27764435114 Cr-Commit-Position: refs/heads/master@{#411011} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/05df3156828407c7a51f8af78d4ba27764435114 Cr-Commit-Position: refs/heads/master@{#411011} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
