Bug 93306 – Convert ScrollableArea ASSERT_NOT_REACHED virtuals into pure virtuals

• Component: Platform
• Status: RESOLVED FIXED

[Adrienne Walker]

Convert ScrollableArea ASSERT_NOT_REACHED virtuals into pure virtuals

[Adrienne Walker]

Created attachment 156783 [details]
Patch

[Adrienne Walker]

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.

[Build Bot]

Comment on attachment 156783 [details]
Patch

Attachment 156783 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13449252

[Adrienne Walker]

Created attachment 156800 [details]
Fix override build errors

[Build Bot]

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

[Adrienne Walker]

Created attachment 156816 [details]
Fix PopupMenuWin errors

[Build Bot]

Comment on attachment 156816 [details]
Fix PopupMenuWin errors

Attachment 156816 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13447637

[Alexey Proskuryakov]

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.

[Build Bot]

Comment on attachment 156816 [details]
Fix PopupMenuWin errors

Attachment 156816 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13461052

[Adrienne Walker]

(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.

[Adrienne Walker]

Created attachment 157350 [details]
Implement most funcs in ScrollableArea

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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?

[Adrienne Walker]

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.

[Adrienne Walker]

Created attachment 159994 [details]
Fix WebKit2 / Win build errors

[Build Bot]

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

[Adrienne Walker]

Created attachment 159998 [details]
Fix WebPopupMenuProxyWin build error

[Build Bot]

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

[Adrienne Walker]

Created attachment 160007 [details]
Fix WebPopupMenuProxyWin.cpp error

[WebKit Review Bot]

Comment on attachment 160007 [details]
Fix WebPopupMenuProxyWin.cpp error

Clearing flags on attachment: 160007

Committed r126444: <http://trac.webkit.org/changeset/126444>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Committed r126455: <http://trac.webkit.org/changeset/126455>