Summary: | Convert ScrollableArea ASSERT_NOT_REACHED virtuals into pure virtuals | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||||||||||||
Component: | Platform | Assignee: | Adrienne Walker <enne> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aestes, andersca, ap, enne, eric, fishd, jam, jamesr, kbr, mifenton, mitz, simon.fraser, tkent, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Adrienne Walker
2012-08-06 16:11:21 PDT
Created attachment 156783 [details]
Patch
This is a follow-up to bug 93297 (http://trac.webkit.org/changeset/124804), where http://trac.webkit.org/changeset/124714 added a call to ScrollableArea::scrollPosition() which broke Chromium WebKit unit tests in debug builds. This would have been caught if scrollPosition() had an override in the Chromium unit tests. Comment on attachment 156783 [details] Patch Attachment 156783 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13449252 Created attachment 156800 [details]
Fix override build errors
Comment on attachment 156800 [details] Fix override build errors Attachment 156800 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13445573 Created attachment 156816 [details]
Fix PopupMenuWin errors
Comment on attachment 156816 [details] Fix PopupMenuWin errors Attachment 156816 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13447637 Comment on attachment 156816 [details] Fix PopupMenuWin errors View in context: https://bugs.webkit.org/attachment.cgi?id=156816&action=review > Source/WebCore/ChangeLog:9 > + ASSERT_NOT_REACHED is a bad way to make sure that a virtual function > + gets an override, because it only fails at run-time in debug builds. These are not exactly equivalent, but I'm not sure if ASSERT_NOT_REACHED was used on purpose. There are lots of functions there that are likely only needed by some platforms, so making every platform create an override will complicate maintenance. Perhaps someone CC'ed on this bug remembers the reasons for this approach. Comment on attachment 156816 [details] Fix PopupMenuWin errors Attachment 156816 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13461052 (In reply to comment #8) > (From update of attachment 156816 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156816&action=review > > > Source/WebCore/ChangeLog:9 > > + ASSERT_NOT_REACHED is a bad way to make sure that a virtual function > > + gets an override, because it only fails at run-time in debug builds. > > These are not exactly equivalent, but I'm not sure if ASSERT_NOT_REACHED was used on purpose. There are lots of functions there that are likely only needed by some platforms, so making every platform create an override will complicate maintenance. I'm having a hard time understand what the original intent is here, as this is not a design pattern I have seen before and it looks dangerous. I definitely agree with you that it adds some overhead to deriving from ScrollableArea. However, both ASSERT_NOT_REACHED and a bogus return value in a base class virtual is a potential landmine for future developers who might see those functions in the interface and try to call them. I feel that there's more overhead to knowing which functions are safe and correct to call than there is in just implementing them in the first place. Another option might be to implement reasonable versions in ScrollableArea where possible, which would limit the amount of abstract functions that derived classes need to create. Created attachment 157350 [details]
Implement most funcs in ScrollableArea
Comment on attachment 157350 [details] Implement most funcs in ScrollableArea Attachment 157350 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13464134 Comment on attachment 157350 [details] Implement most funcs in ScrollableArea Attachment 157350 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13461389 Comment on attachment 157350 [details] Implement most funcs in ScrollableArea View in context: https://bugs.webkit.org/attachment.cgi?id=157350&action=review Looks good. Please don’t land without fixing Mac and Windows build. Please consider addressing my comments below as well. > Source/WebCore/platform/ScrollView.h:173 > + virtual IntSize scrollOffset() const { return visibleContentRect().location() - IntPoint(); } // Gets the scrolled position as an IntSize. Convenient for adding to other sizes. Why does this function need to be virtual? Was it virtual before? > Source/WebCore/platform/ScrollableArea.cpp:408 > + return IntRect(scrollPosition().x(), > + scrollPosition().y(), > + std::max(0, visibleWidth() + verticalScrollbarWidth), > + std::max(0, visibleHeight() + horizontalScrollbarHeight)); WebKit coding style explicitly suggests avoiding lining up subsequent lines like this. Please change to match the coding style, or argume for getting that changed. > Source/WebCore/platform/chromium/FramelessScrollView.cpp:68 > + // FIXME More detail here? Comment on attachment 157350 [details] Implement most funcs in ScrollableArea View in context: https://bugs.webkit.org/attachment.cgi?id=157350&action=review Thanks for the review. I'll clean up the build errors and submit this. :) >> Source/WebCore/platform/ScrollView.h:173 >> + virtual IntSize scrollOffset() const { return visibleContentRect().location() - IntPoint(); } // Gets the scrolled position as an IntSize. Convenient for adding to other sizes. > > Why does this function need to be virtual? Was it virtual before? Oh, good catch. Will fix. >> Source/WebCore/platform/ScrollableArea.cpp:408 >> + std::max(0, visibleHeight() + horizontalScrollbarHeight)); > > WebKit coding style explicitly suggests avoiding lining up subsequent lines like this. Please change to match the coding style, or argume for getting that changed. I borrowed this code from ScrollView::visibleContentRect, which uses this same style (but different variables) for its visibleContentRect implementation. Where is this style explicitly suggested? The best that I can find is that http://www.webkit.org/coding/coding-style.html#braces-one-line implicitly gives an example of not vertically aligning function arguments. However, that's a weak argument. Am I missing something? >> Source/WebCore/platform/chromium/FramelessScrollView.cpp:68 >> + // FIXME > > More detail here? I'll just remove it. I was mirroring the // FIXME in isActive() above to indicate that (likely) this value isn't entirely intended. Created attachment 159994 [details]
Fix WebKit2 / Win build errors
Comment on attachment 159994 [details] Fix WebKit2 / Win build errors Attachment 159994 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13574103 Created attachment 159998 [details]
Fix WebPopupMenuProxyWin build error
Comment on attachment 159998 [details] Fix WebPopupMenuProxyWin build error Attachment 159998 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13570369 Created attachment 160007 [details]
Fix WebPopupMenuProxyWin.cpp error
Comment on attachment 160007 [details] Fix WebPopupMenuProxyWin.cpp error Clearing flags on attachment: 160007 Committed r126444: <http://trac.webkit.org/changeset/126444> All reviewed patches have been landed. Closing bug. Committed r126455: <http://trac.webkit.org/changeset/126455> |