54301 2011-02-11 11:38:49 -0800 RenderBR::width() tries but fails to override RenderText::width() 2011-02-21 14:39:43 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 54367 1 thakis thakis andersca darin hyatt jamesr mitz oldest_to_newest 349880 0 thakis 2011-02-11 11:38:49 -0800 RenderBR::width() tries but fails to override RenderText::width() 349883 1 82158 thakis 2011-02-11 11:40:44 -0800 Created attachment 82158 Patch 349884 2 thakis 2011-02-11 11:41:00 -0800 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; 349898 3 82158 darin 2011-02-11 12:04:05 -0800 Comment on attachment 82158 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? 349899 4 darin 2011-02-11 12:04:35 -0800 Oh, I see, the compiler found it. Still, can we make a test case? Does fixing this change any regression test results? 349914 5 thakis 2011-02-11 12:27:59 -0800 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. 349916 6 darin 2011-02-11 12:28:56 -0800 (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. 350433 7 jamesr 2011-02-13 11:49:30 -0800 This doesn't seem to change the results of any layout tests on my snow leopard mac. 350464 8 andersca 2011-02-13 15:13:04 -0800 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. 350465 9 andersca 2011-02-13 15:13:57 -0800 I'm going to go ahead and land this. 350466 10 82158 andersca 2011-02-13 15:19:50 -0800 Comment on attachment 82158 Patch Clearing flags on attachment: 82158 Committed r78444: <http://trac.webkit.org/changeset/78444> 350467 11 andersca 2011-02-13 15:19:56 -0800 All reviewed patches have been landed. Closing bug. 82158 2011-02-11 11:40:44 -0800 2011-02-13 15:19:50 -0800 Patch bug-54301-20110211114042.patch text/plain 1928 thakis U3VidmVyc2lvbiBSZXZpc2lvbjogNzgzNTEKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBmOWYzOWY5OTA3MzQyOWNh Zjk0ZmExMTFkZGVkMjNhYTIzZjNkOWMwLi43ZDNmMDdkZWJiYmQwMmMxNjhjMzE4MjY5OWMzNzA3 M2EwN2IwNzAyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDItMTEgIE5pY28g V2ViZXIgIDx0aGFraXNAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIFJlbmRlckJSOjp3aWR0aCgpIHRyaWVzIGJ1dCBmYWlscyB0 byBvdmVycmlkZSBSZW5kZXJUZXh0Ojp3aWR0aCgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NDMwMQorCisgICAgICAgIFRoZSBzdXBlcmNsYXNzIG1l dGhvZCdzIHNpZ25hdHVyZSBjaGFuZ2VkLCBlLmcuIGluCisgICAgICAgIGh0dHA6Ly90cmFjLndl YmtpdC5vcmcvY2hhbmdlc2V0LzU3MjE1LCBidXQgdGhlIHN1YmNsYXNzIHdhcyBub3QKKyAgICAg ICAgdXBkYXRlZC4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJCUi5oOgorICAgICAgICAo V2ViQ29yZTo6UmVuZGVyQlI6OndpZHRoKToKKwogMjAxMS0wMi0xMSAgU2hlcmlmZiBCb3QgIDx3 ZWJraXQucmV2aWV3LmJvdEBnbWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGlu ZyBvdXQgcjc4MzMxLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRl ckJSLmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQlIuaAppbmRleCA3MjE2YjVh MGMyYmE2ZDE4MmUzZmJkNDYzMTcwZGU4NTk3YjZmZjJmLi43MmE0NTE0NTM0ZjVjZTczZjc2Yzkw YzMwZmMyYjk3MzM0NzFjOWZiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyQlIuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyQlIuaApAQCAt NDAsOCArNDAsOCBAQCBwdWJsaWM6CiAgCiAgICAgdmlydHVhbCBJbnRSZWN0IHNlbGVjdGlvblJl Y3RGb3JSZXBhaW50KFJlbmRlckJveE1vZGVsT2JqZWN0KiAvKnJlcGFpbnRDb250YWluZXIqLywg Ym9vbCAvKmNsaXBUb1Zpc2libGVDb250ZW50Ki8pIHsgcmV0dXJuIEludFJlY3QoKTsgfQogCi0g ICAgdmlydHVhbCB1bnNpZ25lZCB3aWR0aCh1bnNpZ25lZCAvKmZyb20qLywgdW5zaWduZWQgLyps ZW4qLywgY29uc3QgRm9udCYsIGludCAvKnhwb3MqLykgY29uc3QgeyByZXR1cm4gMDsgfQotICAg IHZpcnR1YWwgdW5zaWduZWQgd2lkdGgodW5zaWduZWQgLypmcm9tKi8sIHVuc2lnbmVkIC8qbGVu Ki8sIGludCAvKnhwb3MqLywgYm9vbCAvKmZpcnN0TGluZSA9IGZhbHNlKi8pIGNvbnN0IHsgcmV0 dXJuIDA7IH0KKyAgICB2aXJ0dWFsIHVuc2lnbmVkIHdpZHRoKHVuc2lnbmVkIC8qZnJvbSovLCB1 bnNpZ25lZCAvKmxlbiovLCBjb25zdCBGb250JiwgaW50IC8qeFBvcyovLCBIYXNoU2V0PGNvbnN0 IFNpbXBsZUZvbnREYXRhKj4qID0gMCAvKmZhbGxiYWNrRm9udHMqLyAsIEdseXBoT3ZlcmZsb3cq ID0gMCkgY29uc3QgeyByZXR1cm4gMDsgfQorICAgIHZpcnR1YWwgdW5zaWduZWQgd2lkdGgodW5z aWduZWQgLypmcm9tKi8sIHVuc2lnbmVkIC8qbGVuKi8sIGludCAvKnhwb3MqLywgYm9vbCA9IGZh bHNlIC8qZmlyc3RMaW5lKi8sIEhhc2hTZXQ8Y29uc3QgU2ltcGxlRm9udERhdGEqPiogPSAwIC8q ZmFsbGJhY2tGb250cyovLCBHbHlwaE92ZXJmbG93KiA9IDApIGNvbnN0IHsgcmV0dXJuIDA7IH0K IAogICAgIGludCBsaW5lSGVpZ2h0KGJvb2wgZmlyc3RMaW5lKSBjb25zdDsKIAo=