New issue
Advanced search Search tips

Issue 866487 link

Starred by 1 user
Status: Fixed
Owner:
f...@opera.com
Closed: Sep 25
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

SVG modifies paint properties during paint

Project Member Reported by pdr@chromium.org, Jul 23 
I made a patch that ensures we do not change paint properties during paint (https://chromium-review.googlesource.com/c/chromium/src/+/1146150) but it fails the following tests:
  svg/text/selection-background-color.xhtml
  svg/text/selection-partial-gradient.html
  svg/text/selection-pseudo-resource-invalidation-crash.html
  svg/text/selection-styles.xhtml

Check failed: GetDocument().Lifecycle().GetState() != DocumentLifecycle::LifecycleState::kInPaint:
  blink::LayoutObject::SetNeedsPaintPropertyUpdate [0x02FD76C4+116]
  blink::SVGResourcesCache::AddResourcesFromLayoutObject [0x03A8E3B4+132]
  blink::SVGResourcesCache::TemporaryStyleScope::SwitchTo [0x03A8ECEC+124]
  blink::SVGResourcesCache::TemporaryStyleScope::TemporaryStyleScope [0x03A8EC4A+250]
  blink::SVGInlineTextBoxPainter::PaintText [0x04CF2BF8+216]

We're calling SetNeedsPaintPropertyUpdate during paint because the selection style changes. Selection styles are special and do not allow styles that change paint properties (only fill, stroke, etc). Because of this, I think we can suppress the call to SetNeedPaintPropertyUpdate when it is called from SVGResourcesCache::TemporaryStyleScope for selection.

Comment 1 by f...@opera.com, Aug 13 

Yes, this code-path definitely does not need to call SetNeedsPaintPropertyUpdate... (I've been working to remove the SVGResources object altogether, but it's proven to be a somewhat arduous process thus far...)

One option to consider would be hoisting the call, and let Add/RemoveResourcesFromLayoutObject (which are often called in a paired fashion) indicate whether it's needed or not.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 25 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ae36bb9152e639783ed04510389aa20b13e871c

commit 0ae36bb9152e639783ed04510389aa20b13e871c
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue Sep 25 15:26:21 2018

Hoist SetNeedsPaintPropertyUpdate calls in SVGResourcesCache

We don't want/need to call SetNeedsPaintPropertyUpdate in the
TemporaryStyleScope helper.
Hoist the calls to SetNeedsPaintPropertyUpdate out of
AddResourcesFromLayoutObject/RemoveResourcesFromLayoutObject.
Also add a new UpdateResourcesFromLayoutObject function that wraps the
common Remove+Add sequence.

Bug:  866487 
Change-Id: I8f8a82802be0e16807e04eead0e409562ecbdb77
Reviewed-on: https://chromium-review.googlesource.com/1243244
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#593943}
[modify] https://crrev.com/0ae36bb9152e639783ed04510389aa20b13e871c/third_party/blink/renderer/core/layout/svg/svg_resources_cache.cc
[modify] https://crrev.com/0ae36bb9152e639783ed04510389aa20b13e871c/third_party/blink/renderer/core/layout/svg/svg_resources_cache.h

Comment 3 by f...@opera.com, Sep 25

Owner: f...@opera.com
Status: Fixed (was: Available)
►

Sign in to add a comment