Rename transformToRoot:ish names to screenScaleFactorChanged:ish ones

Rename transformToRoot:ish names to screenScaleFactorChanged:ish ones

This better reflects the function of this machinery, since it's used to
update the scale of fonts based on the computed "screen scale factor".

BUG=603956
Committed: https://crrev.com/fd51da99e43dcce990befd948740515e7a4a2900
Cr-Commit-Position: refs/heads/master@{#388749}

[fs, 2016-04-20 17:42:04]

fs@opera.com changed reviewers:
+ fmalita@chromium.org, pdr@chromium.org, schenney@chromium.org

[fs, 2016-04-20 17:42:04]

This is bikeshed-fodder: I've picked 'screen' to be included in the name because
that's (one of) the term(s) used in the name of the function that computes the
scale to use [1]. 'font' might be another possibility, but I thought that might
look out of place and describe function rather than semantic. Your input
welcome. 

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Stephen Chennney, 2016-04-20 19:36:31]

On 2016/04/20 17:42:04, fs wrote:
> This is bikeshed-fodder: I've picked 'screen' to be included in the name
because
> that's (one of) the term(s) used in the name of the function that computes the
> scale to use [1]. 'font' might be another possibility, but I thought that
might
> look out of place and describe function rather than semantic. Your input
> welcome. 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm happy with it but I'll let pdr@ or fmalita@ also weigh in.

[f(malita), 2016-04-20 20:00:32]

https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp (right):

https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp:59:
m_didScreenScaleFactorChange = updatedTransform ||
SVGLayoutSupport::screenScaleFactorChanged(parent());
If we only care about scale factor changes, then this looks too conservative.  I
presume we could do something smart about it (only track actual scale changes),
or tweak the name to reflect the current reality:
s/screenScaleChanged/screenTransformChanged/

I'm fine with landing as-is too, if the plan is to fix reality later :)

[fs, 2016-04-20 20:18:20]

https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp (right):

https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp:59:
m_didScreenScaleFactorChange = updatedTransform ||
SVGLayoutSupport::screenScaleFactorChanged(parent());
On 2016/04/20 at 20:00:32, f(malita) wrote:
> If we only care about scale factor changes, then this looks too conservative. 
I presume we could do something smart about it (only track actual scale
changes), or tweak the name to reflect the current reality:
s/screenScaleChanged/screenTransformChanged/

Right, it was "undisclosed" that I intend to make this less conservative by
replacing the "updatedTransform" bit with something tighter in a follow-up CL.

[f(malita), 2016-04-20 20:20:30]

On 2016/04/20 20:18:20, fs wrote:
>
https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp (right):
> 
>
https://codereview.chromium.org/1911473002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGContainer.cpp:59:
> m_didScreenScaleFactorChange = updatedTransform ||
> SVGLayoutSupport::screenScaleFactorChanged(parent());
> On 2016/04/20 at 20:00:32, f(malita) wrote:
> > If we only care about scale factor changes, then this looks too
conservative. 
> I presume we could do something smart about it (only track actual scale
> changes), or tweak the name to reflect the current reality:
> s/screenScaleChanged/screenTransformChanged/
> 
> Right, it was "undisclosed" that I intend to make this less conservative by
> replacing the "updatedTransform" bit with something tighter in a follow-up CL.

SGTM/LGTM

[pdr., 2016-04-20 22:48:32]

LGTM3 with a meta-question (LGTM regardless of the answer): is updating the font
after scaling the correct pattern to use? It was added a long time ago and I'm
not sure we even need it anymore. For example, when we put a 3d transform on a
div, we don't need to re-layout--what makes SVG need a layout? The minimum font
size was one reason, but that would be much better to implement elsewhere now
that we don't depend on CoreGraphics.

Nit: could update the comment in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
too, but that's optional.

[fs, 2016-04-21 10:43:39]

On 2016/04/20 at 22:48:32, pdr wrote:
> LGTM3 with a meta-question (LGTM regardless of the answer): is updating the
font after scaling the correct pattern to use? It was added a long time ago and
I'm not sure we even need it anymore. For example, when we put a 3d transform on
a div, we don't need to re-layout--what makes SVG need a layout? The minimum
font size was one reason, but that would be much better to implement elsewhere
now that we don't depend on CoreGraphics.

The primary reason, as I understand it, would be that fonts don't (are not
guaranteed) to scale linearly. This means we can't just apply a new scale to a
subtree and expect paint invalidation to produce correct rectangles - most of
the time it likely will - or with small enough errors to not be noticable etc.
"Almost right" tends to not cut here though. (And since new metrics could affect
the glyph layout, we need to run at least the SVGTextLayoutEngine-related bits
of <text> layout.) That being said though, once we have support for a "simple"
repaint-only on transform (translation) updates, it should be straightforward to
experiment with other, more general, cases.
 
> Nit: could update the comment in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
too, but that's optional.

Updated.

[fs, 2016-04-21 11:58:22]

The CQ bit was checked by fs@opera.com

