Bug 14185

Summary: Implement 'round' and 'space' values for border-image
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: CSSAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, davidbarr, dglazkov, eric, gyuyoung.kim, hyatt, jonlee, ojan.autocc, peter, qfox, rakuco, rniwa, sabouhallawa, simon.fraser, syoichi, udaykiran4u, webkit-bug-importer, webkit.bugzilla, webkit.review.bot
Priority: P2 Keywords: InRadar, WebExposed
Version: 523.x (Safari 3)   
Hardware: Macintosh   
OS: OS X 10.4   
Bug Depends on: 110507    
Bug Blocks: 110185, 110188    
Attachments:
Description Flags
patch
none
patch v2
none
somewhat bigger patch
none
Rebased patch
none
Updated patch
none
Updated patch
none
Updated patch
none
Updated patch
none
Rebased patch
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch none

Description Dave Hyatt 2007-06-15 16:46:58 PDT
'round' has little rounding errors.  Have to break up into two pattern tiles in order to avoid this.
Comment 1 Dave Hyatt 2007-06-15 17:32:45 PDT
I forgot that we dropped round support.  Repeat is busted because the removal of round support was botched.
Comment 2 Dave Hyatt 2007-06-15 17:36:20 PDT
Changing this bug to be about just implementing 'round' again period.
Comment 3 Benjamin Otte 2009-11-05 10:10:34 PST
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.
Comment 4 Benjamin Otte 2009-11-05 11:57:03 PST
Created attachment 42585 [details]
patch v2

Second try, with nastier testcase.
Comment 5 Darin Adler 2009-11-05 16:19:27 PST
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
Comment 6 Benjamin Otte 2009-11-06 15:10:00 PST
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?
Comment 7 Alexey Proskuryakov 2010-06-12 15:46:41 PDT
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.
Comment 8 Alex Yaroshevich 2011-06-01 06:37:43 PDT
Good job, Ben. Thank you!)
Comment 9 Alex Yaroshevich 2011-06-01 07:01:08 PDT
That could be implemented 2 years ago. Whats wrong with it?
Comment 10 Peter Beverloo 2011-06-01 07:05:33 PDT
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.
Comment 11 David Barr 2011-11-15 20:02:44 PST
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'.
Comment 12 David Barr 2011-11-15 20:17:42 PST
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?
Comment 13 Simon Fraser (smfr) 2013-02-04 13:50:08 PST
We'll need this to pass CSS Backgrounds and Borders tests.
Comment 14 Uday Kiran 2013-02-14 00:08:16 PST
Created attachment 188272 [details]
Rebased patch

Patch for EWS run
Comment 15 Uday Kiran 2013-02-14 00:16:08 PST
Comment on attachment 188272 [details]
Rebased patch

Marking patch for review to get some comments.
Comment 16 WebKit Review Bot 2013-02-14 02:27:20 PST
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
Comment 17 Uday Kiran 2013-02-14 10:20:07 PST
Created attachment 188377 [details]
Updated patch

One more EWS run
Comment 18 Uday Kiran 2013-02-14 13:38:22 PST
Created attachment 188414 [details]
Updated patch

Fixed failing tests. Added test and test result.
Comment 19 WebKit Review Bot 2013-02-14 16:16:57 PST
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 20 WebKit Review Bot 2013-02-14 17:01:51 PST
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
Comment 21 Uday Kiran 2013-02-14 21:58:23 PST
Created attachment 188478 [details]
Updated patch

Fixed failing tests. border-image-rotate-transform.html test needs to be regenerated.
Comment 22 Build Bot 2013-02-15 01:24:39 PST
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
Comment 23 Uday Kiran 2013-02-15 09:25:31 PST
(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.
Comment 24 Uday Kiran 2013-02-15 14:15:15 PST
Created attachment 188641 [details]
Updated patch

Updated TestExpectations for other platforms.
Comment 25 Uday Kiran 2013-02-15 14:37:13 PST
Created attachment 188644 [details]
Rebased patch

Updated TestExpectations for other platforms.
Comment 26 Build Bot 2013-02-17 02:31:21 PST
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 27 Dave Hyatt 2013-02-18 10:33:49 PST
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.
Comment 28 Dave Hyatt 2013-02-18 10:35:59 PST
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.
Comment 29 Dave Hyatt 2013-02-18 10:36:39 PST
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.
Comment 30 Uday Kiran 2013-02-18 22:29:58 PST
Created attachment 188991 [details]
Patch for landing
Comment 31 WebKit Review Bot 2013-02-18 22:33:10 PST
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.
Comment 32 Uday Kiran 2013-02-18 22:33:43 PST
Created attachment 188993 [details]
Patch for landing
Comment 33 WebKit Review Bot 2013-02-18 22:37:31 PST
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.
Comment 34 Uday Kiran 2013-02-18 22:43:14 PST
Created attachment 188995 [details]
Patch for landing
Comment 35 WebKit Review Bot 2013-02-18 22:46:24 PST
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.
Comment 36 Uday Kiran 2013-02-18 22:48:57 PST
(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.
Comment 37 Uday Kiran 2013-02-18 23:16:32 PST
Created attachment 189000 [details]
Patch for landing
Comment 38 WebKit Review Bot 2013-02-18 23:20:15 PST
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.
Comment 39 Uday Kiran 2013-02-18 23:28:19 PST
style error seems like bug in bot https://bugs.webkit.org/show_bug.cgi?id=107724
Comment 40 Uday Kiran 2013-02-18 23:46:22 PST
(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 41 WebKit Review Bot 2013-02-19 12:49:35 PST
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
Comment 42 Uday Kiran 2013-02-19 13:01:28 PST
Created attachment 189148 [details]
Patch for landing
Comment 43 WebKit Review Bot 2013-02-19 16:21:17 PST
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 44 Uday Kiran 2013-02-19 17:00:57 PST
Comment on attachment 189148 [details]
Patch for landing

Failing test is unrelated to mine. Setting cq? again.
Comment 45 WebKit Review Bot 2013-02-19 17:47:57 PST
Comment on attachment 189148 [details]
Patch for landing

Clearing flags on attachment: 189148

Committed r143419: <http://trac.webkit.org/changeset/143419>
Comment 46 WebKit Review Bot 2013-02-19 17:48:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 47 WebKit Review Bot 2013-02-21 13:05:54 PST
Re-opened since this is blocked by bug 110507
Comment 48 Simon Fraser (smfr) 2015-10-25 20:33:13 PDT
Created attachment 264032 [details]
Patch
Comment 49 WebKit Commit Bot 2015-10-26 11:39:51 PDT
Comment on attachment 264032 [details]
Patch

Clearing flags on attachment: 264032

Committed r191590: <http://trac.webkit.org/changeset/191590>
Comment 50 WebKit Commit Bot 2015-10-26 11:40:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 51 Radar WebKit Bug Importer 2015-11-04 17:35:52 PST
<rdar://problem/23405291>