Bug 132600

Summary:

[CSS Blending] Cleanup tests in css3/blending

Product:

WebKit

Reporter:

Ion Rosca <rosca>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bunhere, cdumez, commit-queue, gyuyoung.kim, sergio

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

95614

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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