Fix assertion: !visualMetricsValues.isEmpty() in layoutTextOnLineOrPath

Fix assertion: !visualMetricsValues.isEmpty() in layoutTextOnLineOrPath

Having only whitespaces within a <tspan> and setting white-space
to pre-wrap fires the !visualMetricsValues.isEmpty() assert,
because the new renderer has no visualMetricsValues. However,
the renderer shouldn't even be created in such case.

So, if the text only contains whitespaces, and the white-space
CSS property is set to pre-wrap, and the parent of the text is
an SVG node, we avoid creating a renderer.

Peter Molnar (pmolnar.u-szeged@partner.samsung.com) started this
patch before, and he let me continue his work.

BUG=416833

[zherczeg, 2014-11-24 10:08:53]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ eseidel@chromium.org, kinuko@chromium.org, pdr@chromium.org

[zherczeg, 2014-11-24 10:08:53]

Fixed Peter's original patch (https://codereview.chromium.org/635353002/) and
simplified the test case.

@eseidel, please review this.

[zherczeg, 2014-12-01 07:37:13]

> @eseidel, please review this.

Ping!

[zherczeg, 2014-12-03 12:20:40]

On 2014/12/01 07:37:13, zherczeg wrote:
> > @eseidel, please review this.
> 
> Ping!

Eseidel, please take a look. I think this is an easy bug.

[zherczeg, 2014-12-07 12:17:51]

zherczeg.u-szeged@partner.samsung.com changed reviewers:
+ esprehn@chromium.org, hayato@chromium.org, tonyg@chromium.org
- kinuko@chromium.org, pdr@chromium.org

[zherczeg, 2014-12-07 12:17:51]

If Eseidel is not available, plase somebody else review this. This patch is here
for a long time.

[pdr., 2014-12-08 21:30:41]

On 2014/12/07 at 12:17:51, zherczeg.u-szeged wrote:
> If Eseidel is not available, plase somebody else review this. This patch is
here for a long time.

Please use the owners files and "git log" for finding reviewers. fs@opera.com
and kouhei@chromium would be great reviewers for this patch.

[pdr., 2014-12-08 21:30:57]

pdr@chromium.org changed reviewers:
+ fs@opera.com, kouhei@chromium.org
- eseidel@chromium.org, esprehn@chromium.org, hayato@chromium.org,
tonyg@chromium.org

[pdr., 2014-12-08 21:31:04]

pdr@chromium.org changed reviewers:
+ pdr@chromium.org

[kouhei (in TOK), 2014-12-09 09:15:23]

https://codereview.chromium.org/755613002/diff/1/LayoutTests/svg/text/tspan-p...
File LayoutTests/svg/text/tspan-pre-wrap-whitespace-crash.html (right):

https://codereview.chromium.org/755613002/diff/1/LayoutTests/svg/text/tspan-p...
LayoutTests/svg/text/tspan-pre-wrap-whitespace-crash.html:4: <tspan
style="white-space:pre-wrap";> </tspan>
; should be before "

https://codereview.chromium.org/755613002/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/755613002/diff/1/Source/core/dom/Text.cpp#new...
Source/core/dom/Text.cpp:260: if (style.whiteSpace() == PRE_WRAP &&
parent.isSVG()) // pre-wrap in SVG never makes renderer.
Should we also care about pre/pre-line?

[fs, 2014-12-09 10:18:33]

https://codereview.chromium.org/755613002/diff/1/LayoutTests/svg/text/tspan-p...
File LayoutTests/svg/text/tspan-pre-wrap-whitespace-crash.html (right):

https://codereview.chromium.org/755613002/diff/1/LayoutTests/svg/text/tspan-p...
LayoutTests/svg/text/tspan-pre-wrap-whitespace-crash.html:4: <tspan
style="white-space:pre-wrap";> </tspan>
On 2014/12/09 09:15:22, kouhei wrote:
> ; should be before "

Or just drop it entirely.

https://codereview.chromium.org/755613002/diff/1/Source/core/dom/Text.cpp
File Source/core/dom/Text.cpp (right):

https://codereview.chromium.org/755613002/diff/1/Source/core/dom/Text.cpp#new...
Source/core/dom/Text.cpp:260: if (style.whiteSpace() == PRE_WRAP &&
parent.isSVG()) // pre-wrap in SVG never makes renderer.
On 2014/12/09 09:15:23, kouhei wrote:
> Should we also care about pre/pre-line?

All cases in which the metrics builder will end up collapsing I suppose - ie.
everything not 'pre'. (SVG2 will change this though.)

[zherczeg, 2014-12-09 12:13:02]

> All cases in which the metrics builder will end up collapsing I suppose - ie.
> everything not 'pre'. (SVG2 will change this though.)

If I understand this correctly, I only need to remove the semicolon. Thank you
for the comments.

[fs, 2014-12-09 12:18:08]

On 2014/12/09 12:13:02, zherczeg wrote:
> > All cases in which the metrics builder will end up collapsing I suppose -
ie.
> > everything not 'pre'. (SVG2 will change this though.)
> 
> If I understand this correctly, I only need to remove the semicolon. Thank you
> for the comments.

The way I would interpret myself would be: "style.whiteSpace() != PRE && ...".
But tests would confirm (or deny) that.

[zherczeg, 2014-12-16 13:16:19]

> The way I would interpret myself would be: "style.whiteSpace() != PRE && ...".
> But tests would confirm (or deny) that.

I still don't understand the change, but I tried to remove the
style.whiteSpace() check at all and got many fails. The following lines were
removed from dozens of svg tests:

RenderSVGInlineText {#text} at (0,0) size 0x0

Perhaps this is not a bug, just dropping an otherwise unnecessary element. I
don't know.

[fs, 2014-12-16 13:47:16]

On 2014/12/16 13:16:19, zherczeg wrote:
> > The way I would interpret myself would be: "style.whiteSpace() != PRE &&
...".
> > But tests would confirm (or deny) that.
> 
> I still don't understand the change...

The code that sets up the visualMetricsValues (SVGTextMetricsBuilder,
measureTextRenderer) has:

bool preserveWhiteSpace = text->style()->whiteSpace() == PRE;

meaning that for all other cases (!preserveWhiteSpace => whiteSpace() != PRE) it
will collapse the spaces - which seems to be related to the problem at hand
here.

> ..., but I tried to remove the
> style.whiteSpace() check at all and got many fails. The following lines were
> removed from dozens of svg tests:
>
> RenderSVGInlineText {#text} at (0,0) size 0x0
> 
> Perhaps this is not a bug, just dropping an otherwise unnecessary element. I
> don't know.

It's not obviously worse, no. Would need to be looked at on a case-by-case basis
though.

[fs, 2015-06-04 22:44:02]

"Salvaged" this to https://codereview.chromium.org/1166653005/