Bug 27598 - Clean up the AccessibilityObject class
Summary: Clean up the AccessibilityObject class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-22 23:22 PDT by Beth Dakin
Modified: 2009-07-23 11:11 PDT (History)
0 users

See Also:


Attachments
Patch (28.37 KB, patch)
2009-07-22 23:25 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2009-07-22 23:22:47 PDT
The AccessibilityObject class is the base class for all types accessibility objects. It has a lot of empty stubs of functions so that the main client of AccessibilityObject -- the AccessibilityObjectWrapper -- does not have to cast to different types of AccessibilityObjects to call appropriate functions. Some of these stubs should be pure virtual since all of the sub-classes implement them anyway. Others will need to stay empty stubs to avoid casting, but we can still clean up the class by moving them all to the header file since only half of these stubs are there now.
Comment 1 Beth Dakin 2009-07-22 23:25:12 PDT
Created attachment 33316 [details]
Patch
Comment 2 Darin Adler 2009-07-23 08:27:02 PDT
Comment on attachment 33316 [details]
Patch

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
Comment 3 Beth Dakin 2009-07-23 11:11:32 PDT
Thanks Darin! I fixed the lastIndexOK thing.

Fixed with r46276.