[webkit-reviews] review granted: [Bug 130892] [CSS Blending] Isolation descendant dependent flags are not updated correctly : [Attachment 228494] addressing Mihnea's comments 2

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Ion Rosca <rosca at adobe.com>'s request
for review:
Bug 130892: [CSS Blending] Isolation descendant dependent flags are not updated
correctly
https://bugs.webkit.org/show_bug.cgi?id=130892

Attachment 228494: addressing Mihnea's comments 2
https://bugs.webkit.org/attachment.cgi?id=228494&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=228494&action=review


Patch is ok, but the tests need a fair amount of cleanup.

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-append-non-stacking-con
text-blending.html:8
> +	.stacking-context {
> +		-webkit-isolation: isolate;
> +		isolation: isolate;
> +	}

Many of these test files are using 8 space indents.

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-append-non-stacking-con
text-blending.html:54
> +		<!-- <div class="append-root">
> +			<div class="blending leaf">
> +			</div>
> +		</div> -->

Did you mean to leave this in?

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-append-stacking-context
-blending.html:46
> +    function change() {
> +	var target = document.getElementById("target");
> +	var toAppend = document.createElement('div');
> +	var blendingElement = document.createElement('div');
> +	blendingElement.className = "blending leaf";
> +	toAppend.appendChild(blendingElement);
> +	toAppend.className = "stacking-context append-root"
> +	target.appendChild(toAppend);
> +
> +	   if (window.testRunner)
> +	       window.testRunner.notifyDone();
> +    }

Indentation is weird. Also, you're mixing ' and "

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-append-stacking-context
-blending.html:54
> +		<!-- <div class="stacking-context append-root">
> +			<div class="blending leaf">
> +			</div>
> +		</div> -->

Again, intentional?

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-remove-non-stacking-con
text-blending.html:41
> +    function change() {
> +	var toremove = document.getElementById("toremove");
> +	toremove.parentNode.removeChild(toremove);
> +
> +	   if (window.testRunner)
> +	       window.testRunner.notifyDone();
> +    }

More weirdness.

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-turn-off-blending-no-is
olation.html:36
> +		<div class="">

Please remove class="" (it's in a few places)

>
LayoutTests/css3/compositing/blend-mode-isolation-flags-turn-off-blending.html:
31
> +    function change() {
> +	var target = document.getElementById("target");
> +	target.className = "stacking-context";
> +	   if (window.testRunner)
> +	       window.testRunner.notifyDone();
> +    }

More weirdness.