[webkit-reviews] review granted: [Bug 27598] Clean up the AccessibilityObject class : [Attachment 33316] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 27598: Clean up the AccessibilityObject class
https://bugs.webkit.org/show_bug.cgi?id=27598

Attachment 33316: Patch
https://bugs.webkit.org/attachment.cgi?id=33316&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Making the functions pure virtual seems great to me!

It also seems OK to move these "stubs" to the header. I know Hyatt likes this
style.

But personally I normally avoid putting bodies of virtual functions into the
class definition in the header, even small stubs. For one thing, it means
changing any of these requires recompilation of any files that include the
header. And generally there are few opportunities for virtual functions to be
inlined, which is the usual reason for putting things in a header or in the
class definition.

> -    virtual VisiblePosition visiblePositionForIndex(unsigned indexValue,
bool lastIndexOK) const;
> +    virtual VisiblePosition visiblePositionForIndex(unsigned, bool) const {
return VisiblePosition(); }

Here I think it's not so great to lose the names of the arguments, since the
meaning of the bool is not clear without a name. Maybe keep lastIndexOK around,
but commented out, like this:

    virtual VisiblePosition visiblePositionForIndex(unsigned, bool /*
lastIndexOK */) const { return VisiblePosition(); }

Or leave this one in the implementation file.

r=me as-is, but please consider the lastIndexOK thing