Bug 40376

Created attachment 58273 [details] Adding a handleGeometryChange() to correct this problem.

Created attachment 58555 [details] Proposed patch with proper coding style

Comment on attachment 58555 [details] Proposed patch with proper coding style I don't know about problem, but this patch won't be accepted because it doesn't apply (cq-) and have multiple coding style issues (r-). You can use WebKitTools/Scripts/check-coding-style script to find some coding style problems. Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck! > + No new tests. (OOPS!) It is possible to add a new autotest? > + if (QGraphicsScene *s = scene()) { '*' is in a wrong place. > - m_backingStoreKey = QPixmapCache::insert(pixmap); > + m_backingStoreKey = QPixmapCache::insert(pixmap); Spaces at the end of the line > @@ -398,7 +410,10 @@ void GraphicsLayerQtImpl::updateTransform() > if (!inverseOk) > return; > > - setTransform(transform2D); > + if ( transform2D != transform() ) { > + handleGeometryChange(); > + setTransform(transform2D); > + } Wrong intendation > @@ -537,20 +552,23 @@ void GraphicsLayerQtImpl::flushChanges(bool recursive, bool forceUpdateTransform > > if (m_changeMask & SizeChange) { > if (m_layer->size() != m_state.size) { > - prepareGeometryChange(); > + handleGeometryChange(); Wrong indentation > +#if 0 > // FIXME: this is a hack, due to a probable QGraphicsScene bug when rapidly modifying the perspective > // but without this line we get graphic artifacts > if ((m_changeMask & ChildrenTransformChange) && m_state.childrenTransform != m_layer->childrenTransform()) > if (scene()) > scene()->update(); > +#endif If you believe that this code in unneeded you should remove it. > + // handleGeometryChange(); It is not a proper comment. > if (!recursive) > - return; > + return; Spaces at the end of line. > - } > + } Spaces at the end of line. > +#if 1 > // FIXME: this is a hack, due to a probable QGraphicsScene bug. > // Without this the opacity change doesn't always have immediate effect. > if (m_layer.data()->scene() && !m_layer.data()->opacity() && opacity) > m_layer.data()->scene()->update(); > +#endif #if 1 is not pointless as 1 == true, so this code will be always included.

