Bug 144045 – Clean up: Use references instead of pointers in more SVG files

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Daniel Bates]

Use references instead of pointers in files SVGRootInlineBox.{h, cpp}, SVGTextLayoutEngine.{h, cpp} and SVGTextLayoutEngineBaseline.{h, cpp} when we expect a non-null object.

[Daniel Bates]

Created attachment 251324 [details]
Patch

[Darin Adler]

Comment on attachment 251324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251324&action=review

> Source/WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:57
> -    default:
> +    case BS_LENGTH:
>          ASSERT_NOT_REACHED();
>          return 0;
>      }

WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:58:1: error: control reaches end of non-void function [-Werror=return-type]

The right way to write this is:

    case BS_LENGTH:
         break;
    }
    ASSERT_NOT_REACHED();
    return 0;

This catches unhanded valid enum values at compile time, and both unhanded valid and invalid enum values at runtime.

[Daniel Bates]

Created attachment 251330 [details]
Patch

Add default case statement to switch block in calculateBaselineShift() to make the EFL EWS happy :( Alternatively we could remove the default case, add a case for BS_LENGTH, and ASSERT_NOT_REACHED() and return outside the switch. I chose to add the default case instead of duplicating logic. Updated ChangeLog entry for calculateBaselineShift() to reflect the rename of parameter contextElement to context since the suffix is implied by the data type of this parameter being SVGElement. Let me know if contextElement is preferred. Or even better, can we come up with a more descriptive name for this parameter?

[Daniel Bates]

(In reply to comment #2)
> > Source/WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:57
> > -    default:
> > +    case BS_LENGTH:
> >          ASSERT_NOT_REACHED();
> >          return 0;
> >      }
> 
> WebCore/rendering/svg/SVGTextLayoutEngineBaseline.cpp:58:1: error: control
> reaches end of non-void function [-Werror=return-type]
> 
> The right way to write this is:
> 
>     case BS_LENGTH:
>          break;
>     }
>     ASSERT_NOT_REACHED();
>     return 0;
> 
> This catches unhanded valid enum values at compile time, and both unhanded
> valid and invalid enum values at runtime.

Will use this approach.

[Daniel Bates]

Created attachment 251331 [details]
Patch

Updated patch based on Darin Adler's feedback.

[Daniel Bates]

Created attachment 251333 [details]
Patch

[Build Bot]

Comment on attachment 251333 [details]
Patch

Attachment 251333 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6423437786480640

New failing tests:
compositing/scrolling/touch-scroll-to-clip.html

[Build Bot]

Created attachment 251334 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Darin Adler]

Comment on attachment 251333 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251333&action=review

Any idea why the regression test is failing? False result or real bug this somehow introduces?

> Source/WebCore/rendering/svg/SVGTextLayoutEngineBaseline.h:40
> +    float calculateBaselineShift(const SVGRenderStyle&, SVGElement* context) const;

I’m surprised that context can ever be null. I think that eventually that too will be a reference.

[Daniel Bates]

(In reply to comment #9)
> Comment on attachment 251333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251333&action=review
> 
> Any idea why the regression test is failing? False result or real bug this
> somehow introduces?
> 

This test fails without the patch on my OS X Yosemite machine. For some reason, this tests doesn't fail on any of the Yosemite test bots on build.webkit.org (why?). Filed bug #144127 to track this test failure.

> > Source/WebCore/rendering/svg/SVGTextLayoutEngineBaseline.h:40
> > +    float calculateBaselineShift(const SVGRenderStyle&, SVGElement* context) const;
> 
> I’m surprised that context can ever be null. I think that eventually that
> too will be a reference.

We can make this a reference if we can prove that SVGTextLayoutEngine::layoutTextOnLineOrPath() is never passed a RenderSVGInlineText text object whose parent is an anonymous renderer. That is, lengthContext is never nullptr on <http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp?rev=182864#L410>.

[Daniel Bates]

Comment on attachment 251333 [details]
Patch

Clearing flags on attachment: 251333

Committed r183219: <http://trac.webkit.org/changeset/183219>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.