Summary: | REGRESSION(r216212): RenderReplaced::paint() should not save and restore the context unless it has to | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||
Component: | Layout and Rendering | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, commit-queue, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-06-02 17:05:02 PDT
Created attachment 311893 [details]
Patch
Comment on attachment 311893 [details]
Patch
Thanks Tim!
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. Comment on attachment 311893 [details] Patch Clearing flags on attachment: 311893 Committed r217748: <http://trac.webkit.org/changeset/217748> All reviewed patches have been landed. Closing bug. 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". 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. |