(In reply to comment #3) > #if 1 is not pointless as 1 == true, so this code will be always included. Oops, I mean that you can remove the #if 1 :-)

Please provide a test case fro the bug

(In reply to comment #3) > (From update of attachment 58555 [details]) > I don't know about problem, but this patch won't be accepted because it doesn't apply (cq-) and have multiple coding style issues (r-). You can use WebKitTools/Scripts/check-coding-style script to find some coding style problems. > > Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck! > > > + No new tests. (OOPS!) > It is possible to add a new autotest? > I don't have an autotest, it was clearly a corner case that had the pixels left uncovered. I very much doubt I'll get a case to cover it - but can try later. Can you point me to a autotest that does pixel comparisons? > > + if (QGraphicsScene *s = scene()) { > '*' is in a wrong place. > For whatever its worth check-webkit-style doesn't flag it, thus I didn't spot it. Fixed. > > - m_backingStoreKey = QPixmapCache::insert(pixmap); > > + m_backingStoreKey = QPixmapCache::insert(pixmap); > Spaces at the end of the line > Why do you want spaces at the end of the line? > > @@ -398,7 +410,10 @@ void GraphicsLayerQtImpl::updateTransform() > > if (!inverseOk) > > return; > > > > - setTransform(transform2D); > > + if ( transform2D != transform() ) { > > + handleGeometryChange(); > > + setTransform(transform2D); > > + } > Wrong intendation > Fixed, check-webkit-style didn't find this either. > > @@ -537,20 +552,23 @@ void GraphicsLayerQtImpl::flushChanges(bool recursive, bool forceUpdateTransform > > > > if (m_changeMask & SizeChange) { > > if (m_layer->size() != m_state.size) { > > - prepareGeometryChange(); > > + handleGeometryChange(); > Wrong indentation > Thanks. > > +#if 0 > > // FIXME: this is a hack, due to a probable QGraphicsScene bug when rapidly modifying the perspective > > // but without this line we get graphic artifacts > > if ((m_changeMask & ChildrenTransformChange) && m_state.childrenTransform != m_layer->childrenTransform()) > > if (scene()) > > scene()->update(); > > +#endif > If you believe that this code in unneeded you should remove it. > This was for No'am to experiment with as I think my fix removes the need for the above "hack" as they are solving a similar problem. > > + // handleGeometryChange(); > It is not a proper comment. > In this case it isn't needed, but the review seems scattershot. I can't use #if 0 to leave in code that will still need to be experimented with, I can't use c comments to leave in code that warrants further examination. I see other places in the code base where a line of code is commented out, this doesn't seem to be a real issue. > > if (!recursive) > > - return; > > + return; > Spaces at the end of line. > > > - } > > + } > Spaces at the end of line. > Again, I don't see why you want the spaces at the end of the line. > > +#if 1 > > // FIXME: this is a hack, due to a probable QGraphicsScene bug. > > // Without this the opacity change doesn't always have immediate effect. > > if (m_layer.data()->scene() && !m_layer.data()->opacity() && opacity) > > m_layer.data()->scene()->update(); > > +#endif > #if 1 is not pointless as 1 == true, so this code will be always included. Agree the #if 1 here is unnecessary - its a piece of code I never triggered so I left in No'am's "hack" to work around the issue since I couldn't confirm whether it was still necessary. My hope was that No'am would test if it was still necessary in the test that required it and possibly remove it.

Created attachment 58802 [details] Proposed patch with proper coding style and commented on the test case in the changelog

(In reply to comment #6) > (In reply to comment #3) > > (From update of attachment 58555 [details] [details]) > > Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck! > > > > > + No new tests. (OOPS!) > > It is possible to add a new autotest? > I don't have an autotest, it was clearly a corner case that had the pixels left uncovered. I very much doubt I'll get a case to cover it - but can try later. Can you point me to a autotest that does pixel comparisons? I can't as you didn't provide a test case, I can't reproduce it. I'm not sure what you want to fix, but maybe you should look at graphicsview autotest (part of Qt). > > Spaces at the end of the line > Why do you want spaces at the end of the line? Sorry, my message wasn't clear. I don't want spaces at the end of line, but you shouldn't change it, as it is not related to the pixel dust problem. Actually the rule is more general a patch should touch only relevant code and only fix one issue. > > > +#if 0 > > If you believe that this code in unneeded you should remove it. > This was for Noam to experiment with as I think my fix removes the need for the above "hack" as they are solving a similar problem. > > > +#if 1 > Agree the #if 1 here is unnecessary - its a piece of code I never triggered so I left in Noam's "hack" to work around the issue since I couldn't confirm whether it was still necessary. My hope was that Noam would test if it was still necessary in the test that required it and possibly remove it. Great, so we need Noam's expertise :-) Preferably, we avoid #if 0/1. If you feel that a code could be removed then you can remove it, if code have to stay then you shouldn't touch it. If you need to share some experimental code, try to do it in other branch unless it is something really big. WebKit code base is huge and it is difficult to maintain it so lets try to keep it simple :-)

Created attachment 59431 [details] Rediff against trunk

Comment on attachment 59431 [details] Rediff against trunk Almost there, just a few nits WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:171 + void handleGeometryChange() wrong coding style... no need to add more than one space after void. WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:174 + // I do this here because it seems QGraphicsView doesn't seem to take into account the translated bounding rect when prepareGeometryChange happens Add a dot at the end. Personally I would wrap it at the middle WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:176 + const QRectF g = mapRectToScene(boundingRect()); g ? geometry? WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:453 + if ( transform2D != transform() ) { Remove space after transform2D and after transform()

We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.

(In reply to comment #11) > We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt. So this is not interesting for the qtwebkit2.0.x branch?

It is, but it's a corner case and in low priority to getting the major ones work faster (which would happen after 2.0/2.1).

(In reply to comment #13) > It is, but it's a corner case and in low priority to getting the major ones work faster (which would happen after 2.0/2.1). OK, I will let that up to you then. Please take a look at AC patches on webkit.org/pending-review then to WONTFIX those needed.