| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 95614 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ion Rosca
2014-05-05 22:55:23 PDT
Created attachment 234243 [details]
Patch
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? Created attachment 235208 [details]
Patch
Created attachment 235209 [details]
Patch
Created attachment 235210 [details]
Patch
Comment on attachment 235210 [details]
Patch
r=me
Comment on attachment 235210 [details] Patch Clearing flags on attachment: 235210 Committed r171295: <http://trac.webkit.org/changeset/171295> All reviewed patches have been landed. Closing bug. |