<br> nodes should not be allowed within SVG text

<br> nodes should not be allowed within SVG text

Add new method isRenderableTextNode to SVGRenderSupport to filter out any
RenderObjects that return true for isText(), but is not recognized by the
SVG text rendering codepath.

BUG=339993, 337607
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166505

[Stephen Chennney, 2014-02-03 16:01:15]

LGTM with a nit.

https://codereview.chromium.org/145493012/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGText.cpp (right):

https://codereview.chromium.org/145493012/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGText.cpp:73: return (child->isText() &&
SVGRenderSupport::isRenderableTextNode(child)) || child->isSVGInline();
Slightly more efficient to order the || clause the other way, right? You don't
expect one or other clause to be true more often, and the isSVGInline is
cheaper.

[Stephen Chennney, 2014-02-03 16:02:25]

NLGTM. Should be a test. Is there something that failed before the patch and
doesn't now?

[Stephen Chennney, 2014-02-03 16:02:51]

unLGTM?

[f(malita), 2014-02-03 16:04:07]

On 2014/02/03 16:02:51, Stephen Chenney wrote:
> unLGTM?

lngtm maybe?

[f(malita), 2014-02-03 16:05:47]

On 2014/02/03 16:04:07, Florin Malita wrote:
> On 2014/02/03 16:02:51, Stephen Chenney wrote:
> > unLGTM?
> 
> lngtm maybe?

not lgtm?

[fs, 2014-02-03 16:19:46]

On 2014/02/03 16:01:15, Stephen Chenney wrote:
> LGTM with a nit.
> 
>
https://codereview.chromium.org/145493012/diff/1/Source/core/rendering/svg/Re...
> File Source/core/rendering/svg/RenderSVGText.cpp (right):
> 
>
https://codereview.chromium.org/145493012/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGText.cpp:73: return (child->isText() &&
> SVGRenderSupport::isRenderableTextNode(child)) || child->isSVGInline();
> Slightly more efficient to order the || clause the other way, right? You don't
> expect one or other clause to be true more often, and the isSVGInline is
> cheaper.

I didn't see it as clear cut - and it does depend on expected frequency.

For the two interesting case the "cost" for the current version will be:

TextNode: isText()[==true] + isRenderable...[==true] (bit-test + virtual call +
"simple conditional" (hasEmptyText))
SVGInline: isText()[==false] + isSVGInline()[==true] (bit-test + virtual call)

and for the other ordering:

TextNode: isSVGInline()[==false] + isText()[==true] + isRenderable...[==true] (2
* virtual call + bit-test + "simple conditional")
SVGInline: isSVGInline()[==true] (virtual call)

And I'd think that the TextNode case is the more common one? Anyway, I don't
really mind changing it (back).

[fs, 2014-02-03 16:21:12]

On 2014/02/03 16:02:25, Stephen Chenney wrote:
> NLGTM. Should be a test. Is there something that failed before the patch and
> doesn't now?

There's a (or, well, a number of) fuzzer-generated TCs - it looked daunting to
reduce, but I'll give it another try...

[f(malita), 2014-02-03 16:28:39]

On 2014/02/03 16:21:12, fs wrote:
> On 2014/02/03 16:02:25, Stephen Chenney wrote:
> > NLGTM. Should be a test. Is there something that failed before the patch and
> > doesn't now?
> 
> There's a (or, well, a number of) fuzzer-generated TCs - it looked daunting to
> reduce, but I'll give it another try...

Even if you cannot repro the original issue, maybe you can come up with a test
that captures the intent of this change: <br> renderers should not be
instantiated within the SVG render tree.

[fs, 2014-02-03 17:22:22]

On 2014/02/03 16:28:39, Florin Malita wrote:
> On 2014/02/03 16:21:12, fs wrote:
> > On 2014/02/03 16:02:25, Stephen Chenney wrote:
> > > NLGTM. Should be a test. Is there something that failed before the patch
and
> > > doesn't now?
> > 
> > There's a (or, well, a number of) fuzzer-generated TCs - it looked daunting
to
> > reduce, but I'll give it another try...
> 
> Even if you cannot repro the original issue, maybe you can come up with a test
> that captures the intent of this change: <br> renderers should not be
> instantiated within the SVG render tree.

Turns out I had done juat that, but forgot about it (doh!)... Coming up
shortly...

[fs, 2014-02-03 17:50:50]

TC added + order of disjunction restored.

[pdr., 2014-02-03 19:07:33]

On 2014/02/03 17:50:50, fs wrote:
> TC added + order of disjunction restored.

LGTM

[fs, 2014-02-04 08:25:33]

On 2014/02/03 16:05:47, Florin Malita wrote:
> On 2014/02/03 16:04:07, Florin Malita wrote:
> > On 2014/02/03 16:02:51, Stephen Chenney wrote:
> > > unLGTM?
> > 
> > lngtm maybe?
> 
> not lgtm?

@fmalita: Could you "undo" this now, please? (Given that you are ok with the
latest PS naturally...)

[fs, 2014-02-05 14:45:18]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-02-05 14:45:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/145493012/230001

[commit-bot: I haz the power, 2014-02-05 15:32:04]

Change committed as 166505