Bug 98450

Summary: Add support for blendmode to WebKit1 platform code
Product: WebKit Reporter: Rik Cabanier <cabanier>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dglazkov, dino, donggwan.kim, eric, gtk-ews, gustavo, noam, peter+ews, philn, simon.fraser, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 98950    
Bug Blocks: 95614    
Attachments:
Description Flags
just seeing what the bots say. not ready for checkin yet
webkit-ews: commit-queue-
made more code protected by flags. not ready
gtk-ews: commit-queue-
more conditional code. Not ready for review
gyuyoung.kim: commit-queue-
fixes for QT & Chromium. Not ready for review
gyuyoung.kim: commit-queue-
this is the actual patch. Previous one was old. not for review
webkit-ews: commit-queue-
fix for strange webkit style problem
none
silly QT typo
buildbot: commit-queue-
Type in non-compositing code
none
previous patch failed to apply
buildbot: commit-queue-
added test case and changelog
none
Fixed a couple of typos simon.fraser: review-

Description Rik Cabanier 2012-10-04 13:50:58 PDT
Bug 95614 will add support for WebKit2 code.
This patch will add it to the WebKit1 codebase.
Comment 1 Rik Cabanier 2012-10-08 13:13:34 PDT
Created attachment 167593 [details]
just seeing what the bots say. not ready for checkin yet
Comment 2 Early Warning System Bot 2012-10-08 13:25:31 PDT
Comment on attachment 167593 [details]
just seeing what the bots say. not ready for checkin yet

Attachment 167593 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14209584
Comment 3 Early Warning System Bot 2012-10-08 13:29:29 PDT
Comment on attachment 167593 [details]
just seeing what the bots say. not ready for checkin yet

Attachment 167593 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14211484
Comment 4 Gyuyoung Kim 2012-10-08 13:34:18 PDT
Comment on attachment 167593 [details]
just seeing what the bots say. not ready for checkin yet

Attachment 167593 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14209588
Comment 5 Build Bot 2012-10-08 13:52:16 PDT
Comment on attachment 167593 [details]
just seeing what the bots say. not ready for checkin yet

Attachment 167593 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14205768
Comment 6 Rik Cabanier 2012-10-08 13:59:19 PDT
Created attachment 167602 [details]
made more code protected by flags. not ready
Comment 7 kov's GTK+ EWS bot 2012-10-08 14:07:59 PDT
Comment on attachment 167602 [details]
made more code protected by flags. not ready

Attachment 167602 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14215450
Comment 8 Gyuyoung Kim 2012-10-08 14:13:38 PDT
Comment on attachment 167602 [details]
made more code protected by flags. not ready

Attachment 167602 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14204787
Comment 9 Rik Cabanier 2012-10-08 14:52:51 PDT
Created attachment 167622 [details]
more conditional code. Not ready for review
Comment 10 Gyuyoung Kim 2012-10-08 15:04:43 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14215470
Comment 11 Early Warning System Bot 2012-10-08 15:10:28 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14207644
Comment 12 Early Warning System Bot 2012-10-08 15:18:21 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14204818
Comment 13 WebKit Review Bot 2012-10-08 15:31:01 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14228366
Comment 14 Peter Beverloo (cr-android ews) 2012-10-08 16:08:05 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14229287
Comment 15 Build Bot 2012-10-08 16:50:34 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14229301
Comment 16 Build Bot 2012-10-08 19:09:45 PDT
Comment on attachment 167622 [details]
more conditional code. Not ready for review

Attachment 167622 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14229368

New failing tests:
css3/compositing/should-have-compositing-layer.html
Comment 17 Rik Cabanier 2012-10-09 01:14:13 PDT
Created attachment 167705 [details]
fixes for QT & Chromium. Not ready for review
Comment 18 Gyuyoung Kim 2012-10-09 01:23:44 PDT
Comment on attachment 167705 [details]
fixes for QT & Chromium. Not ready for review

Attachment 167705 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14209803
Comment 19 Early Warning System Bot 2012-10-09 01:24:29 PDT
Comment on attachment 167705 [details]
fixes for QT & Chromium. Not ready for review

Attachment 167705 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14238195
Comment 20 Early Warning System Bot 2012-10-09 01:26:30 PDT
Comment on attachment 167705 [details]
fixes for QT & Chromium. Not ready for review

Attachment 167705 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14207815
Comment 21 Rik Cabanier 2012-10-09 01:27:40 PDT
Created attachment 167709 [details]
this is the actual patch. Previous one was old. not for review
Comment 22 WebKit Review Bot 2012-10-09 01:30:00 PDT
Attachment 167709 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/GraphicsC..." exit_code: 1
Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Rik Cabanier 2012-10-09 01:43:44 PDT
Created attachment 167710 [details]
fix for strange webkit style problem
Comment 24 Early Warning System Bot 2012-10-09 01:57:12 PDT
Comment on attachment 167709 [details]
this is the actual patch. Previous one was old. not for review

Attachment 167709 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14238207
Comment 25 Early Warning System Bot 2012-10-09 01:58:52 PDT
Comment on attachment 167709 [details]
this is the actual patch. Previous one was old. not for review

Attachment 167709 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14228553
Comment 26 Rik Cabanier 2012-10-09 02:04:10 PDT
Created attachment 167712 [details]
silly QT typo
Comment 27 Build Bot 2012-10-09 02:12:34 PDT
Comment on attachment 167709 [details]
this is the actual patch. Previous one was old. not for review

Attachment 167709 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14229470
Comment 28 Build Bot 2012-10-09 02:43:31 PDT
Comment on attachment 167712 [details]
silly QT typo

Attachment 167712 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14245018
Comment 29 Build Bot 2012-10-09 03:20:56 PDT
Comment on attachment 167709 [details]
this is the actual patch. Previous one was old. not for review

Attachment 167709 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14223633

New failing tests:
css3/compositing/should-have-compositing-layer.html
Comment 30 Build Bot 2012-10-09 03:56:19 PDT
Comment on attachment 167712 [details]
silly QT typo

Attachment 167712 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14214692

New failing tests:
css3/compositing/should-have-compositing-layer.html
Comment 31 Rik Cabanier 2012-10-09 09:37:37 PDT
Created attachment 167774 [details]
Type in non-compositing code
Comment 32 Rik Cabanier 2012-10-09 11:19:47 PDT
Created attachment 167794 [details]
previous patch failed to apply
Comment 33 Rik Cabanier 2012-10-09 14:30:48 PDT
Created attachment 167848 [details]
added test case and changelog
Comment 34 Rik Cabanier 2012-10-09 15:31:40 PDT
Created attachment 167862 [details]
Fixed a couple of typos
Comment 35 Simon Fraser (smfr) 2012-10-09 16:50:07 PDT
Comment on attachment 167862 [details]
Fixed a couple of typos

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.
Comment 36 Build Bot 2012-10-09 20:38:08 PDT
Comment on attachment 167794 [details]
previous patch failed to apply

Attachment 167794 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14228900

New failing tests:
css3/compositing/should-have-compositing-layer.html