Use references instead of pointers in files SVGRootInlineBox.{h, cpp}, SVGTextLayoutEngine.{h, cpp} and SVGTextLayoutEngineBaseline.{h, cpp} when we expect a non-null object.
Created attachment 251324 [details] Patch
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.
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?
(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.
Created attachment 251331 [details] Patch Updated patch based on Darin Adler's feedback.
Created attachment 251333 [details] Patch
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
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
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.
(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>.
Comment on attachment 251333 [details] Patch Clearing flags on attachment: 251333 Committed r183219: <http://trac.webkit.org/changeset/183219>
All reviewed patches have been landed. Closing bug.