Bug 104176 – Extend platform layer so it can pass blend modes to the compositing calls

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Rik Cabanier]

Canvas has a proposal to allow blending in the compositing operator.
The plaform code needs to be enhanced to support this. This bug will just extend the platform interface to allow this.

[Rik Cabanier]

Created attachment 177847 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 177847 [details]
Patch

Attachment 177847 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15170261

[Rik Cabanier]

Created attachment 177854 [details]
Patch

[Rik Cabanier]

Created attachment 177866 [details]
Patch

[WebKit Review Bot]

Attachment 177866 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 177866 [details]
Patch

Attachment 177866 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15173128

[Peter Beverloo (cr-android ews)]

Comment on attachment 177866 [details]
Patch

Attachment 177866 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15153648

[Rik Cabanier]

Created attachment 177908 [details]
Patch

[WebKit Review Bot]

Attachment 177908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/html/HTMLCanvasElement.cpp'..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

This patch updates interfaces so relevant calls also pass blend modes.
In addition, the blend mode is stored in the graphics context.

[Dirk Schulze]

Comment on attachment 177908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177908&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:409
>          void setCompositeOperation(CompositeOperator);
> +        void setCompositeOperation(CompositeOperator, BlendMode);

Does it make sense to combine both and use the default value BlendModeNormal like you did for setPlatformCompositeOperation ?

[Rik Cabanier]

Created attachment 177949 [details]
Patch

[WebKit Review Bot]

Attachment 177949 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 177908 [details]
Patch

Attachment 177908 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15154752

[Build Bot]

Comment on attachment 177949 [details]
Patch

Attachment 177949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15181029

[Stephen Chenney]

Comment on attachment 177949 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177949&action=review

