1358828 - Various bits of cleanup related to VectorImage painting

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

In VectorImage::Draw() we create a new SVGImageContext to store preserveAspectRatio override information if necessary. In addition to requiring us to create a new SVGImageContext, storing this information on SVGImageContext adds extra hashing overhead.

Storing this information here serves two purposes:

 1) it is used to pass the override value to AutoSVGRenderingState
    which then sets it on the root element of the SVG image

 2) it makes sure an override value - which changes the
    rendering - also changes the surface cache key,
    since SVGImageContext::Hash() is part of the key

#1 is unnecessary, since we are actually creating the modified SVGImageContext and passing it to the AutoSVGRenderingState ctor in VectorImage::Draw(), so there's no need to use the SVGImageContext as an intermediary. (Doing so also actually obfuscates what the code is doing IMO.)

#2 is more important, but can be dealt with in a better way. We already pass the information to LookupCachedSurface that we need to determine whether or not there is a preserveAspectRatio override. Specifically the SVGDrawingParameters object that we pass contains the flags that will or will not contain FLAG_FORCE_PRESERVEASPECTRATIO_NONE. Therefore in LookupCachedSurface we can check for that flag and modify VectorSurfaceKey as appropriate.

We should stop storing preserveAspectRatio override information on SVGImageContext to make the code more efficient (and readable).

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add some missing includes

I need this for a later patch.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Avoid setting state on SVG images if we use an image from the surface cache

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a06749566178
part 1 - Add some missing includes. r=longsonr
https://hg.mozilla.org/integration/mozilla-inbound/rev/97af1ab68078
part 2 - Avoid setting state on SVG images if we use an image from the surface cache. r=longsonr

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Split AutoSVGRenderingState into two separate classes

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: incomplete part 4 - Stop using SVGImageContext to store/pass preserveAspectRatio override information

I overlooked one other place where we set preserveAspectRatio on SVGImageContext, which is in nsSVGImageFrame. I'm not sure quite how/if to deal with that, but I still want to land part 3 in order to clear some ground to use the split out AutoPreserveAspectRatioOverride in bug 1353771.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a06749566178
https://hg.mozilla.org/mozilla-central/rev/97af1ab68078

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f0302be66e
part 3 - Split AutoSVGRenderingState into two separate classes. r=longsonr

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72f0302be66e

Comment 9

[Jonathan Watt [:jwatt]]

I guess I'm not going to do part 4, so let's call this done.

Comment 10

[Julian Seward [:jseward]]

(In reply to Pulsebot from comment #7)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/72f0302be66e
> part 3 - Split AutoSVGRenderingState into two separate classes. r=longsonr

I think one effect of this is to cause
AutoPreserveAspectRatioOverride::mDidOverride to be used on all paths in the
destructor, but only to be set on some paths in the constructor.  In other
words, the destructor use may use it uninitialised.

Comment 11

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.