[webkit-reviews] review denied: [Bug 73161] Absolute child is not repainted when parent opacity changes : [Attachment 116664] Fix Absolute child is not repainted when parent opacity changes

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Philip Rogers
<pdr at google.com>'s request for review:
Bug 73161: Absolute child is not repainted when parent opacity changes
https://bugs.webkit.org/show_bug.cgi?id=73161

Attachment 116664: Fix Absolute child is not repainted when parent opacity
changes
https://bugs.webkit.org/attachment.cgi?id=116664&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116664&action=review


r- for the absence of a proper ChangeLog and the current change which needs
some refinements.

Note: we usually reopen and reuse bugs when a change was rolled out as it makes
it easier to follow the patch iterations.

> Source/WebCore/ChangeLog:9
> +

Your explanations from the bug should be here. The ChangeLog should - at least
- explain why you are doing your change. Detailed explanations about each block
of code changed is better but not mandatory. Here I had to tab between the bug
and your patch as your ChangeLog is lagging.

> Source/WebCore/rendering/RenderObject.cpp:1703
> +		   diff = StyleDifferenceLayout;

This looks like a bad change as you may force a layout when a repaint is just
what is needed. Your explanations don't explain why we do need to fake a layout
(changing opacity really does not mandate doing a layout). It looks like this
was just a means for you to get your repaint later in setStyle().

The styleWillChange / styleDidChange logic should handle a proper repaint in
this case without having to abuse the |diff|.