Bug 43921 – Window size changes + resources on absolute sized content leads to pixelation

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Resizing a SVG document containing an absolute sized object where a resources is applied leads to pixelation (for masks, atm.)
The resource is not being invalidated. When using a target object with relative lengths everything works fine.

[Nikolas Zimmermann]

Created attachment 64229 [details]
Patch

[Dirk Schulze]

Comment on attachment 64229 [details]
Patch

>  void SVGRenderSupport::layoutChildren(RenderObject* start, bool selfNeedsLayout)
>  {
>      bool layoutSizeChanged = svgRootTreeObject(start)->isLayoutSizeChanged();
> +    HashSet<RenderObject*> notlayoutedObjects;
>  
>      for (RenderObject* child = start->firstChild(); child; child = child->nextSibling()) {
>          bool needsLayout = selfNeedsLayout;
> @@ -252,11 +263,25 @@ void SVGRenderSupport::layoutChildren(Re
>          if (needsLayout) {
>              child->setNeedsLayout(true, false);
>              child->layout();
> -        } else
> -            child->layoutIfNeeded();
> +        } else {
> +            if (child->needsLayout())
> +                child->layout();
> +            else if (layoutSizeChanged)
> +                notlayoutedObjects.add(child);
> +        }
>  
>          ASSERT(!child->needsLayout());
>      }
> +
> +    if (!layoutSizeChanged) {
> +        ASSERT(notlayoutedObjects.isEmpty());
> +        return;
> +    }
> +
> +    // If the layout size changed, invalidate all resources of all children that didn't go through the layout() code path.
> +    HashSet<RenderObject*>::iterator end = notlayoutedObjects.end();
> +    for (HashSet<RenderObject*>::iterator it = notlayoutedObjects.begin(); it != end; ++it)
> +        invalidateResourcesOfChildren(*it);
>  }
>  

I'm not sure about the last loop here. You store the objects, because they don't need a relayout. But when you call invalidateResourcesOfChildren, the resources want the objects to relayout at least the boundaries. Seems unneccessary to me, because we already know, that they don't need a relayout.

[Nikolas Zimmermann]

(In reply to comment #2)
> > +    // If the layout size changed, invalidate all resources of all children that didn't go through the layout() code path.
> > +    HashSet<RenderObject*>::iterator end = notlayoutedObjects.end();
> > +    for (HashSet<RenderObject*>::iterator it = notlayoutedObjects.begin(); it != end; ++it)
> > +        invalidateResourcesOfChildren(*it);
> >  }
> >  
> 
> I'm not sure about the last loop here. You store the objects, because they don't need a relayout. But when you call invalidateResourcesOfChildren, the resources want the objects to relayout at least the boundaries. Seems unneccessary to me, because we already know, that they don't need a relayout.

You mixed up something. They call setNeedsBoundariesUpdate(), but not do any relayouting.
It just marks for a boundaries update, next time it relayouts. But this relayout doesn't happen, so there's no problem.

[Dirk Schulze]

(In reply to comment #3)
> (In reply to comment #2)
> > > +    // If the layout size changed, invalidate all resources of all children that didn't go through the layout() code path.
> > > +    HashSet<RenderObject*>::iterator end = notlayoutedObjects.end();
> > > +    for (HashSet<RenderObject*>::iterator it = notlayoutedObjects.begin(); it != end; ++it)
> > > +        invalidateResourcesOfChildren(*it);
> > >  }
> > >  
> > 
> > I'm not sure about the last loop here. You store the objects, because they don't need a relayout. But when you call invalidateResourcesOfChildren, the resources want the objects to relayout at least the boundaries. Seems unneccessary to me, because we already know, that they don't need a relayout.
> 
> You mixed up something. They call setNeedsBoundariesUpdate(), but not do any relayouting.
> It just marks for a boundaries update, next time it relayouts. But this relayout doesn't happen, so there's no problem.

Yes, I know it's just a flag. This makes still no sense. Why should we mark something for realyouting, but never relayout it? And even if we call layout sometime later, we have to update the boundaries, independent why we call layout. An expensive operation for RenderPath.

[Nikolas Zimmermann]

Created attachment 64313 [details]
Patch v2

New approach after discussing with Dirk.

[Dirk Schulze]

Comment on attachment 64313 [details]
Patch v2

LGTM. r=me

[Nikolas Zimmermann]

Landed in r65310.