[Webkit-unassigned] [Bug 132600] [CSS Blending] Cleanup tests in css3/blending

Thu Jul 3 00:03:57 PDT 2014

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

Mihnea Ovidenie <mihnea at adobe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234243|review?                     |review-
               Flag|                            |

--- Comment #2 from Mihnea Ovidenie <mihnea at adobe.com>  2014-07-03 00:04:13 PST ---
(From update of attachment 234243)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.