RenderBR::width() tries but fails to override RenderText::width()
Created attachment 82158 [details] Patch
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;
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?
Oh, I see, the compiler found it. Still, can we make a test case? Does fixing this change any regression test results?
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.
(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.
This doesn't seem to change the results of any layout tests on my snow leopard mac.
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.
I'm going to go ahead and land this.
Comment on attachment 82158 [details] Patch Clearing flags on attachment: 82158 Committed r78444: <http://trac.webkit.org/changeset/78444>
All reviewed patches have been landed. Closing bug.