Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412803007:
Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens (Closed)
|
Created:
5 years, 1 month ago by rwlbuis Modified:
5 years, 1 month ago Reviewers:
Timothy Loh CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParse text-decoration shorthand in CSSPropertyParser with CSSParserTokens
Move text-decoration shorthand property handling from
LegacyCSSPropertyParser into CSSPropertyParser. Also
move its related longhands.
This patch adds a generic consumeShorthandGreedily that can be
used to convert other shorthands.
Finally the patch refuses duplicate keyword entries for text-decoration-line
(like "blink blink") and adds tests for this to text-decoration-line.html.
BUG=499780
Committed: https://crrev.com/df53d14ebf099ad282ccbdd066e892bb6e5cfdaf
Cr-Commit-Position: refs/heads/master@{#357842}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Try to fix failures #Patch Set 4 : Rebase #Patch Set 5 : Fix "blink blink" problem #Patch Set 6 : Alternative fix #
Total comments: 6
Patch Set 7 : Improvements to consumeShorthand #Patch Set 8 : Patch for review #
Total comments: 4
Patch Set 9 : Address review comments #
Total comments: 2
Patch Set 10 : Patch for landing #Patch Set 11 : Speculative fix #Patch Set 12 : Patch for landing #
Messages
Total messages: 47 (20 generated)
Description was changed from ========== text-deco WIP Move two color related properties into CSSPropertyParser Move two color related properties handling from LegacyCSSPropertyParser into CSSPropertyParser. BUG=499780 patch from issue 1415953002 at patchset 100001 (http://crrev.com/1415953002#ps100001) ========== to ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthand that can be used to convert other shorthands. BUG=499780 ==========
rob.buis@samsung.com changed reviewers: + timloh@chromium.org
PTAL. I think we can choose between #5 and #6.
@timloh, I just noticed you have shorthand assigned, not sure if I assigned to myself or not, but this patch makes an attempt so it is better you see this sooner rather than later to prevent duplicate work. The other item is: move fastpath keyword callsite into parseSingleValue This patch works around this while this has not been implemented yet. I think it is ok to do this in comsumeShorthand provided there is a FIXME/TODO reminder.
On 2015/11/02 03:09:30, rwlbuis wrote:
> @timloh, I just noticed you have shorthand assigned, not sure if I assigned to
> myself or not, but this patch makes an attempt so it is better you see this
> sooner rather than later to prevent duplicate work.
Yeah I had a WIP implementation for borders. I looked over your function and I
think I prefer my loop structure (as usual, do-while makes it clear that there
should be at least one value), but having a CSSValue array instead of a boolean
array is probably better. Perhaps longhandValue or similar works better as a
name than foundProperty. BTW we don't need the propertiesForInitialization logic
in this patch, and I'm planning on making border shorthand parsing not use
consumeShorthand so won't have to add it. Also the CSSPropertyID argument isn't
needed :)
My implementation:
bool CSSPropertyParser::consumeShorthandGreedily(const StylePropertyShorthand&
shorthand, bool important)
{
ASSERT(shorthand.length() <= 6); // Existing shorthands have at most 6
longhands
const CSSPropertyID* longhands = shorthand.properties();
bool parsedLonghand[6] = { };
do {
bool foundProperty = false;
for (size_t i = 0; i < shorthand.length(); ++i) {
if (parsedLonghand[i])
continue;
if (RefPtrWillBeRawPtr<CSSValue> value =
parseSingleValue(longhands[i])) {
addProperty(longhands[i], value, important);
parsedLonghand[i] = true;
foundProperty = true;
break;
}
}
if (!foundProperty)
return false;
} while (!m_range.atEnd());
for (size_t i = 0; i < shorthand.length(); ++i) {
if (!parsedLonghand[i])
addProperty(longhands[i],
cssValuePool().createImplicitInitialValue(), important);
}
return true;
}
> The other item is:
> move fastpath keyword callsite into parseSingleValue
>
> This patch works around this while this has not been implemented yet. I think
it
> is ok to do this in comsumeShorthand provided there is a FIXME/TODO reminder.
It didn't occur to me to do work around it this way, but it seems fine (with a
TODO). We can move the fast-path keyword logic across when there are no
shorthands in the legacy property parser which need it.
https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1429: ListHashSet<int>::AddResult result = ids.add(ident->getValueID()); I guess we can just use list->hasValue(ident.get()) instead of having a ListHashSet. https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1435: // Values are either valid or in shorthand scope. I don't understand this comment :\ https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1542: ASSERT(property != CSSPropertyTextDecorationColor || RuntimeEnabledFeatures::css3TextDecorationsEnabled()); Might as well just put the text-decoration-color case by itself if we want this assertion
On 2015/11/02 03:26:26, Timothy Loh wrote:
> On 2015/11/02 03:09:30, rwlbuis wrote:
> > @timloh, I just noticed you have shorthand assigned, not sure if I assigned
to
> > myself or not, but this patch makes an attempt so it is better you see this
> > sooner rather than later to prevent duplicate work.
>
> Yeah I had a WIP implementation for borders. I looked over your function and I
> think I prefer my loop structure (as usual, do-while makes it clear that there
> should be at least one value), but having a CSSValue array instead of a
boolean
> array is probably better. Perhaps longhandValue or similar works better as a
> name than foundProperty. BTW we don't need the propertiesForInitialization
logic
> in this patch, and I'm planning on making border shorthand parsing not use
> consumeShorthand so won't have to add it. Also the CSSPropertyID argument
isn't
> needed :)
>
> My implementation:
>
> bool CSSPropertyParser::consumeShorthandGreedily(const StylePropertyShorthand&
> shorthand, bool important)
> {
> ASSERT(shorthand.length() <= 6); // Existing shorthands have at most 6
> longhands
> const CSSPropertyID* longhands = shorthand.properties();
> bool parsedLonghand[6] = { };
>
> do {
> bool foundProperty = false;
> for (size_t i = 0; i < shorthand.length(); ++i) {
> if (parsedLonghand[i])
> continue;
>
> if (RefPtrWillBeRawPtr<CSSValue> value =
> parseSingleValue(longhands[i])) {
> addProperty(longhands[i], value, important);
> parsedLonghand[i] = true;
> foundProperty = true;
> break;
> }
> }
> if (!foundProperty)
> return false;
> } while (!m_range.atEnd());
>
> for (size_t i = 0; i < shorthand.length(); ++i) {
> if (!parsedLonghand[i])
> addProperty(longhands[i],
> cssValuePool().createImplicitInitialValue(), important);
> }
>
> return true;
> }
Oh, cool, actually having two implementations to choose from may be better :)
I'll study consumeShorthandGreedily tomorrow. It is important to get something
like that in soon IMHO since I expect lots of shorthands need it, for example I
already have motion shorthand implemented on top of this CL, and once
consumeShorthand is in those kind of patches should be small and easy to review.
> > The other item is:
> > move fastpath keyword callsite into parseSingleValue
> >
> > This patch works around this while this has not been implemented yet. I
think
> it
> > is ok to do this in comsumeShorthand provided there is a FIXME/TODO
reminder.
>
> It didn't occur to me to do work around it this way, but it seems fine (with a
> TODO). We can move the fast-path keyword logic across when there are no
> shorthands in the legacy property parser which need it.
Agreed.
Description was changed from ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthand that can be used to convert other shorthands. BUG=499780 ========== to ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthandGreedily that can be used to convert other shorthands. BUG=499780 ==========
PTAL. I took most of your consumeShorthandGreedily implementation. However since we chose the array of CSSValue's instead of booleans, it is easier to add the longhands in the final loop since releasing the CSSValues would cause the entries to become zero, and we can't test anymore if we need to add an implicit value.
I had some comments earlier on the text-deco specific stuff in case you missed it https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2005: for (unsigned propertyIndex = 0; !foundLonghand && propertyIndex < shorthand.length(); ++propertyIndex) { Can we be consistent with the loop below (which uses "size_t i")? https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2009: if (CSSParserFastPaths::isKeywordPropertyID(shorthand.properties()[propertyIndex])) { Would be nice to have a variable for shorthand.properties() instead of writing it out five times
https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2005: for (unsigned propertyIndex = 0; !foundLonghand && propertyIndex < shorthand.length(); ++propertyIndex) { On 2015/11/03 05:50:58, Timothy Loh wrote: > Can we be consistent with the loop below (which uses "size_t i")? Done. https://codereview.chromium.org/1412803007/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:2009: if (CSSParserFastPaths::isKeywordPropertyID(shorthand.properties()[propertyIndex])) { On 2015/11/03 05:50:58, Timothy Loh wrote: > Would be nice to have a variable for shorthand.properties() instead of writing > it out five times Done.
Ouch, I missed these remarks first time :| PTAL. https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1429: ListHashSet<int>::AddResult result = ids.add(ident->getValueID()); On 2015/11/02 03:36:28, Timothy Loh wrote: > I guess we can just use list->hasValue(ident.get()) instead of having a > ListHashSet. Good idea, however I think HashSet solution is better so I put it back in :) https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1435: // Values are either valid or in shorthand scope. On 2015/11/02 03:36:28, Timothy Loh wrote: > I don't understand this comment :\ Probably outdated, I removed it. https://codereview.chromium.org/1412803007/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1542: ASSERT(property != CSSPropertyTextDecorationColor || RuntimeEnabledFeatures::css3TextDecorationsEnabled()); On 2015/11/02 03:36:28, Timothy Loh wrote: > Might as well just put the text-decoration-color case by itself if we want this > assertion Done.
lgtm, but mention the behaviour improvement in the patch description :) https://codereview.chromium.org/1412803007/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1412803007/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1510: while (matchKeywords(id, ids)) { just use ids.contains(id) https://codereview.chromium.org/1412803007/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1619: ASSERT(RuntimeEnabledFeatures::css3TextDecorationsEnabled()); I'd prefer if it didn't fall-through (it's a single line anyway).
Description was changed from ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthandGreedily that can be used to convert other shorthands. BUG=499780 ========== to ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthandGreedily that can be used to convert other shorthands. Finally the patch refuses duplicate keyword entries for text-decoration-line (like "blink blink") and adds tests for this to text-decoration-line.html. BUG=499780 ==========
Description was changed from ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthandGreedily that can be used to convert other shorthands. Finally the patch refuses duplicate keyword entries for text-decoration-line (like "blink blink") and adds tests for this to text-decoration-line.html. BUG=499780 ========== to ========== Parse text-decoration shorthand in CSSPropertyParser with CSSParserTokens Move text-decoration shorthand property handling from LegacyCSSPropertyParser into CSSPropertyParser. Also move its related longhands. This patch adds a generic consumeShorthandGreedily that can be used to convert other shorthands. Finally the patch refuses duplicate keyword entries for text-decoration-line (like "blink blink") and adds tests for this to text-decoration-line.html. BUG=499780 ==========
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1412803007/#ps180001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1412803007/#ps200001 (title: "Speculative fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/200001
On 2015/11/04 03:35:09, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1412803007/200001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1412803007/200001 Sigh, some ASSERT related to the contains call (unsigned/int mixup?) Anyway, should be able to fix it tomorrow with a debug build in hand.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob.buis@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1412803007/#ps220001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rob.buis@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412803007/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412803007/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/df53d14ebf099ad282ccbdd066e892bb6e5cfdaf Cr-Commit-Position: refs/heads/master@{#357842}
Message was sent while issue was closed.
Patchset #13 (id:240001) has been deleted
Message was sent while issue was closed.
Looks like this cl may be responsible for crbug/551638? I'll revert for now. Apologies if its not the culprit.
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:220001) has been created in https://codereview.chromium.org/1417733010/ by michaeln@chromium.org. The reason for reverting is: Looks like this cl may be responsible for crbug/551638? Apologies if its not the culprit..
Message was sent while issue was closed.
On 2015/11/05 20:10:53, michaeln wrote: > A revert of this CL (patchset #12 id:220001) has been created in > https://codereview.chromium.org/1417733010/ by mailto:michaeln@chromium.org. > > The reason for reverting is: Looks like this cl may be responsible for > crbug/551638? Apologies if its not the culprit.. It is the culprit. The problem is addressed in https://codereview.chromium.org/1422363007/ , I'll incorporate it with this patch to bring this functionality back. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
