[webkit-reviews] review denied: [Bug 98450] Add support for blendmode to WebKit1 platform code : [Attachment 167862] Fixed a couple of typos
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Oct 9 16:49:30 PDT 2012
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Rik Cabanier
<cabanier at adobe.com>'s request for review:
Bug 98450: Add support for blendmode to WebKit1 platform code
https://bugs.webkit.org/show_bug.cgi?id=98450
Attachment 167862: Fixed a couple of typos
https://bugs.webkit.org/attachment.cgi?id=167862&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167862&action=review
> Source/WebCore/platform/graphics/GraphicsContext.cpp:595
> +#if ENABLE(CSS_COMPOSITING)
> + image->draw(this, styleColorSpace, FloatRect(dest.location(),
FloatSize(tw, th)), FloatRect(src.location(), FloatSize(tsw, tsh)), op,
useLowQualityScale, blendMode);
> +#else
> image->draw(this, styleColorSpace, FloatRect(dest.location(),
FloatSize(tw, th)), FloatRect(src.location(), FloatSize(tsw, tsh)), op,
useLowQualityScale);
> + UNUSED_PARAM(blendMode);
> +#endif
> setImageInterpolationQuality(previousInterpolationQuality);
> } else
> +#if ENABLE(CSS_COMPOSITING)
> + image->draw(this, styleColorSpace, FloatRect(dest.location(),
FloatSize(tw, th)), FloatRect(src.location(), FloatSize(tsw, tsh)), op,
useLowQualityScale, blendMode);
> +#else
> image->draw(this, styleColorSpace, FloatRect(dest.location(),
FloatSize(tw, th)), FloatRect(src.location(), FloatSize(tsw, tsh)), op,
useLowQualityScale);
> +#endif
Why not just pass the blendMode in? I see no advantage of using #if
ENABLE(CSS_COMPOSITING) down here; it should just be used to not expose new
functionality to CSS.
> Source/WebCore/platform/graphics/GraphicsContext.cpp:704
> +void GraphicsContext::setCompositeOperation(CompositeOperator
compositeOperation )
Extra space
> Source/WebCore/platform/graphics/GraphicsContext.h:555
> +#if ENABLE(CSS_COMPOSITING)
> + void setPlatformCompositeOperation(CompositeOperator, BlendMode =
BlendModeNormal);
> +#else
> void setPlatformCompositeOperation(CompositeOperator);
> +#endif
No need for the #ifdef.
> Source/WebCore/platform/graphics/GraphicsLayer.cpp:74
> +#if ENABLE(CSS_COMPOSITING)
> + , m_blendMode(BlendModeNormal)
> +#endif
Why is this in the CG patch? GraphicsLayers are just about accelerated
compositing.
> Source/WebCore/platform/graphics/GraphicsLayer.h:38
> #endif
> +#if ENABLE(CSS_COMPOSITING)
> +#include "GraphicsTypes.h"
> +#endif
This file shouldn't change at all.
> Source/WebCore/platform/graphics/ImageBuffer.h:134
> +#if ENABLE(CSS_COMPOSITING)
> + void draw(GraphicsContext*, ColorSpace, const FloatRect& destRect,
const FloatRect& srcRect = FloatRect(0, 0, -1, -1), CompositeOperator =
CompositeSourceOver, bool useLowQualityScale = false, BlendMode =
BlendModeNormal);
> +#else
> void draw(GraphicsContext*, ColorSpace, const FloatRect& destRect,
const FloatRect& srcRect = FloatRect(0, 0, -1, -1), CompositeOperator =
CompositeSourceOver, bool useLowQualityScale = false);
> +#endif
Dont' #ifdef.
> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:273
> +#if ENABLE(CSS_COMPOSITING)
> +void GraphicsContext::drawNativeImage(NativeImagePtr imagePtr, const
FloatSize& imageSize, ColorSpace styleColorSpace, const FloatRect& destRect,
const FloatRect& srcRect, CompositeOperator op, ImageOrientation orientation,
BlendMode blendMode)
> +#else
> void GraphicsContext::drawNativeImage(NativeImagePtr imagePtr, const
FloatSize& imageSize, ColorSpace styleColorSpace, const FloatRect& destRect,
const FloatRect& srcRect, CompositeOperator op, ImageOrientation orientation)
> +#endif
> {
Don't #ifdef. Also put the BlendMode after the CompositeOperator.
> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:336
> +#if ENABLE(CSS_COMPOSITING)
> + setPlatformCompositeOperation(op, blendMode);
> +#else
> setPlatformCompositeOperation(op);
> +#endif
>
Ditto.
> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1757
> +#if ENABLE(CSS_COMPOSITING)
> + if (blendMode != BlendModeNormal)
> + switch (blendMode) {
> + case BlendModeMultiply:
> + target = kCGBlendModeMultiply;
> + break;
> + case BlendModeScreen:
> + target = kCGBlendModeScreen;
> + break;
> + case BlendModeOverlay:
> + target = kCGBlendModeOverlay;
> + break;
> + case BlendModeDarken:
> + target = kCGBlendModeDarken;
> + break;
> + case BlendModeLighten:
> + target = kCGBlendModeLighten;
> + break;
> + case BlendModeColorDodge:
> + target = kCGBlendModeColorDodge;
> + break;
> + case BlendModeColorBurn:
> + target = kCGBlendModeColorBurn;
> + break;
> + case BlendModeHardLight:
> + target = kCGBlendModeHardLight;
> + break;
> + case BlendModeSoftLight:
> + target = kCGBlendModeSoftLight;
> + break;
> + case BlendModeDifference:
> + target = kCGBlendModeDifference;
> + break;
> + case BlendModeExclusion:
> + target = kCGBlendModeExclusion;
> + break;
> + case BlendModeHue:
> + target = kCGBlendModeHue;
> + break;
> + case BlendModeSaturation:
> + target = kCGBlendModeSaturation;
> + break;
> + case BlendModeColor:
> + target = kCGBlendModeColor;
> + break;
> + case BlendModeLuminosity:
> + target = kCGBlendModeLuminosity;
> + default:
> + break;
> + } else
> +#endif
Don't #ifdef.
> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:266
> +#if ENABLE(CSS_COMPOSITING)
> +void ImageBuffer::draw(GraphicsContext* destContext, ColorSpace
styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect,
CompositeOperator op, bool useLowQualityScale, BlendMode blendMode)
> +#else
> void ImageBuffer::draw(GraphicsContext* destContext, ColorSpace
styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect,
CompositeOperator op, bool useLowQualityScale)
> +#endif
Ditto.
> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:283
> +#if ENABLE(CSS_COMPOSITING)
> + destContext->drawNativeImage(image.get(), internalSize(), colorSpace,
destRect, adjustedSrcRect, op, DefaultImageOrientation, blendMode);
> +#else
> + destContext->drawNativeImage(image.get(), internalSize(), colorSpace,
destRect, adjustedSrcRect, op, DefaultImageOrientation);
> +#endif
Ditto.
> Source/WebCore/rendering/FilterEffectRenderer.cpp:476
> +void FilterEffectRenderer::createPassthroughFilter()
> +{
> +#if ENABLE(CSS_SHADERS)
> + m_hasCustomShaderFilter = false;
> +#endif
> +
> + RefPtr<FilterEffect> previousEffect = m_sourceGraphic;
> +
> + ComponentTransferFunction nullFunction;
> + ComponentTransferFunction transferFunction;
> + transferFunction.type = FECOMPONENTTRANSFER_TYPE_TABLE;
> + Vector<float> transferParameters;
> + transferParameters.append(0);
> + transferParameters.append(1.0f);
> + transferFunction.tableValues = transferParameters;
> +
> + RefPtr<FilterEffect> effect = FEComponentTransfer::create(this,
nullFunction, nullFunction, nullFunction, transferFunction);
> + effect->setClipsToBounds(false);
> + effect->setColorSpace(ColorSpaceDeviceRGB);
> + effect->inputEffects().append(previousEffect);
> +
> + m_effects.clear();
> + m_effects.append(effect);
> +
> + previousEffect = effect.release();
> +
> + setMaxEffectRects(m_sourceDrawingRegion);
> +}
> +
Not sure if this is related.
> Source/WebCore/rendering/FilterEffectRenderer.h:120
> + void createPassthroughFilter();
Unrelated?
> Source/WebCore/rendering/RenderLayer.cpp:1193
> - return renderer()->isTransparent() || renderer()->hasMask();
> + return renderer()->isTransparent() || renderer()->hasMask() ||
renderer()->hasBlendMode();
This method needs renaming. It's no longer just about opacity.
> Source/WebCore/rendering/RenderLayer.cpp:3171
> + FilterEffectRendererHelper filterPainter(filterRenderer() &&
(paintsWithFilters() || hasBlendMode()));
Doesn't paintsWithFilters() include blend mode?
> Source/WebCore/rendering/RenderLayer.h:618
> + bool hasBlendMode() const { return m_blendMode != BlendModeNormal; }
> + BlendMode blendMode() const { return m_blendMode; }
I don't see m_blendMode being updated in styleDidChange.
> LayoutTests/css3/compositing/effect-blend-mode-expected.txt:38
> +layer at (0,0) size 800x600
> + RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> + RenderBlock {HTML} at (0,0) size 800x600
> + RenderBody {BODY} at (8,8) size 784x584
> + RenderBlock {UL} at (0,0) size 784x0
> + RenderBlock (floating) {LI} at (45,5) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (185,5) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (325,5) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (465,5) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (605,5) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (45,145) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (185,145) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (325,145) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (465,145) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (605,145) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (45,285) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (185,285) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (325,285) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (465,285) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (605,285) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
> + RenderBlock (floating) {LI} at (45,425) size 130x130
> + RenderImage {IMG} at (0,0) size 130x130
This should be dumpAsText() test.
More information about the webkit-reviews
mailing list