> Source/WebCore/platform/graphics/GraphicsContext.cpp:475
> +{    if (paintingDisabled() || !image)

Missing a newline in here.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:548
> +    drawImageBuffer(image, styleColorSpace, p, IntRect(0, 0, -1, -1), op, blendMode);

I am not a fan of all of the methods in GraphicsContext named drawImageBuffer that chain together to finally end up at the one call with the FloatRect, FloatRect arguments. Can short-circuit all of these calls and go direct to the correct method? I'll probably take it on as a separate patch.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:553
> +    drawImageBuffer(image, styleColorSpace, r, IntRect(0, 0, -1, -1), op, blendMode, useLowQualityScale);

Ditto.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:558
> +    drawImageBuffer(image, styleColorSpace, IntRect(dest, srcRect.size()), srcRect, op, blendMode);

Ditto.

> Source/WebCore/platform/graphics/GraphicsContext.h:409
> +        void setCompositeOperation(CompositeOperator, BlendMode);

Why not add BlendMode as a default parameter to the existing method? It would be functionally the same and avoid further bloat to this class.

>> Source/WebKit/chromium/tests/DragImageTest.cpp:87
>> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Ignore this.

>> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99
>> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Ignore this.

[Rik Cabanier]

Created attachment 178089 [details]
Patch

[WebKit Review Bot]

Attachment 178089 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

(In reply to comment #11)
> (From update of attachment 177908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177908&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:409
> >          void setCompositeOperation(CompositeOperator);
> > +        void setCompositeOperation(CompositeOperator, BlendMode);
> 
> Does it make sense to combine both and use the default value BlendModeNormal like you did for setPlatformCompositeOperation ?

I did not combine them because there are dependencies between the different WebKit libraries and I wanted to minimize the changes.

[Rik Cabanier]

(In reply to comment #16)
> (From update of attachment 177949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177949&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:475
> > +{    if (paintingDisabled() || !image)
> 
> Missing a newline in here.

Fixed

> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:548
> > +    drawImageBuffer(image, styleColorSpace, p, IntRect(0, 0, -1, -1), op, blendMode);
> 
> I am not a fan of all of the methods in GraphicsContext named drawImageBuffer that chain together to finally end up at the one call with the FloatRect, FloatRect arguments. Can short-circuit all of these calls and go direct to the correct method? I'll probably take it on as a separate patch.

I agree. It's messy.
when you say "I'll take it on", do you mean you or me?

> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:553
> > +    drawImageBuffer(image, styleColorSpace, r, IntRect(0, 0, -1, -1), op, blendMode, useLowQualityScale);
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:558
> > +    drawImageBuffer(image, styleColorSpace, IntRect(dest, srcRect.size()), srcRect, op, blendMode);
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:409
> > +        void setCompositeOperation(CompositeOperator, BlendMode);
> 
> Why not add BlendMode as a default parameter to the existing method? It would be functionally the same and avoid further bloat to this class.
> 
> >> Source/WebKit/chromium/tests/DragImageTest.cpp:87
> >> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Ignore this.
> 
> >> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99
> >> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Ignore this.

[Rik Cabanier]

(In reply to comment #16)
> (From update of attachment 177949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177949&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:475
> > +{    if (paintingDisabled() || !image)
> 
> Missing a newline in here.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:548
> > +    drawImageBuffer(image, styleColorSpace, p, IntRect(0, 0, -1, -1), op, blendMode);
> 
> I am not a fan of all of the methods in GraphicsContext named drawImageBuffer that chain together to finally end up at the one call with the FloatRect, FloatRect arguments. Can short-circuit all of these calls and go direct to the correct method? I'll probably take it on as a separate patch.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:553
> > +    drawImageBuffer(image, styleColorSpace, r, IntRect(0, 0, -1, -1), op, blendMode, useLowQualityScale);
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:558
> > +    drawImageBuffer(image, styleColorSpace, IntRect(dest, srcRect.size()), srcRect, op, blendMode);
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:409
> > +        void setCompositeOperation(CompositeOperator, BlendMode);
> 
> Why not add BlendMode as a default parameter to the existing method? It would be functionally the same and avoid further bloat to this class.

Krit had the same remark.
If I change this, there will be a link issues between the libraries.
I can fix this in another patch. 

> 
> >> Source/WebKit/chromium/tests/DragImageTest.cpp:87
> >> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Ignore this.
> 
> >> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99
> >> +                      WebCore::CompositeOperator, WebCore::BlendMode)
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Ignore this.

[Rik Cabanier]

Created attachment 178099 [details]
Patch

[WebKit Review Bot]

Attachment 178099 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

(In reply to comment #20)
> (In reply to comment #16)
> > (From update of attachment 177949 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177949&action=review

> > > Source/WebCore/platform/graphics/GraphicsContext.cpp:548
> > > +    drawImageBuffer(image, styleColorSpace, p, IntRect(0, 0, -1, -1), op, blendMode);
> > 
> > I am not a fan of all of the methods in GraphicsContext named drawImageBuffer that chain together to finally end up at the one call with the FloatRect, FloatRect arguments. Can short-circuit all of these calls and go direct to the correct method? I'll probably take it on as a separate patch.
> 
> I agree. It's messy.
> when you say "I'll take it on", do you mean you or me?

I mean myself. https://bugs.webkit.org/show_bug.cgi?id=104367

[Dirk Schulze]

Comment on attachment 178099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178099&action=review

> Source/WebCore/platform/graphics/GraphicsContext.cpp:710
>  void GraphicsContext::setCompositeOperation(CompositeOperator compositeOperation)
>  {
> +    setCompositeOperation(compositeOperation, BlendModeNormal);
> +}

I don't get your point here actually. Which libraries do you mean? All files that include this functions can still use setCompositeOperation(compositeOperation)? Can you give an example please?

[Rik Cabanier]

Created attachment 178242 [details]
Patch

[WebKit Review Bot]

Attachment 178242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rik Cabanier]

(In reply to comment #25)
> (From update of attachment 178099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178099&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.cpp:710
> >  void GraphicsContext::setCompositeOperation(CompositeOperator compositeOperation)
> >  {
> > +    setCompositeOperation(compositeOperation, BlendModeNormal);
> > +}
> 
> I don't get your point here actually. Which libraries do you mean? All files that include this functions can still use setCompositeOperation(compositeOperation)? Can you give an example please?

I changed the .def.in file that defines the exported functions. This fixed the problem.

[Dirk Schulze]

Comment on attachment 178242 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178242&action=review

Can you just upload a new patch with a better change log?

> Source/WebCore/ChangeLog:8
> +        Interface changes for the new blend modes

Can you provide more details here? The changelog helps other people to understand what exaclty you did, w/o looking at the code.

[Rik Cabanier]

Created attachment 178266 [details]
Patch

[WebKit Review Bot]

Attachment 178266 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 178266 [details]
Patch

Clearing flags on attachment: 178266

Committed r136993: <http://trac.webkit.org/changeset/136993>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104415

[Rik Cabanier]

Created attachment 178304 [details]
Added missing exports file for mac

[WebKit Review Bot]

Attachment 178304 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/tests/DragImageTest.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 178304 [details]
Added missing exports file for mac

Lets try it agin. r=me.

[WebKit Review Bot]

Comment on attachment 178304 [details]
Added missing exports file for mac

Clearing flags on attachment: 178304

Committed r137011: <http://trac.webkit.org/changeset/137011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.