'round' has little rounding errors. Have to break up into two pattern tiles in order to avoid this.
I forgot that we dropped round support. Repeat is busted because the removal of round support was botched.
Changing this bug to be about just implementing 'round' again period.
Created attachment 42575 [details] patch Here's a patch. The test is an adapted border-image-01.html that makes the size of the divs more uneven, so rounding can be properly evaluated.
Created attachment 42585 [details] patch v2 Second try, with nastier testcase.
Comment on attachment 42585 [details] patch v2 Thanks for tackling this. > if (hRule == Image::RepeatTile) > scaleX = scaleY; > + else if (hRule == Image::RoundTile) > + scaleX = dstRect.width() / (srcRect.width() * roundf(dstRect.width() / (srcRect.width() * scaleY))); > + > if (vRule == Image::RepeatTile) > scaleY = scaleX; > + else if (vRule == Image::RoundTile) > + scaleY = dstRect.height() / (srcRect.height() * roundf(dstRect.height() / (srcRect.height() * scaleX))); I think these should use switch statements now. It helps document that we have cases to cover all values. Since you're adding a new regression test, you need to include regression test results as well. Also, a good test documents what the result is supposed to look like so people, not just computers, can judge whether it is a success or failure. Ideally tests can be judged for correctness without requiring pixel-test results, but I suppose that may be impossible in this case. review- because the patch has a new test, but no test results
Created attachment 42674 [details] somewhat bigger patch So I was annoyed yesterday that the code got the scale factors all wrong and set out to change it. This is the result. As it implements border-image-repeat: round as a side effect, I decided to just put it here. The code quickly became complicated and I concentrated more on keeping it readable and understandable, so I probably failed to adhere to the Webkit coding standards in places. But it does work. I also did not add any expected results for the test for a very simple reason: The test is a pixel test, I'm on Linux/gtk and our DRT does not support pixel tests yet. Also, Cairo and CG produce different output for this example; they seem to scale slightly differently. Last but not least: The patch is done on top of the patch in bug 31187. I'm now mostly interested in 3 things: - Are there any bugs in it? - A rough review of the code style. Are there any refactorings needed? - What's the best way for me to get test results?
Please mark the patch for review if you'd like to have it commented on. Also, please see <http://webkit.org/coding/contributing.html> for more information.
Good job, Ben. Thank you!)
That could be implemented 2 years ago. Whats wrong with it?
As comment 7 states, the patch wasn't marked for review. This usually means that it's just a WIP. At the least, the patch needs to be updated to today's codebase before it can be reviewed or commented on, let alone land.
I noticed this when analyzing https://bugs.webkit.org/show_bug.cgi?id=70871 Border images with 'fill' that would otherwise look fine are terrible without support for 'round'.
Ben, any chance you could upload a squashed version of this series and be sure to include the svn rev of WebKit that it is based on?
We'll need this to pass CSS Backgrounds and Borders tests.
Created attachment 188272 [details] Rebased patch Patch for EWS run
Comment on attachment 188272 [details] Rebased patch Marking patch for review to get some comments.
Comment on attachment 188272 [details] Rebased patch Attachment 188272 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16559385 New failing tests: fast/borders/border-image-01.html fast/borders/border-image-rotate-transform.html fast/borders/scaled-border-image.html fast/borders/border-image-outset.html fast/borders/border-image-border-radius.html fast/backgrounds/mask-box-image.html fast/borders/inline-mask-overlay-image-outset-vertical-rl.html fast/borders/border-image-source.html fast/borders/border-image-scaled.html fast/borders/inline-mask-overlay-image-outset.html fast/borders/block-mask-overlay-image-outset.html fast/css/transformed-mask.html fast/borders/border-image-slice-constrained.html fast/forms/datalist/update-range-with-datalist.html fast/borders/border-image-scaled-gradient.html fast/borders/border-image-side-reduction.html fast/borders/border-image-repeat.html css3/filters/filter-mask-clip-order.html animations/cross-fade-border-image-source.html fast/borders/border-image-massive-scale.html fast/borders/border-image-longhand.html fast/borders/border-image-outset-split-inline.html fast/borders/border-image-scrambled.html fast/borders/block-mask-overlay-image.html fast/borders/border-image-omit-right-slice.html fast/borders/border-image-scale-transform.html fast/borders/border-image-slices.html fast/borders/border-image-outset-split-inline-vertical-lr.html fast/borders/border-image-outset-in-shorthand.html fast/borders/inline-mask-overlay-image.html
Created attachment 188377 [details] Updated patch One more EWS run
Created attachment 188414 [details] Updated patch Fixed failing tests. Added test and test result.
Comment on attachment 188414 [details] Updated patch Attachment 188414 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16542785 New failing tests: fast/borders/border-image-rotate-transform.html fast/borders/inline-mask-overlay-image-outset-vertical-rl.html fast/borders/border-image-side-reduction.html fast/borders/border-image-02.html fast/borders/inline-mask-overlay-image-outset.html scrollbars/overflow-scrollbar-combinations.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
Comment on attachment 188414 [details] Updated patch Attachment 188414 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16537829 New failing tests: fast/borders/border-image-rotate-transform.html fast/borders/inline-mask-overlay-image-outset-vertical-rl.html fast/borders/border-image-side-reduction.html fast/borders/border-image-02.html fast/borders/inline-mask-overlay-image-outset.html scrollbars/overflow-scrollbar-combinations.html platform/chromium/virtual/gpu/compositedscrolling/scrollbars/overflow-scrollbar-combinations.html
Created attachment 188478 [details] Updated patch Fixed failing tests. border-image-rotate-transform.html test needs to be regenerated.
Comment on attachment 188478 [details] Updated patch Attachment 188478 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16570746 New failing tests: fast/borders/border-image-02.html
(In reply to comment #22) > (From update of attachment 188478 [details]) > Attachment 188478 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://queues.webkit.org/results/16570746 > > New failing tests: > fast/borders/border-image-02.html I will update TestExpectations in patch after review comments.
Created attachment 188641 [details] Updated patch Updated TestExpectations for other platforms.
Created attachment 188644 [details] Rebased patch Updated TestExpectations for other platforms.
Comment on attachment 188644 [details] Rebased patch Attachment 188644 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16600663 New failing tests: inspector/styles/stylesheet-tracking.html
Comment on attachment 188644 [details] Rebased patch This looks good. r=me I think we should perhaps seek clarification regarding painting order of border images. My instinct would be that the stacking order would be that we would paint the middle, then the sides, and finally the corners on top. In the case of overlap, I'd expect the middle to be underneath the side edges and corners, and I'd expect the sides to be underneath the corners. Might be worth filing a follow-up bug about that. When you do land this, watch the tree and be on IRC in case this causes regressions that necessitate a rollout.
Oh one other thing. This is obviously not the best implementation of "round," since it's going to be very prone to pixel cracking. The reason I never implemented "round" is because doing a really good implementation is pretty involved. You can't get away with just doing one tiling pass. Ideally you want all the tiles to be integral widths, which means working out a strategy for how to distribute the extra space to the tiles to make that happen.
So basically I'm saying we should file a follow-up bug for working out the paint order and another follow-up bug for improving the rounding algorithm.
Created attachment 188991 [details] Patch for landing
Attachment 188991 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 188993 [details] Patch for landing
Attachment 188993 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 188995 [details] Patch for landing
Attachment 188995 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #35) > Attachment 188995 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1 > LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] > Total errors found: 1 in 17 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. This seems strange even though my subversion config file has properties set.
Created attachment 189000 [details] Patch for landing
Attachment 189000 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-image-02.html', u'LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/chromium/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.png', u'LayoutTests/platform/gtk/fast/borders/border-image-02-expected.txt', u'LayoutTests/platform/mac-wk2/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/GraphicsContext.cpp', u'Source/WebCore/platform/graphics/GraphicsContext.h', u'Source/WebCore/platform/graphics/Image.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/rendering/RenderBoxModelObject.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/borders/border-image-02-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
style error seems like bug in bot https://bugs.webkit.org/show_bug.cgi?id=107724
(In reply to comment #27) > (From update of attachment 188644 [details]) > This looks good. r=me > > I think we should perhaps seek clarification regarding painting order of border images. My instinct would be that the stacking order would be that we would paint the middle, then the sides, and finally the corners on top. In the case of overlap, I'd expect the middle to be underneath the side edges and corners, and I'd expect the sides to be underneath the corners. Might be worth filing a follow-up bug about that. > https://bugs.webkit.org/show_bug.cgi?id=110185 >follow-up bug for improving the rounding algorithm. https://bugs.webkit.org/show_bug.cgi?id=110188 > When you do land this, watch the tree and be on IRC in case this causes regressions that necessitate a rollout. Sure I will try to land this tomorrow and check the tree if it causes any regressions. I will be on IRC too (:ukr) Hopefully no regression :) Thanks for review.
Comment on attachment 189000 [details] Patch for landing Rejecting attachment 189000 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189000, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: um/third_party/v8-i18n --revision 164 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 50>At revision 164. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Total errors found: 0 in 6 files Full output: http://queues.webkit.org/results/16626841
Created attachment 189148 [details] Patch for landing
Comment on attachment 189148 [details] Patch for landing Rejecting attachment 189148 [details] from commit-queue. New failing tests: platform/chromium/fast/forms/calendar-picker/calendar-picker-appearance-required.html Full output: http://queues.webkit.org/results/16636103
Comment on attachment 189148 [details] Patch for landing Failing test is unrelated to mine. Setting cq? again.
Comment on attachment 189148 [details] Patch for landing Clearing flags on attachment: 189148 Committed r143419: <http://trac.webkit.org/changeset/143419>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 110507
Created attachment 264032 [details] Patch
Comment on attachment 264032 [details] Patch Clearing flags on attachment: 264032 Committed r191590: <http://trac.webkit.org/changeset/191590>
<rdar://problem/23405291>