Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
On Sat, Nov 17, 2018 at 3:45 AM Ryosuke Niwa wrote: > I think the right macro to use here would have HAVE(ACCESSIBILITY). It > never makes sense to compile out accessibility support if you have the > support in a given platform/port. The question is really whether a given > port / platform has the support for accessibility or not. > > Thank you for the feedback. Let's keep HAVE_ACCESSIBILITY macro. Can I remove ENABLE_ACCESSIBILITY CMake variable? It is used only in WebKitTestRunner. https://github.com/WebKit/webkit/blob/dea6e0141a5df7b678221d4a474b5846176a913d/Tools/WebKitTestRunner/CMakeLists.txt#L76 Do you want to keep this condition by renaming it to HAVE_ACCESSIBILITY CMake variable? > if (HAVE_ACCESSIBILITY) We can not keep this code by using platform variable, for example WTF_PLATFORM_WIN_CAIRO, because no port is using the code. > if (WTF_PLATFORM_WIN_CAIRO) Or, keep it by doing following? > if (FALSE) Bug 191831 – [CMake] Remove ENABLE_ACCESSIBILITY CMake variable https://bugs.webkit.org/show_bug.cgi?id=191831 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
> On Nov 16, 2018, at 10:44 AM, Ryosuke Niwa wrote: > > If a stub implementation is ok for ports without an actual accessibility > implementation then I’d say remove both flags. Then downstream users would > never have to worry about breaking due to AX signatures changing. > > > I don't think a stub implementation is okay. Removing ENABLE(ACCESSIBILITY) would be fine. Either way, I do think we need a better way to maintain !HAVE(ACCESSIBILITY). There have been at least ~38 build fixes for !HAVE(ACCESSIBILITY) over the years — one every 2-3 months on average. It was one of the pain points I remember in maintaining an outside port. https://trac.webkit.org/search?q=%21HAVE%28ACCESSIBILITY%29 — Timothy Hatcher ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
On Thu, Nov 15, 2018 at 10:59 AM wrote: > It’s weird that we have ENABLE(ACCESSIBILITY) and HAVE(ACCESSIBILITY) so > at the very least one of these should go away, probably HAVE. > > > > Currently there are actual implementations for iOS, Mac, Windows, and ATK. > The WPE implementation is just a stub with everything notImplemented. I did > some work to make the WPE implementation just be the default at > https://bugs.webkit.org/show_bug.cgi?id=190608 but it broke a build so I > rolled it out and haven’t returned to it yet so this is good timing for the > discussion as that’ll influence what needs to be done with that. > > > > If a stub implementation is ok for ports without an actual accessibility > implementation then I’d say remove both flags. Then downstream users would > never have to worry about breaking due to AX signatures changing. > I don't think a stub implementation is okay. If not my vote is to just have ENABLE(ACCESSIBILITY) since it can then be > toggled on and off with webkit-build. A company like Tesla could always > provide a buildbot that turns off accessibility on a public port if this is > a real pain point since all public ports have it enabled. > I think the right macro to use here would have HAVE(ACCESSIBILITY). It never makes sense to compile out accessibility support if you have the support in a given platform/port. The question is really whether a given port / platform has the support for accessibility or not. - R. Niwa *From:* webkit-dev *On Behalf Of *Timothy > Hatcher > *Sent:* Wednesday, November 14, 2018 7:06 PM > *To:* Ryosuke Niwa > *Cc:* WebKit Development > *Subject:* Re: [webkit-dev] Remove HAVE_ACCESSIBILITY > > > > It wasn’t added for Tesla. But they did build with it disabled at the time > I lasted worked on it. It was a frequent pain point to keep the build > working when AX changes happened. > > — Timothy Hatcher > > > On Nov 14, 2018, at 5:29 PM, Ryosuke Niwa wrote: > > I think it was added for Telsa's private port. Probably not worth > maintaining the flag if the maintenance cost is high but is it? > > > - R. Niwa > > > > On Wed, Nov 14, 2018 at 5:19 PM Fujii Hironori > wrote: > > Hi webkit-dev, > > > > It seems that all port defines HAVE_ACCESSIBILITY=1. Can I remove all code > for !HAVE(ACCESSIBILITY)? > > > > > https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Platform.h?rev=237992#L648 > > > > #if PLATFORM(COCOA) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(WPE) > > #define HAVE_ACCESSIBILITY 1 > > #endif > > > > Bug 21802 – Rename HAVE_ACCESSIBILITY to ENABLE_ACCESSIBILITY > > https://bugs.webkit.org/show_bug.cgi?id=21802 > > > > -- Fujii > > > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > > ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
It’s weird that we have ENABLE(ACCESSIBILITY) and HAVE(ACCESSIBILITY) so at the very least one of these should go away, probably HAVE. Currently there are actual implementations for iOS, Mac, Windows, and ATK. The WPE implementation is just a stub with everything notImplemented. I did some work to make the WPE implementation just be the default at https://bugs.webkit.org/show_bug.cgi?id=190608 but it broke a build so I rolled it out and haven’t returned to it yet so this is good timing for the discussion as that’ll influence what needs to be done with that. If a stub implementation is ok for ports without an actual accessibility implementation then I’d say remove both flags. Then downstream users would never have to worry about breaking due to AX signatures changing. If not my vote is to just have ENABLE(ACCESSIBILITY) since it can then be toggled on and off with webkit-build. A company like Tesla could always provide a buildbot that turns off accessibility on a public port if this is a real pain point since all public ports have it enabled. - Don From: webkit-dev On Behalf Of Timothy Hatcher Sent: Wednesday, November 14, 2018 7:06 PM To: Ryosuke Niwa Cc: WebKit Development Subject: Re: [webkit-dev] Remove HAVE_ACCESSIBILITY It wasn’t added for Tesla. But they did build with it disabled at the time I lasted worked on it. It was a frequent pain point to keep the build working when AX changes happened. — Timothy Hatcher On Nov 14, 2018, at 5:29 PM, Ryosuke Niwa mailto:rn...@webkit.org>> wrote: I think it was added for Telsa's private port. Probably not worth maintaining the flag if the maintenance cost is high but is it? - R. Niwa On Wed, Nov 14, 2018 at 5:19 PM Fujii Hironori mailto:fujii.hiron...@gmail.com>> wrote: Hi webkit-dev, It seems that all port defines HAVE_ACCESSIBILITY=1. Can I remove all code for !HAVE(ACCESSIBILITY)? https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Platform.h?rev=237992#L648 #if PLATFORM(COCOA) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(WPE) #define HAVE_ACCESSIBILITY 1 #endif Bug 21802 – Rename HAVE_ACCESSIBILITY to ENABLE_ACCESSIBILITY https://bugs.webkit.org/show_bug.cgi?id=21802 -- Fujii ___ webkit-dev mailing list webkit-dev@lists.webkit.org<mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org<mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
On Wed, Nov 14, 2018 at 7:49 PM, Fujii Hironori wrote: No, it isn't high. It is no problem to keep the code. CMake defines the similar name macro ENABLE_ACCESSIBILITY=0 in generated cmakeconfig.h. https://trac.webkit.org/browser/webkit/trunk/Source/cmake/WebKitFeatures.cmake#L88 This confuses me. I want to fix. I would remove the flag if it's enabled for all ports. Michael ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
On Thu, Nov 15, 2018 at 12:05 PM Timothy Hatcher wrote: > It wasn’t added for Tesla. But they did build with it disabled at the time > I lasted worked on it. It was a frequent pain point to keep the build > working when AX changes happened. > > For the record, HAVE_ACCESSIBILITY has been introduced in the followoing commits. https://trac.webkit.org/changeset/31986 https://trac.webkit.org/changeset/32042 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
It wasn’t added for Tesla. But they did build with it disabled at the time I lasted worked on it. It was a frequent pain point to keep the build working when AX changes happened. — Timothy Hatcher > On Nov 14, 2018, at 5:29 PM, Ryosuke Niwa wrote: > > I think it was added for Telsa's private port. Probably not worth maintaining > the flag if the maintenance cost is high but is it? > > - R. Niwa > >> On Wed, Nov 14, 2018 at 5:19 PM Fujii Hironori >> wrote: >> Hi webkit-dev, >> >> It seems that all port defines HAVE_ACCESSIBILITY=1. Can I remove all code >> for !HAVE(ACCESSIBILITY)? >> >> https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Platform.h?rev=237992#L648 >> >> #if PLATFORM(COCOA) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(WPE) >> #define HAVE_ACCESSIBILITY 1 >> #endif >> >> Bug 21802 – Rename HAVE_ACCESSIBILITY to ENABLE_ACCESSIBILITY >> https://bugs.webkit.org/show_bug.cgi?id=21802 >> >> -- Fujii >> >> ___ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org >> https://lists.webkit.org/mailman/listinfo/webkit-dev > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
On Thu, Nov 15, 2018 at 10:29 AM Ryosuke Niwa wrote: > I think it was added for Telsa's private port. Probably not worth > maintaining the flag if the maintenance cost is high but is it? > No, it isn't high. It is no problem to keep the code. CMake defines the similar name macro ENABLE_ACCESSIBILITY=0 in generated cmakeconfig.h. https://trac.webkit.org/browser/webkit/trunk/Source/cmake/WebKitFeatures.cmake#L88 This confuses me. I want to fix. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Remove HAVE_ACCESSIBILITY
I think it was added for Telsa's private port. Probably not worth maintaining the flag if the maintenance cost is high but is it? - R. Niwa On Wed, Nov 14, 2018 at 5:19 PM Fujii Hironori wrote: > Hi webkit-dev, > > It seems that all port defines HAVE_ACCESSIBILITY=1. Can I remove all code > for !HAVE(ACCESSIBILITY)? > > > https://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/Platform.h?rev=237992#L648 > > #if PLATFORM(COCOA) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(WPE) > #define HAVE_ACCESSIBILITY 1 > #endif > > Bug 21802 – Rename HAVE_ACCESSIBILITY to ENABLE_ACCESSIBILITY > https://bugs.webkit.org/show_bug.cgi?id=21802 > > -- Fujii > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev > ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
