Currently, shadows are affected by all transformations except scaling. The correct behavior is not being affected by any kind of transformation, and keep the same offset in relation to the shape/image. NOTE: this is the correct behavior when operating on a canvas, but not with box shadows (where offsets must follow the transformation matrix).
(In reply to comment #2)
> (From update of attachment 75432[details])
> Why does this qt-only change affect shared layout test results?
I'm assuming you're referring to canvas-strokePath. I wrote that test very recently. I'm changing it slightly now it by using a lineWidth > 1 to make it simultaneously easier to test and more fair among all ports, since we can be dealing with different transformation smoothness/aliasing settings in the different ports.
Comment on attachment 76006[details]
Refactored, more comments, added new test
View in context: https://bugs.webkit.org/attachment.cgi?id=76006&action=review> WebCore/platform/graphics/ContextShadow.cpp:165
> + QTransform transform = p->transform();
Use const ref.
> WebCore/platform/graphics/ContextShadow.cpp:167
> + QPolygonF transDestPolygon = transform.map(QPolygonF(m_layerArea));
transformedPolygon? I don't think Dest is necessary here (there is no Source and Dest per se, for transformation)
> WebCore/platform/graphics/ContextShadow.h:110
> + bool shadowsIgnoreTransforms() { return m_shadowsIgnoreTransforms; }
Make it a const.
> WebCore/platform/graphics/ContextShadow.h:127
> + // Enclosing int rect where shadow needs to be drawn to using the layer context.
> IntRect m_layerRect;
> + // Same as m_layerRec but has full precision (useful for lossless transformations).
> + FloatRect m_layerFloatRect;
> + // Rect occupied by the original shape or image.
> + FloatRect m_layerArea;
> + // Buffer to where the temporary shadow will be drawn to.
> PlatformImage m_layerImage;
Instead of m_layerArea, m_layerFloatRect and m_layerInflation, I propose using m_layerOrigin (top left corner for endShadowLayer) and m_sourceRect (for the scratch image). These variables are potentially easier to understand.
Comment on attachment 76155[details]
Consts, refactoring, separate ports calc function for now
View in context: https://bugs.webkit.org/attachment.cgi?id=76155&action=review> LayoutTests/ChangeLog:5
> + [Qt] No shadow offset should be affected by any transformation
I suggest simplifying this to "Canvas shadow offset should not be affected..." (and the bug title as well)
> LayoutTests/fast/canvas/canvas-transforms-fillRect-shadow-expected.txt:1
> +Ensure correct behavior of canvas with fillRect+shadow after translation+rotation+scaling. A blue and red checkered pattern should be displayed.
Isn't the box solid and not patterned?
> WebCore/platform/graphics/ContextShadow.cpp:159
> + int inflation = 0;
Is it possible to make this 'float' instead? Beside removing confusion (enlarging float rect with int), we are prepared for possible further enhancement when the blur function supports non-integer blur radius.
> WebCore/platform/graphics/qt/ContextShadowQt.cpp:140
> + // Set the origin as the top left corner of the scratch image.
> + if (p->transform().isIdentity()) {
> + m_layerContext->translate(offset());
> + m_layerContext->translate(-layerRect.x(), -layerRect.y());
> + } else {
> + const int frameSize = (m_sourceRect.width() - layerArea.width()) / 2;
> + m_layerContext->translate(-layerArea.x() + frameSize, -layerArea.y() + frameSize);
> + }
Can we get away with the specialization of the identity transform? I can't see why it's different.
(In reply to comment #14)
> Isn't the box solid and not patterned?
"Pattern" does not refer to a canvas pattern brush, but rather to a pattern in the literal meaning of the word (i.e. the test displays a pattern). I've used the very same sentence in all my canvas tests, as all of them result in the same pattern.
> Is it possible to make this 'float' instead?
Absolutely, this is something I forgot to change.
> Can we get away with the specialization of the identity transform? I can't see why it's different.
Again, absolutely, I also forgot to change that before uploading the last patch.
I'll upload a new version shortly.
http://trac.webkit.org/changeset/74040 might have broken Qt Linux Release
The following tests are not passing:
canvas/philip/tests/2d.shadow.canvas.transparent.2.html
canvas/philip/tests/2d.shadow.image.transparent.2.html
Apparently this commit broke these tests in the gtk bots:
fast/canvas/canvas-scale-fillPath-shadow.html -> failed
fast/canvas/canvas-scale-fillRect-shadow.html -> failed
fast/canvas/canvas-scale-strokePath-shadow.html -> failed
fast/canvas/canvas-transforms-fillRect-shadow.html -> failed
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r74042%20(17079)/results.html
I've checked the results and not sure if it is a problem with the patch or it could be the same issue we have in bug 49964.
(In reply to comment #24)
> Apparently this commit broke these tests in the gtk bots:
I've reverted this patch as it breaks 2 Qt tests, but I ran the GTK tests and they work fine. It's very unlikely that they're failing because of the patch. I can double check later.
(In reply to comment #23)
> http://trac.webkit.org/changeset/74040 might have broken Qt Linux Release
> The following tests are not passing:
> canvas/philip/tests/2d.shadow.canvas.transparent.2.html
> canvas/philip/tests/2d.shadow.image.transparent.2.html
I found out this is a clipping issue introduced by the patch when we take transformations into account. Essentially, the origin of m_layerContext must be adjusted again in case the shadow is partially clipped out. I have a patch that fixes it, will upload shortly.
Comment on attachment 76620[details]
Add changes in comment #27 fixing failing tests
View in context: https://bugs.webkit.org/attachment.cgi?id=76620&action=review> WebCore/platform/graphics/qt/ContextShadowQt.cpp:139
> + const FloatSize clippedOffset = m_unclippedLayerOrigin - m_layerOrigin;
> + const float translationX = -layerArea.x() + frameSize - abs(clippedOffset.width());
> + const float translationY = -layerArea.y() + frameSize - abs(clippedOffset.height());
> + m_layerContext->translate(translationX, translationY);
Having both m_unclippedLayerOrigin and m_layerOrigin will be confusing. In particular since we never need the former.
Here is an alternative: precompute the translation (as a point) in calculateLayerBoundingRect and save it and the use it later here.
Comment on attachment 76635[details]
Add suggestions from comment #29
View in context: https://bugs.webkit.org/attachment.cgi?id=76635&action=review> WebCore/platform/graphics/qt/ContextShadowQt.cpp:135
> + // Set the origin as the top left corner of the scratch image, or, in case there's a clipped
> + // out region, set the origin accordingly to the full bounding rect's top-left corner.
> + m_layerContext->translate(m_layerContextTranslation);
The comment is confusing if it is paced here. Maybe in calculateLayerBoundingRect?
(In reply to comment #25)
> (In reply to comment #24)
> > Apparently this commit broke these tests in the gtk bots:
>
> I've reverted this patch as it breaks 2 Qt tests, but I ran the GTK tests and they work fine. It's very unlikely that they're failing because of the patch. I can double check later.
This patch has broken GTK+ bots, as was warned in comment #24.
(In reply to comment #38)
> Doesn't break GTK when opening tests in launcher
So when I test using the launcher, all is fine. Using DRT, OTOH, fails miserably. Could it be that the launcher is using X11, and DRT is using Image (or whatever Cairo's raster backend is called)?
(In reply to comment #45)
> Created an attachment (id=76913) [details]
> Cross-platform version of the previous patch
Martin, you need to upload the patch manually, you have the same issue I ran into yesterday: Git is considering that the new transforms test is a rename from one of your the platform scale expected files. There should be 3 new files and 3 deleted files. Just do a git show of the commit to a file and upload manually.
Comment on attachment 76918[details]
Entire patch
Rejecting attachment 76918[details] from commit-queue.
Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76918]" exit_code: 2
Last 500 characters of output:
raphicsContextQt.cpp
Hunk #1 FAILED at 53.
Hunk #2 FAILED at 510.
Hunk #3 FAILED at 548.
Hunk #4 succeeded at 664 with fuzz 2 (offset 106 lines).
Hunk #5 FAILED at 790.
Hunk #6 FAILED at 798.
Hunk #7 FAILED at 822.
Hunk #8 FAILED at 846.
Hunk #9 FAILED at 1026.
8 out of 9 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsContextQt.cpp.rej
Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ariya Hidayat', u'--force']" exit_code: 1
Full output: http://queues.webkit.org/results/7220034
(In reply to comment #53)
> Committed r74317: <http://trac.webkit.org/changeset/74317>
The patch that was landed is not the same. The gtk platform expected results files were not deleted, and the new canvas-transforms-shadow test is not there either.
2010-12-02 16:53 PST, Helder Correia
2010-12-02 19:22 PST, Helder Correia
2010-12-03 01:13 PST, Helder Correia
2010-12-03 02:22 PST, Helder Correia
2010-12-08 19:51 PST, Helder Correia
2010-12-09 18:39 PST, Helder Correia
2010-12-13 13:29 PST, Helder Correia
2010-12-14 00:31 PST, Helder Correia
2010-12-14 20:10 PST, Helder Correia
2010-12-15 00:07 PST, Helder Correia
2010-12-15 14:33 PST, Helder Correia
2010-12-16 12:54 PST, Helder Correia
2010-12-16 16:21 PST, Helder Correia
2010-12-16 16:26 PST, Helder Correia
2010-12-16 17:04 PST, Helder Correia
2010-12-16 17:38 PST, Helder Correia
2010-12-17 14:41 PST, Martin Robinson
2010-12-17 15:20 PST, Martin Robinson
2010-12-17 15:24 PST, Martin Robinson
2010-12-17 15:30 PST, Martin Robinson
commit-queue: commit-queue-