Bug 93306

Summary: Convert ScrollableArea ASSERT_NOT_REACHED virtuals into pure virtuals
Product: WebKit Reporter: Adrienne Walker <enne>
Component: PlatformAssignee: 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 Flags
Patch
none
Fix override build errors
none
Fix PopupMenuWin errors
none
Implement most funcs in ScrollableArea
none
Fix WebKit2 / Win build errors
none
Fix WebPopupMenuProxyWin build error
none
Fix WebPopupMenuProxyWin.cpp error none

Description Adrienne Walker 2012-08-06 16:11:21 PDT
Convert ScrollableArea ASSERT_NOT_REACHED virtuals into pure virtuals
Comment 1 Adrienne Walker 2012-08-06 16:16:55 PDT
Created attachment 156783 [details]
Patch
Comment 2 Adrienne Walker 2012-08-06 16:19:13 PDT
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 3 Build Bot 2012-08-06 16:42:26 PDT
Comment on attachment 156783 [details]
Patch

Attachment 156783 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13449252
Comment 4 Adrienne Walker 2012-08-06 16:56:49 PDT
Created attachment 156800 [details]
Fix override build errors
Comment 5 Build Bot 2012-08-06 17:34:55 PDT
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
Comment 6 Adrienne Walker 2012-08-06 18:07:05 PDT
Created attachment 156816 [details]
Fix PopupMenuWin errors
Comment 7 Build Bot 2012-08-06 18:34:31 PDT
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 8 Alexey Proskuryakov 2012-08-07 16:22:10 PDT
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 9 Build Bot 2012-08-08 00:08:17 PDT
Comment on attachment 156816 [details]
Fix PopupMenuWin errors

Attachment 156816 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13461052
Comment 10 Adrienne Walker 2012-08-08 10:51:10 PDT
(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.
Comment 11 Adrienne Walker 2012-08-08 17:40:51 PDT
Created attachment 157350 [details]
Implement most funcs in ScrollableArea
Comment 12 Build Bot 2012-08-08 18:27:44 PDT
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 13 Build Bot 2012-08-08 19:30:30 PDT
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 14 Darin Adler 2012-08-14 12:48:40 PDT
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 15 Adrienne Walker 2012-08-14 13:24:57 PDT
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.
Comment 16 Adrienne Walker 2012-08-22 13:00:36 PDT
Created attachment 159994 [details]
Fix WebKit2 / Win build errors
Comment 17 Build Bot 2012-08-22 13:20:34 PDT
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
Comment 18 Adrienne Walker 2012-08-22 13:24:11 PDT
Created attachment 159998 [details]
Fix WebPopupMenuProxyWin build error
Comment 19 Build Bot 2012-08-22 13:57:25 PDT
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
Comment 20 Adrienne Walker 2012-08-22 14:01:44 PDT
Created attachment 160007 [details]
Fix WebPopupMenuProxyWin.cpp error
Comment 21 WebKit Review Bot 2012-08-23 10:40:57 PDT
Comment on attachment 160007 [details]
Fix WebPopupMenuProxyWin.cpp error

Clearing flags on attachment: 160007

Committed r126444: <http://trac.webkit.org/changeset/126444>
Comment 22 WebKit Review Bot 2012-08-23 10:41:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Kenneth Russell 2012-08-23 11:36:30 PDT
Committed r126455: <http://trac.webkit.org/changeset/126455>