Bug 68040 – Make sure border-image sub-properties can be specified in any order.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dave Hyatt]

Make sure border-image sub-properties can be specified in any order.

[Dave Hyatt]

Created attachment 107252 [details]
Patch

[WebKit Review Bot]

Attachment 107252 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 107252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107252&action=review

> Source/WebCore/css/CSSParser.cpp:5188
> +        m_allowImage = m_allowSlash = m_requireWidth = m_requireOutset = false;

Normally we don’t do those all on one line. Can you put them on their own lines?

[Darin Adler]

Comment on attachment 107252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107252&action=review

> LayoutTests/ChangeLog:7
> +        * fast/borders/border-image-scrambled.html: Added.

Could we test this with computed style instead, so we get a platform-independent test?

[Beth Dakin]

You should address Darin's comments, and the style bot's request for a bug number in the change log.

[Dave Hyatt]

(In reply to comment #3)
> (From update of attachment 107252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107252&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:5188
> > +        m_allowImage = m_allowSlash = m_requireWidth = m_requireOutset = false;
> 
> Normally we don’t do those all on one line. Can you put them on their own lines?

(In reply to comment #4)
> (From update of attachment 107252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107252&action=review
> 
> > LayoutTests/ChangeLog:7
> > +        * fast/borders/border-image-scrambled.html: Added.
> 
> Could we test this with computed style instead, so we get a platform-independent test?

I explained this on IRC, but basically the reason I didn't do this is we are suffering badly from a dearth of shadow, border-image, and background pixel results. For these particular features, pixels are king, and we need to have more (not fewer) samples. Therefore when I write new tests in these areas, I deliberately bias towards pixel results, since they give us more chances to catch mistakes made in the rendering itself.

[Dave Hyatt]

Fixed in r95058.

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/95077

See https://bugs.webkit.org/show_bug.cgi?id=68051 for details.

[Dave Hyatt]

Created attachment 107351 [details]
Patch

This patch is identical to the previous patch but it ensures that when you completely fail to parse any sub-property that you bail instead of infinite looping trying to examine the same property over and over again.

[WebKit Review Bot]

Attachment 107351 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 107352 [details]
Patch that passes style queue.

[WebKit Review Bot]

Attachment 107352 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Last 3072 characters of output:
p://src.chromium.org/svn/trunk/src/build@100742
    should_process: True
    requirements: set(['./'])
  
    name: net
    url: http://src.chromium.org/svn/trunk/src/net@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/net@100742
    should_process: True
    requirements: set(['./'])
  
    name: tools/data_pack
    url: http://src.chromium.org/svn/trunk/src/tools/data_pack@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/data_pack@100742
    should_process: True
    requirements: set(['./'])
  
    name: third_party/ffmpeg
    url: From('chromium_deps', 'src/third_party/ffmpeg')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/generate_stubs
    url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@100742
    should_process: True
    requirements: set(['./'])
  
    name: third_party/libjpeg_turbo
    url: From('chromium_deps', 'src/third_party/libjpeg_turbo')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/v8-i18n
    url: From('chromium_deps', 'src/third_party/v8-i18n')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/grit
    url: http://src.chromium.org/svn/trunk/src/tools/grit@100742
    parsed_url: http://src.chromium.org/svn/trunk/src/tools/grit@100742
    should_process: True
    requirements: set(['./'])
  
    name: base
    url: http://src.chromium.org/svn/trunk/src/base@100742
    should_process: True
    requirements: set(['./'])
  
    name: sql
    url: http://src.chromium.org/svn/trunk/src/sql@100742
    should_process: True
    requirements: set(['./'])
  
    name: v8
    url: From('chromium_deps', 'src/v8')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: testing/gtest
    url: From('chromium_deps', 'src/testing/gtest')
    should_process: True
    requirements: set(['testing', 'chromium_deps', './'])
  
    name: third_party/libwebp
    url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@100742
    should_process: True
    requirements: set(['third_party', './'])
  
    name: googleurl
    url: From('chromium_deps', 'src/googleurl')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: third_party/skia/include
    url: From('chromium_deps', 'src/third_party/skia/include')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/ots
    url: From('chromium_deps', 'src/third_party/ots')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/snappy/src
    url: From('chromium_deps', 'src/third_party/snappy/src')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])

Died at Tools/Scripts/update-webkit-chromium line 80.
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Comment on attachment 107352 [details]
Patch that passes style queue.

Hyatt and I went over the changes in person.

[Dave Hyatt]

Trying again in r95099.