Summary: | Make sure border-image sub-properties can be specified in any order. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||
Component: | CSS | Assignee: | Dave Hyatt <hyatt> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bdakin, ossy, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 68051, 68058 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Dave Hyatt
2011-09-13 16:18:05 PDT
Created attachment 107252 [details]
Patch
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 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 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? You should address Darin's comments, and the style bot's request for a bug number in the change log. (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. 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. 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.
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.
Created attachment 107352 [details]
Patch that passes style queue.
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 on attachment 107352 [details]
Patch that passes style queue.
Hyatt and I went over the changes in person.
Trying again in r95099. |