Bug 68040 - Make sure border-image sub-properties can be specified in any order.
Summary: Make sure border-image sub-properties can be specified in any order.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on: 68051 68058
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 16:18 PDT by Dave Hyatt
Modified: 2011-09-14 10:57 PDT (History)
3 users (show)

See Also:


Attachments
Patch (53.46 KB, patch)
2011-09-13 16:19 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (53.35 KB, patch)
2011-09-14 10:37 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch that passes style queue. (53.43 KB, patch)
2011-09-14 10:41 PDT, Dave Hyatt
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2011-09-13 16:18:05 PDT
Make sure border-image sub-properties can be specified in any order.
Comment 1 Dave Hyatt 2011-09-13 16:19:51 PDT
Created attachment 107252 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-13 16:22:40 PDT
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.
Comment 3 Darin Adler 2011-09-13 16:31:54 PDT
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?
Comment 4 Darin Adler 2011-09-13 16:33:15 PDT
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?
Comment 5 Beth Dakin 2011-09-13 16:35:25 PDT
You should address Darin's comments, and the style bot's request for a bug number in the change log.
Comment 6 Dave Hyatt 2011-09-13 17:00:41 PDT
(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.
Comment 7 Dave Hyatt 2011-09-13 17:16:27 PDT
Fixed in r95058.
Comment 8 Csaba Osztrogonác 2011-09-13 23:46:45 PDT
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.
Comment 9 Dave Hyatt 2011-09-14 10:37:39 PDT
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.
Comment 10 WebKit Review Bot 2011-09-14 10:40:14 PDT
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.
Comment 11 Dave Hyatt 2011-09-14 10:41:53 PDT
Created attachment 107352 [details]
Patch that passes style queue.
Comment 12 WebKit Review Bot 2011-09-14 10:42:58 PDT
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.
Comment 13 Beth Dakin 2011-09-14 10:51:20 PDT
Comment on attachment 107352 [details]
Patch that passes style queue.

Hyatt and I went over the changes in person.
Comment 14 Dave Hyatt 2011-09-14 10:57:26 PDT
Trying again in r95099.