|
Description
Helder Correia
2010-12-02 16:47:10 PST
Created attachment 75432 [details]
Patch
Comment on attachment 75432 [details]
Patch
Why does this qt-only change affect shared layout test results?
(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. Created attachment 75453 [details]
Patch
Comment on attachment 75453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75453&action=review > WebCore/platform/graphics/qt/ContextShadowQt.cpp:31 > +#include <QDebug> Is it needed? Comment on attachment 75453 [details]
Patch
QDebug include is just a leftover. Will clean that up. Also need to resolve rebase conflicts.
Created attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers
Comment on attachment 75472 [details]
Resolve conflicts and remove QDebug leftovers
Need to fix the build issues for Cairo.
Created attachment 75481 [details]
Don't break Cairo port build
Comment on attachment 75481 [details]
Don't break Cairo port build
Removing R? since I found a case where the patch needs to be fixed.
Created attachment 76006 [details]
Refactored, more comments, added new test
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. Created attachment 76155 [details]
Consts, refactoring, separate ports calc function for now
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. Created attachment 76425 [details] Add suggestions in comment #14 Comment on attachment 76425 [details] Add suggestions in comment #14 LGTM. Comment on attachment 76425 [details] Add suggestions in comment #14 Rejecting attachment 76425 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--non-interactive', 76425]" exit_code: 2 Last 500 characters of output: (offset -3 lines). Hunk #6 succeeded at 700 (offset -3 lines). Hunk #7 succeeded at 708 (offset -3 lines). Hunk #8 succeeded at 732 (offset -3 lines). Hunk #9 succeeded at 756 (offset -3 lines). Hunk #10 succeeded at 954 with fuzz 2 (offset -3 lines). 2 out of 10 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsContextQt.cpp.rej Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Ariya Hidayat', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7056001 Created attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict
Comment on attachment 76509 [details]
Resolve "m_common->state" -> "m_state" conflict
LGTM.
Comment on attachment 76509 [details] Resolve "m_common->state" -> "m_state" conflict Clearing flags on attachment: 76509 Committed r74040: <http://trac.webkit.org/changeset/74040> All reviewed patches have been landed. Closing bug. 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. Rolled out. (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. Created attachment 76620 [details] Add changes in comment #27 fixing failing tests 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. Created attachment 76635 [details] Add suggestions from comment #29 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? Created attachment 76687 [details] Add suggestion from comment #31 Comment on attachment 76687 [details] Add suggestion from comment #31 LGTM. Comment on attachment 76687 [details] Add suggestion from comment #31 Clearing flags on attachment: 76687 Committed r74154: <http://trac.webkit.org/changeset/74154> All reviewed patches have been landed. Closing bug. (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. cq wouldn't allow me to revert the patch, so I rolled it out manually in r74187. Created attachment 76804 [details]
Doesn't break GTK when opening tests in launcher
(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)? Created attachment 76825 [details]
Remove GTK specific expected results files, tests pass now
Created attachment 76826 [details]
Remove GTK specific expected results files, tests pass now
Created attachment 76831 [details]
Upload manually, git would rename files no matter what
Patch was rolled out. Created attachment 76835 [details]
Once more, to force the bots to work
Created attachment 76913 [details]
Cross-platform version of the previous patch
(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. Attachment 76913 [details] did not build on qt: Build output: http://queues.webkit.org/results/7216027 Created attachment 76915 [details]
Cross platform version of the previous patch (#2)
Okay. Here's the output of 'git diff'
Created attachment 76917 [details]
Cross platform version of the previous patch (#3)
Uploading a new patch that builds on Qt.
Created attachment 76918 [details]
Entire patch
Comment on attachment 76915 [details] Cross platform version of the previous patch (#2) Cleared Martin Robinson's review+ from obsolete attachment 76915 [details] so that this bug does not appear in http://webkit.org/pending-commit. 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 Committed r74317: <http://trac.webkit.org/changeset/74317> (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. Seems to be fine. |