Bug 172883 – REGRESSION(r216212): RenderReplaced::paint() should not save and restore the context unless it has to

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

The change r216212 made RenderReplaced::paint() saves and restores the context every time it is called. However the context should be saved and restored only if the renderer is dragged and it is painted in a low alpha. Restoring the context is an expensive operation and should be called only when we have to. Actually with the low alpha painting case, we may not need the context to be saved and restored. All we need:

At the beginning of the function:
float alpha = paintInfo.context().alpha();

And before it exits:
paintInfo.context().setAlpha(alpha);

This affects the MotionMark scores.

[Radar WebKit Bug Importer]

<rdar://problem/32548614>

[Wenson Hsieh]

Created attachment 311893 [details]
Patch

[Wenson Hsieh]

Comment on attachment 311893 [details]
Patch

Thanks Tim!

[Said Abou-Hallawa]

Comment on attachment 311893 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        graphics context.

The whole context will be saved and restored because we just changed its alpha. This is still expensive. This might be okay because dragging the renderer is not the common case.

[WebKit Commit Bot]

Comment on attachment 311893 [details]
Patch

Clearing flags on attachment: 311893

Committed r217748: <http://trac.webkit.org/changeset/217748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[zalan]

Comment on attachment 311893 [details]
Patch

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

> Source/WebCore/rendering/RenderReplaced.cpp:166
>      if (element() && element()->parentOrShadowHostElement()) {
>          auto* parentContainer = element()->parentOrShadowHostElement();
> -        if (draggedContentContainsReplacedElement(document().markers().markersFor(parentContainer, DocumentMarker::DraggedContent), *element()))
> +        if (draggedContentContainsReplacedElement(document().markers().markersFor(parentContainer, DocumentMarker::DraggedContent), *element())) {
> +            savedGraphicsContext.save();

I'd rather have this as a state somewhere like we have it with selection (since dragging has a start and an end). We make all these calls on every paint when "paints with dragging/all paints --> 0".

[Wenson Hsieh]

Comment on attachment 311893 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        graphics context.
> 
> The whole context will be saved and restored because we just changed its alpha. This is still expensive. This might be okay because dragging the renderer is not the common case.

The renderer being dragged is a very rare case -- we should be okay here.

>> Source/WebCore/rendering/RenderReplaced.cpp:166
>> +            savedGraphicsContext.save();
> 
> I'd rather have this as a state somewhere like we have it with selection (since dragging has a start and an end). We make all these calls on every paint when "paints with dragging/all paints --> 0".

I see. I considered an approach like this, but didn't want to introduce new state to RenderObject or RenderText. What do you think about the following?
1. Make a new DraggedRange struct that has start/end offsets and make it an optional member of RenderObject.
2. When beginning a drag, use TextIterator to find text/images in the dragged range and set their renderers' dragged ranges.
3. When painting a RenderText or RenderReplaced, inspect this dragged content range to determine whether we need to fade before painting.
4. When dragging concludes, iterate through all nodes in the dragged range and clear their renderers' dragged ranges. Alternately, we could remember exactly which nodes we set dragged ranges for at the beginning of the drag, and unset those.