[webkit-reviews] review denied: [Bug 132600] [CSS Blending] Cleanup tests in css3/blending : [Attachment 234243] Patch

Thu Jul 3 00:03:54 PDT 2014

Mihnea Ovidenie <mihnea at adobe.com> has denied Ion Rosca <rosca at adobe.com>'s
request for review:
Bug 132600: [CSS Blending] Cleanup tests in css3/blending
https://bugs.webkit.org/show_bug.cgi?id=132600

Attachment 234243: Patch
https://bugs.webkit.org/attachment.cgi?id=234243&action=review

------- Additional Comments from Mihnea Ovidenie <mihnea at adobe.com>
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-ex
pected.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-exp
ected.html:7
> +	       background: #777777;

Same here .gray7

>
LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box-exp
ected.html:8
> +	       margin: 0 0 9px;

This is overwritten by the mask definition below.

>
LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box-exp
ected.html:9
> +	       background: #777777;

gray7

>
LayoutTests/css3/blending/background-blend-mode-background-clip-padding-box.htm
l:8
> +	       margin: 0 0 9px;

Same here, you have a mask definition below.

>
LayoutTests/css3/blending/background-blend-mode-background-origin-border-box-ex
pected.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-expecte
d.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-contex
t-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.ht
ml: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?