Align assumptions in RenderSVGResourceSolidColor with other paint servers

Align assumptions in RenderSVGResourceSolidColor with other paint servers

Since the SVG fonts code-path no longer directly call
RenderSVGResource::applyResource, the same pre-conditions should hold
for RenderSVGResourceSolidColor as for the other paint servers. Drop the
existing (stale) comment and assert that |object| and |style| are
non-null instead.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182159

[fs, 2014-09-12 10:52:02]

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

[f(malita), 2014-09-12 14:00:50]

This is not immediately obvious to me. SVGTextRunRenderingContext::drawSVGGlyphs
passes *parentRenderObject* as a renderer, which it not only doesn't assert but
explicitly checks against null:

    if (parentRenderObject) {
        parentRenderObjectStyle = parentRenderObject->style();
        ASSERT(parentRenderObjectStyle);
        isVerticalText =
parentRenderObjectStyle->svgStyle().isVerticalWritingMode();
    }

Looking at parentRenderObject, it is just the parent() of the text run renderer
(which we do assert as non-null). I *think* it can never be null (can a text
renderer ever be the root?), so the change is likely correct -- but we should
add an assert in drawSVGGlyphs also (and drop the explicit check). WDYT?

[fs, 2014-09-12 14:11:51]

On 2014/09/12 14:00:50, Florin Malita wrote:
> This is not immediately obvious to me.
SVGTextRunRenderingContext::drawSVGGlyphs
> passes *parentRenderObject* as a renderer, which it not only doesn't assert
but
> explicitly checks against null:
> 
>     if (parentRenderObject) {
>         parentRenderObjectStyle = parentRenderObject->style();
>         ASSERT(parentRenderObjectStyle);
>         isVerticalText =
> parentRenderObjectStyle->svgStyle().isVerticalWritingMode();
>     }
> 
> Looking at parentRenderObject, it is just the parent() of the text run
renderer
> (which we do assert as non-null). I *think* it can never be null (can a text
> renderer ever be the root?), so the change is likely correct -- but we should
> add an assert in drawSVGGlyphs also (and drop the explicit check). WDYT?

What really caught my eye was that ~5 lines below the comment, |object| was
dereferenced... (and that code was old)
Yes, I suspect there's more that can be cleaned up around here... I'll drop the
explicit check in favor of an assert - and try refrain from too much code
shuffling.

[fs, 2014-09-17 14:15:30]

On 2014/09/12 14:11:51, fs wrote:
> On 2014/09/12 14:00:50, Florin Malita wrote:
> > This is not immediately obvious to me.
> SVGTextRunRenderingContext::drawSVGGlyphs
> > passes *parentRenderObject* as a renderer, which it not only doesn't assert
> but
> > explicitly checks against null:
> > 
> >     if (parentRenderObject) {
> >         parentRenderObjectStyle = parentRenderObject->style();
> >         ASSERT(parentRenderObjectStyle);
> >         isVerticalText =
> > parentRenderObjectStyle->svgStyle().isVerticalWritingMode();
> >     }
> > 
> > Looking at parentRenderObject, it is just the parent() of the text run
> renderer
> > (which we do assert as non-null). I *think* it can never be null (can a text
> > renderer ever be the root?), so the change is likely correct -- but we
should
> > add an assert in drawSVGGlyphs also (and drop the explicit check). WDYT?
> 
> What really caught my eye was that ~5 lines below the comment, |object| was
> dereferenced... (and that code was old)
> Yes, I suspect there's more that can be cleaned up around here... I'll drop
the
> explicit check in favor of an assert - and try refrain from too much code
> shuffling.

Adjusted after landing https://codereview.chromium.org/565063003, PTAL.

[f(malita), 2014-09-17 14:18:28]

lgtm

[fs, 2014-09-17 14:59:23]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-09-17 14:59:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559653006/60001

[commit-bot: I haz the power, 2014-09-17 15:15:14]

Committed patchset #4 (id:60001) as 182159