132600 – [CSS Blending] Cleanup tests in css3/blending

RESOLVED FIXED 132600

[CSS Blending] Cleanup tests in css3/blending

https://bugs.webkit.org/show_bug.cgi?id=132600

Summary [CSS Blending] Cleanup tests in css3/blending

Ion Rosca

Reported 2014-05-05 22:55:23 PDT

Cleanup tests in css3/blending

Attachments
Patch (142.86 KB, patch) 2014-07-02 00:28 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Patch (156.04 KB, patch) 2014-07-20 23:47 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Patch (155.99 KB, patch) 2014-07-20 23:53 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Patch (155.76 KB, patch) 2014-07-20 23:56 PDT, Ion Rosca	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ion Rosca

Comment 1 2014-07-02 00:28:27 PDT

Created attachment 234243 [details] Patch

Mihnea Ovidenie

Comment 2 2014-07-03 00:04:13 PDT

Comment on attachment 234243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234243&action=review Thanks for taking a look at this one! In general it looks good but it needs another round of small tweaks. > LayoutTests/ChangeLog:7 > + Please add here the summary if changes instead of adding then below the modified files, something like: "Summary of changes: - move .... " I would use the normal verb form instead of "ing" form - move instead of moving, replace instead of replacing etc I would also mention that since Accelerated compositing is always enabled, there is no need to overwrite the preferences anymore. > LayoutTests/ChangeLog:12 > + - removing the 'html' tag for consistency from all tests. Why not keep the 'html' tag in all tests for consistency? I prefer the tests to have: <!DOCTYPE html> <html> > LayoutTests/ChangeLog:13 > + - other minor adjustments. If they are minor lets not mention them. > LayoutTests/ChangeLog:14 > + - changes should not change what tests do. I do not understand what the above sentence means. > LayoutTests/css3/blending/background-blend-mode-background-attachement-fixed-expected.html:7 > + background: #777777; I believe you can use .gray7 instead of the background definition. > LayoutTests/css3/blending/background-blend-mode-background-clip-content-box-expected.html:7 > + background: #777777; Same here .gray7 > LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box-expected.html:8 > + margin: 0 0 9px; This is overwritten by the mask definition below. > LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box-expected.html:9 > + background: #777777; gray7 > LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box.html:8 > + margin: 0 0 9px; Same here, you have a mask definition below. > LayoutTests/css3/blending/background-blend-mode-background-origin-border-box-expected.html:9 > + background: #777777; gray7 > LayoutTests/css3/blending/background-blend-mode-background-position-percentage-expected.html:7 > + background-color: #777777; gray7 > LayoutTests/css3/blending/background-blend-mode-background-size-contain-expected.html:7 > + background: #777777; gray7 > LayoutTests/css3/blending/background-blend-mode-background-size-cover-expected.html:7 > + background: #777777; gray7 > LayoutTests/css3/blending/background-blend-mode-different-image-formats.html:10 > + background: url('resources/ducky.png') no-repeat 0 0 /100% 100%, green; This rule is also used below, so it should also be in the common css file. > LayoutTests/css3/blending/background-blend-mode-svg-color.html:5 > + width: 130px; box130 > LayoutTests/css3/blending/blend-mode-isolation-flags-append-non-stacking-context-blending.html:10 > + .leaf { Any reason why leaf and append-root are not in the common css file? > LayoutTests/css3/blending/blend-mode-isolation-turn-off-self-painting-layer1.html:7 > + window.testRunner.overridePreference("WebKitAcceleratedCompositingEnabled", "1"); I think you do not need this as accelerated compositing is always enabled. > LayoutTests/css3/blending/blend-mode-overflow.html:67 > + <div class="accelerated blendingbg"> Where is blendingbg defined? > LayoutTests/css3/blending/resources/blending-style.css:1 > +/* blending */ Another css rule used a lot is margin: 10px, maybe you can also add it in this file. Another css rules used is: background: url('resources/white_square.svg'), #777777; > LayoutTests/css3/blending/resources/blending-style.css:18 > +.bgmultiply { If you use bgMultiply instead of bgmultiply you will make it easier to read etc. Same below. > LayoutTests/css3/blending/resources/blending-style.css:26 > +.composited, .accelerated { Why 2 classes here?

Ion Rosca

Comment 3 2014-07-20 23:47:42 PDT

Created attachment 235208 [details] Patch

Ion Rosca

Comment 4 2014-07-20 23:53:07 PDT

Created attachment 235209 [details] Patch

Ion Rosca

Comment 5 2014-07-20 23:56:45 PDT

Created attachment 235210 [details] Patch

Mihnea Ovidenie

Comment 6 2014-07-21 00:32:15 PDT

Comment on attachment 235210 [details] Patch r=me

WebKit Commit Bot

Comment 7 2014-07-21 01:26:58 PDT

Comment on attachment 235210 [details] Patch Clearing flags on attachment: 235210 Committed r171295: <http://trac.webkit.org/changeset/171295>

WebKit Commit Bot

Comment 8 2014-07-21 01:27:03 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Nobody

Reported

2014-05-05 22:55 PDT

Modified

2014-07-21 01:27 PDT History

CC List

5 users Show

URL

Keywords

Depends on

Blocks

95614

Dependencies

tree graph