Bug 21443 – Allow HAVE_ACCESSIBILITY to be overridden at build time

• Component: Accessibility
• Status: NEW

[Amanda Walker]

Allow the HAVE_ACCESSIBILITY build setting to be overridden at build time, rather than keying only off of the platform, as a number of other build settings already permit.

[Amanda Walker]

Created attachment 24155 [details]
patch to allow HAVE_ACCESSIBILITY to be overridden at build time.

[Darin Adler]

Comment on attachment 24155 [details]
patch to allow HAVE_ACCESSIBILITY to be overridden at build time.

I don't think this is correct.

The HAVE_ macros are supposed to represent the presence of a certain underlying capability, not whether we want to take advantage of it or not. If we want ACCESSIBILITY to be something we can turn on and off, then it probably needs to be an ENABLE thing instead.

Or this might be yet an another PLATFORM(MAC) and PLATFORM(WIN) snafu of some sort.

Also, tabs in ChangeLog.

[Alp Toker]

As Darin says, accessibility should probably have been an ENABLE, not a HAVE. Would be nice if the patch could address that.

Moreover, the #if PLATFORM(MAC) && HAVE(ACCESSIBILITY) change doesn't make much sense. If you don't want accessibility in your port, you probably don't want to compile the Accessibility* classes in the first place. Can you try wrapping the entire classes with HAVE/ENABLE(ACCESSIBILITY) guards instead?

Thanks

[Alp Toker]

Comment on attachment 24155 [details]
patch to allow HAVE_ACCESSIBILITY to be overridden at build time.

r- based on Darin's comments and mine

[Amanda Walker]

(In reply to comment #2)
That makes sense, though the purpose is to suppress the underlying capability at build time.  I don't object to adding an ENABLE flag as well and requiring both to be set, but it seems unnecessarily verbose at first glance (since it would just change every occurrence of "HAVE(ACCESSIBILITY)" to "HAVE(ACCESSIBILITY) && ENABLE(ACCESSIBILITY)".

Will fix tabs in changelog--oversight.

(In reply to comment #3)
> Moreover, the #if PLATFORM(MAC) && HAVE(ACCESSIBILITY) change doesn't make much
> sense. If you don't want accessibility in your port, you probably don't want to
> compile the Accessibility* classes in the first place. Can you try wrapping the
> entire classes with HAVE/ENABLE(ACCESSIBILITY) guards instead?

Great suggestion.  Will try that and upload a new patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 24155 [details] [edit])
> I don't think this is correct.
> 
> The HAVE_ macros are supposed to represent the presence of a certain underlying
> capability, not whether we want to take advantage of it or not. If we want
> ACCESSIBILITY to be something we can turn on and off, then it probably needs to
> be an ENABLE thing instead.

(In reply to comment #3)
> As Darin says, accessibility should probably have been an ENABLE, not a HAVE.
> Would be nice if the patch could address that.
> 
> Moreover, the #if PLATFORM(MAC) && HAVE(ACCESSIBILITY) change doesn't make much
> sense. If you don't want accessibility in your port, you probably don't want to
> compile the Accessibility* classes in the first place. Can you try wrapping the
> entire classes with HAVE/ENABLE(ACCESSIBILITY) guards instead?

(In reply to comment #5)
> (In reply to comment #2)
> That makes sense, though the purpose is to suppress the underlying capability
> at build time.  I don't object to adding an ENABLE flag as well and requiring
> both to be set, but it seems unnecessarily verbose at first glance (since it
> would just change every occurrence of "HAVE(ACCESSIBILITY)" to
> "HAVE(ACCESSIBILITY) && ENABLE(ACCESSIBILITY)".

See Bug 21802.  I think the underlying assumption is that most (all?) platforms have some kind of an accessibility layer, so changing HAVE(ACCESSIBILITY) to ENABLE(ACCESSIBILITY) is the desired fix.

I don't think we want to be sprinkling "#if HAVE(ACCESSIBILITY) && ENABLE(ACCESSIBILITY)" everywhere, which seems redundant.  :)

[David Kilzer (:ddkilzer)]

See also Bug 22137.

[Darin Adler]

(In reply to comment #6)
> I think the underlying assumption is that most (all?) platforms
> have some kind of an accessibility layer, so changing HAVE(ACCESSIBILITY) to
> ENABLE(ACCESSIBILITY) is the desired fix.

I'd say it differently. I'd say that we expect people to only enable accessibility on platforms that have an accessibility layer, and thus we don't need any indication of the presence or absence of the layer.