Bug 54301 – RenderBR::width() tries but fails to override RenderText::width()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

RenderBR::width() tries but fails to override RenderText::width()

[Nico Weber]

Created attachment 82158 [details]
Patch

[Nico Weber]

Found by clang's new -Woverload-virtual:

In file included from ../rendering/InlineBox.h:24:
../rendering/RenderBR.h:43:22: error: 'WebCore::RenderBR::width' hides overloaded virtual functions [-Woverloaded-virtual]
    virtual unsigned width(unsigned /*from*/, unsigned /*len*/, const Font&, int /*xpos*/) const { return 0; }
                     ^
In file included from /b/build/slave/mac_clang/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../dom/Node.cpp:72:
In file included from ../rendering/RenderBlock.h:30:
In file included from ../rendering/RootInlineBox.h:25:
In file included from ../rendering/InlineFlowBox.h:24:
In file included from ../rendering/InlineBox.h:24:
In file included from ../rendering/RenderBR.h:24:
../rendering/RenderText.h:74:22: note: hidden overloaded virtual function 'WebCore::RenderText::width' declared here
    virtual unsigned width(unsigned from, unsigned len, const Font&, int xPos, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* = 0) const;
                     ^
../rendering/RenderText.h:75:22: note: hidden overloaded virtual function 'WebCore::RenderText::width' declared here
    virtual unsigned width(unsigned from, unsigned len, int xPos, bool firstLine = false, HashSet<const SimpleFontData*>* fallbackFonts = 0, GlyphOverflow* = 0) const;

[Darin Adler]

Comment on attachment 82158 [details]
Patch

What’s the symptom of this bug? We need a test case that will catch this if we make the same mistake in the future.

Did you try to make a test case?

[Darin Adler]

Oh, I see, the compiler found it. Still, can we make a test case?

Does fixing this change any regression test results?

[Nico Weber]

I'm trying to get webkit building with -Woverloaded-virtual so that the compiler catches this problem automatically. Does webkit generally use test cases for things that are found by compilers? If so, I will try to come up with one.

[Darin Adler]

(In reply to comment #5)
> Does webkit generally use test cases for things that are found by compilers?

Yes, if the compiler reveals a mistake that should have caused incorrect behavior.

No, if the compiler catches something that has no real effect.

[James Robinson]

This doesn't seem to change the results of any layout tests on my snow leopard mac.

[Anders Carlsson]

The only (In reply to comment #4)
> Oh, I see, the compiler found it. Still, can we make a test case?
> 
> Does fixing this change any regression test results?

Before this, the width of a RenderBR would take the slower RenderText codepath, which would return 0. (Assuming the width of the string "\n" is 0). I don't think we could make a test case for this.

[Anders Carlsson]

I'm going to go ahead and land this.

[Anders Carlsson]

Comment on attachment 82158 [details]
Patch

Clearing flags on attachment: 82158

Committed r78444: <http://trac.webkit.org/changeset/78444>

[Anders Carlsson]

All reviewed patches have been landed.  Closing bug.