[fs, 2016-04-21 11:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1911473002/#ps20001
(title: "Update comment in LayoutSVGText.")

[commit-bot: I haz the power, 2016-04-21 11:58:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1911473002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1911473002/20001

[commit-bot: I haz the power, 2016-04-21 12:17:28]

Committed patchset #2 (id:20001)

[pdr., 2016-04-21 19:00:00]

On 2016/04/21 at 10:43:39, fs wrote:
> On 2016/04/20 at 22:48:32, pdr wrote:
> > LGTM3 with a meta-question (LGTM regardless of the answer): is updating the
font after scaling the correct pattern to use? It was added a long time ago and
I'm not sure we even need it anymore. For example, when we put a 3d transform on
a div, we don't need to re-layout--what makes SVG need a layout? The minimum
font size was one reason, but that would be much better to implement elsewhere
now that we don't depend on CoreGraphics.
> 
> The primary reason, as I understand it, would be that fonts don't (are not
guaranteed) to scale linearly. This means we can't just apply a new scale to a
subtree and expect paint invalidation to produce correct rectangles - most of
the time it likely will - or with small enough errors to not be noticable etc.
"Almost right" tends to not cut here though. (And since new metrics could affect
the glyph layout, we need to run at least the SVGTextLayoutEngine-related bits
of <text> layout.) That being said though, once we have support for a "simple"
repaint-only on transform (translation) updates, it should be straightforward to
experiment with other, more general, cases.

I'm not sure this is the case and I'm wondering if we can optimize the whole
stack (scale changes included) instead of special-casing translations.

You can specify completely different glyphs for different font sizes, so the
font metrics can wildly change as the font-size changes. Because you can specify
wildly different glyphs though, it seems wrong to ever change the font size from
what the user specifies*. In other words, if they say "font-size='30px'" with
scale(2), we should give them the 30px glyph scaled up, not the 60px glyph. The
shaping engine doesn't take into account the graphics context scale, so we
should be getting the same run width and metrics out regardless of the transform
to root.

Tracking scale changes and updating fonts is very expensive so I think there's
an opportunity to really clean this area up and simplify a bunch of logic.

* The exception to this might be incredibly small font sizes like 0.1px when the
viewport is set to be 0...1. The way we handle this code was done back when we
needed to support CoreGraphics though, which is no longer the case. I think we
can do something smarter now.

[fs, 2016-04-21 19:20:54]

On 2016/04/21 at 19:00:00, pdr wrote:
> On 2016/04/21 at 10:43:39, fs wrote:
> > On 2016/04/20 at 22:48:32, pdr wrote:
> > > LGTM3 with a meta-question (LGTM regardless of the answer): is updating
the font after scaling the correct pattern to use? It was added a long time ago
and I'm not sure we even need it anymore. For example, when we put a 3d
transform on a div, we don't need to re-layout--what makes SVG need a layout?
The minimum font size was one reason, but that would be much better to implement
elsewhere now that we don't depend on CoreGraphics.
> > 
> > The primary reason, as I understand it, would be that fonts don't (are not
guaranteed) to scale linearly. This means we can't just apply a new scale to a
subtree and expect paint invalidation to produce correct rectangles - most of
the time it likely will - or with small enough errors to not be noticable etc.
"Almost right" tends to not cut here though. (And since new metrics could affect
the glyph layout, we need to run at least the SVGTextLayoutEngine-related bits
of <text> layout.) That being said though, once we have support for a "simple"
repaint-only on transform (translation) updates, it should be straightforward to
experiment with other, more general, cases.
> 
> I'm not sure this is the case and I'm wondering if we can optimize the whole
stack (scale changes included) instead of special-casing translations.

Well, we can certainly try =).

> You can specify completely different glyphs for different font sizes, so the
font metrics can wildly change as the font-size changes. Because you can specify
wildly different glyphs though, it seems wrong to ever change the font size from
what the user specifies*. In other words, if they say "font-size='30px'" with
scale(2), we should give them the 30px glyph scaled up, not the 60px glyph. The
shaping engine doesn't take into account the graphics context scale, so we
should be getting the same run width and metrics out regardless of the transform
to root.

That's a fair (and true) point. I guess it somewhat boils down to what happens
when actually painting? (I.e what size face is actually used to rasterize the
glyphs.)

> Tracking scale changes and updating fonts is very expensive so I think there's
an opportunity to really clean this area up and simplify a bunch of logic.

After having spent a decent chunk of my day thinking of ways to break
layout/repaint when removing the conditions like this:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(with no luck - breaking stuff that is...), I do think there could be a lot of
stuff in the various layout()'s that do more harm than good. (I also suspect
that the test-coverage here isn't great to begin with though...)

> * The exception to this might be incredibly small font sizes like 0.1px when
the viewport is set to be 0...1. The way we handle this code was done back when
we needed to support CoreGraphics though, which is no longer the case. I think
we can do something smarter now.

Anything that starts getting dangerously close to LayoutUnit precision will be
problematic I think - not necessarily only for font-size reasons either.

[commit-bot: I haz the power, 2016-04-22 19:33:50]

Description was changed from

==========
Rename transformToRoot:ish names to screenScaleFactorChanged:ish ones

This better reflects the function of this machinery, since it's used to
update the scale of fonts based on the computed "screen scale factor".

BUG=603956
==========

to

==========
Rename transformToRoot:ish names to screenScaleFactorChanged:ish ones

This better reflects the function of this machinery, since it's used to
update the scale of fonts based on the computed "screen scale factor".

BUG=603956

Committed: https://crrev.com/fd51da99e43dcce990befd948740515e7a4a2900
Cr-Commit-Position: refs/heads/master@{#388749}
==========

[commit-bot: I haz the power, 2016-04-22 19:33:51]

Patchset 2 (id:??) landed as
https://crrev.com/fd51da99e43dcce990befd948740515e7a4a2900
Cr-Commit-Position: refs/heads/master@{#388749}