Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1615093005:
Move border-image related longhands into CSSPropertyParser (Closed)
|
Created:
4 years, 11 months ago by rwlbuis Modified:
4 years, 10 months ago Reviewers:
Timothy Loh CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove border-image related longhands into CSSPropertyParser
Move the following border-image related longhands from
LegacyCSSPropertyParser into CSSPropertyParser:
border-image-outset, webkit-mask-box-image-outset
border-image-repeat, webkit-mask-box-image-repeat
border-image-slice, webkit-mask-box-image-slice
border-image-width, webkit-mask-box-image-width
Because consumeRadii can be reused in the new helper methods
rename it to the more generic consume4Sides and start using it.
BUG=499780
Committed: https://crrev.com/616af71fd4830b79f8368ba009b2dad8bed42cd6
Cr-Commit-Position: refs/heads/master@{#372966}
Patch Set 1 #Patch Set 2 : Add image-slice #Patch Set 3 : Rebase #Patch Set 4 : Add width+outset #Patch Set 5 : Fix #Patch Set 6 : Really fix #Patch Set 7 : Prepare for review #
Total comments: 4
Patch Set 8 : Patch for landing #
Messages
Total messages: 22 (14 generated)
Description was changed from ========== image-repeat WIP BUG= ========== to ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused for the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ==========
Description was changed from ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused for the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ========== to ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ==========
Description was changed from ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ========== to ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ==========
rob.buis@samsung.com changed reviewers: + timloh@chromium.org
PTAL. Note that this is a building block for the properties webkit-box-reflect, border-image and webkit-border-image, which I plan to make a CL for next. That one should allow us to remove a lot of code, like the FooParseContext helper classes!
lgtm Apparently the legacy parser has around 500 LoC to parse the border-image properties. It'll be great to see the parse context classes go. https://codereview.chromium.org/1615093005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1615093005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3194: RefPtrWillBeRawPtr<CSSPrimitiveValue> firstValue = nullptr; first -> horizontal, second -> vertical Why not just (and similar for the vertical) horizontalValue = consumeIdent<...reallylongline...>(range); if (!horizontalValue) return nullptr; Also maybe nicer to define a consumeBorderImageRepeatKeyword? https://codereview.chromium.org/1615093005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3216: RefPtrWillBeRawPtr<CSSPrimitiveValue> value = nullptr; Better to declare this inside the loop since it's only used there (also in the next two functions) https://codereview.chromium.org/1615093005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3227: if (consumeIdent<CSSValueFill>(range)) { Maybe nicer as follows: if (!fill && consumeIdent<CSSValueFill>(range)) fill = true; https://codereview.chromium.org/1615093005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3274: if (!value && range.peek().id() == CSSValueAuto) How about if (!value) value = consumeIdent<CSSValueAuto>(range);
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/1615093005/#ps140001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615093005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615093005/140001
Patchset #8 (id:140001) has been deleted
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1615093005/#ps160001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615093005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615093005/160001
Patchset #8 (id:160001) has been deleted
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/1615093005/#ps180001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1615093005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1615093005/180001
Message was sent while issue was closed.
Description was changed from ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ========== to ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 ========== to ========== Move border-image related longhands into CSSPropertyParser Move the following border-image related longhands from LegacyCSSPropertyParser into CSSPropertyParser: border-image-outset, webkit-mask-box-image-outset border-image-repeat, webkit-mask-box-image-repeat border-image-slice, webkit-mask-box-image-slice border-image-width, webkit-mask-box-image-width Because consumeRadii can be reused in the new helper methods rename it to the more generic consume4Sides and start using it. BUG=499780 Committed: https://crrev.com/616af71fd4830b79f8368ba009b2dad8bed42cd6 Cr-Commit-Position: refs/heads/master@{#372966} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/616af71fd4830b79f8368ba009b2dad8bed42cd6 Cr-Commit-Position: refs/heads/master@{#372966} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
