Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153353002:
Fast path when setting border-image-width to initial always fails (Closed)
|
Created:
5 years, 6 months ago by Sunil Ratnu Modified:
5 years, 6 months ago CC:
blink-reviews, blink-reviews-style_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFast path when setting border-image-width to initial always fails
In applyInitialCSSPropertyBorderImageWidth the lengthTypeAndValueMatch check
fails as each BorderImageLength is initialized as 1.0/NumberType, but the
comparison is for 1/LengthType. We add another method numberTypeAndValueMatch()
so that the comparsion is done properly keeping in mind what we initialize.
BUG=492572
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196380
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments #
Total comments: 4
Patch Set 3 : Addressed review comments #Patch Set 4 : Addressed review comments #
Total comments: 4
Patch Set 5 : properly naming methods #Messages
Total messages: 18 (3 generated)
sunil.ratnu@samsung.com changed reviewers: + dstockwell@chromium.org, shans@chromium.org
Please take a look. Thanks!
https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:191: static bool numberTypeAndValueMatch(const BorderImageLengthBox& borderImageLengthBox, float value) Naming is weird, lengthTypeAndValueMatch is checking the Length's type and value, but here we're only checking the number value. It might be cleaner to just pass in a BorderImageLength here and use its operator==, and not need separate paths for lengths/numbers. https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:222: if (numberTypeAndValueMatch(currentImage.borderSlices(), 0)) I think this is wrong. The default for -webkit-mask-box-image-width is auto (see NinePieceImage::setMaskDefaults), not 0. https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:225: if (numberTypeAndValueMatch(currentImage.borderSlices(), 1.0)) This is fixing a bug, right? We could add a test here even though it's not something people are likely to run into. In the example below, the div should have a value of 1 even though it'll have one of 1px. <style> #d { border-image-width: 1px; } div { border-image-width: initial; } </style> <div id=d></div>
sunil.ratnu@samsung.com changed reviewers: + timloh@chromium.org
Hi Timothy, Thanks for the quick review. I've made the changes suggested by you. I will try to come up with a test case if possible. Meanwhile, please take a look at the updated patchset. Thanks Sunil https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:191: static bool numberTypeAndValueMatch(const BorderImageLengthBox& borderImageLengthBox, float value) On 2015/05/29 02:10:15, Timothy Loh wrote: > Naming is weird, lengthTypeAndValueMatch is checking the Length's type and > value, but here we're only checking the number value. It might be cleaner to > just pass in a BorderImageLength here and use its operator==, and not need > separate paths for lengths/numbers. Made the changes. Now passing BorderImageLength as the parameter here but for that we need to construct the BorderImageLength object at the caller site. This does avoid writing multiple functions but at the same time makes us to create an additional object. https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:222: if (numberTypeAndValueMatch(currentImage.borderSlices(), 0)) On 2015/05/29 02:10:15, Timothy Loh wrote: > I think this is wrong. The default for -webkit-mask-box-image-width is auto (see > NinePieceImage::setMaskDefaults), not 0. Done. https://codereview.chromium.org/1153353002/diff/1/Source/build/scripts/templa... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:225: if (numberTypeAndValueMatch(currentImage.borderSlices(), 1.0)) On 2015/05/29 02:10:15, Timothy Loh wrote: > This is fixing a bug, right? We could add a test here even though it's not > something people are likely to run into. In the example below, the div should > have a value of 1 even though it'll have one of 1px. > > <style> > #d { border-image-width: 1px; } > div { border-image-width: initial; } > </style> > <div id=d></div> Not sure if its fixing a bug for which we can have the tests. But as per your suggestion, I will try to write out a test if possible.
lgtm On 2015/05/29 13:56:01, Sunil Ratnu wrote: > Not sure if its fixing a bug for which we can have the tests. But as per your > suggestion, I will try to write out a test if possible. This is definitely fixing a bug, we probably don't need to bother adding a test.
https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:199: if (lengthTypeAndValueMatch(currentImage.borderSlices(), BorderImageLength(Length(0, Auto)))) Better to leave out the first argument and just write Length(Auto).
BTW can you double-check that we don't go through here when the value is initial? Thanks :)
Hi Timothy, Thanks for the review. I've made the suggested changes. Also, I double checked that after making these changes the lengthTypeAndValueMatch() does not fail for initial values. It was failing earlier all the time. Could you please have a look at the final changes so that we can commit. Thanks https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:199: if (lengthTypeAndValueMatch(currentImage.borderSlices(), BorderImageLength(Length(0, Auto)))) On 2015/06/02 00:38:09, Timothy Loh wrote: > Better to leave out the first argument and just write Length(Auto). Done. Changed it at the 2 other places as well. Changed this one to Length() as it automatically assigns type "Auto".
https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:199: if (lengthTypeAndValueMatch(currentImage.borderSlices(), BorderImageLength(Length(0, Auto)))) On 2015/06/02 10:02:02, Sunil Ratnu wrote: > On 2015/06/02 00:38:09, Timothy Loh wrote: > > Better to leave out the first argument and just write Length(Auto). > > Done. Changed it at the 2 other places as well. Changed this one to Length() as > it automatically assigns type "Auto". Sorry if I wasn't clear here, I want this one to be Length(Auto) and the others to stay as Length(0, Fixed). The Length class has several types (Fixed, Percent, Auto, and so on). Some of these have numeric values associated with them (Fixed and Percent have a float, Calculated has a special int), and others are just keywords (Auto and everything else). So writing Length(Fixed) reads a bit weirdly since a Fixed Length should always have a numeric value. For Length() vs. Length(Auto), the latter is more readily understandable since it isn't obvious what the default Length value should be (e.g. Length(0, Fixed) would be a sensible default as well).
Hi Timothy, Made the suggested changes. PTAL. Thanks https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/20001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:199: if (lengthTypeAndValueMatch(currentImage.borderSlices(), BorderImageLength(Length(0, Auto)))) On 2015/06/02 12:07:24, Timothy Loh wrote: > On 2015/06/02 10:02:02, Sunil Ratnu wrote: > > On 2015/06/02 00:38:09, Timothy Loh wrote: > > > Better to leave out the first argument and just write Length(Auto). > > > > Done. Changed it at the 2 other places as well. Changed this one to Length() > as > > it automatically assigns type "Auto". > > Sorry if I wasn't clear here, I want this one to be Length(Auto) and the others > to stay as Length(0, Fixed). The Length class has several types (Fixed, Percent, > Auto, and so on). Some of these have numeric values associated with them (Fixed > and Percent have a float, Calculated has a special int), and others are just > keywords (Auto and everything else). So writing Length(Fixed) reads a bit > weirdly since a Fixed Length should always have a numeric value. For Length() > vs. Length(Auto), the latter is more readily understandable since it isn't > obvious what the default Length value should be (e.g. Length(0, Fixed) would be > a sensible default as well). Yup, makes it more readable and understanble. Made the changes.
Hi Timothy, Can we land this now? Thanks
https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:160: static bool lengthTypeAndValueMatch(const LengthBox& lengthBox, const Length& length) rename to lengthMatchesAllSides? https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:168: static bool lengthTypeAndValueMatch(const BorderImageLengthBox& borderImageLengthBox, const BorderImageLength& borderImageLength) rename to borderImageLengthMatchesAllSides?
Hi Timothy, I've made the suggested changes. PTAL. Thanks https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... File Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl (right): https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:160: static bool lengthTypeAndValueMatch(const LengthBox& lengthBox, const Length& length) On 2015/06/03 05:27:44, Timothy Loh wrote: > rename to lengthMatchesAllSides? Seems more appropriate. Done. https://codereview.chromium.org/1153353002/diff/60001/Source/build/scripts/te... Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl:168: static bool lengthTypeAndValueMatch(const BorderImageLengthBox& borderImageLengthBox, const BorderImageLength& borderImageLength) On 2015/06/03 05:27:44, Timothy Loh wrote: > rename to borderImageLengthMatchesAllSides? Seems more appropriate. Done.
lgtm
The CQ bit was checked by sunil.ratnu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153353002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196380 |
