WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 103036
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
https://bugs.webkit.org/show_bug.cgi?id=103036
Summary
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Krzysztof Czech
Reported
2012-11-22 01:09:31 PST
Possibility to turn off accessibility feature for WebKit-EFL.
Attachments
proposed patch
(3.69 KB, patch)
2012-12-03 14:58 PST
,
Laszlo Gombos
kenneth
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(15.46 KB, patch)
2012-12-10 07:21 PST
,
Krzysztof Czech
webkit-ews
: commit-queue-
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(15.63 KB, patch)
2012-12-10 07:53 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(15.63 KB, patch)
2012-12-10 08:00 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(15.67 KB, patch)
2012-12-10 08:54 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(13.62 KB, patch)
2012-12-11 03:36 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(11.95 KB, patch)
2012-12-12 08:21 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
(11.87 KB, patch)
2012-12-12 08:53 PST
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Laszlo Gombos
Comment 1
2012-12-03 14:58:03 PST
Created
attachment 177339
[details]
proposed patch I had this to change in my environment as I needed it to proceed - I decided to share it.
WebKit Review Bot
Comment 2
2012-12-03 23:22:08 PST
Comment on
attachment 177339
[details]
proposed patch Rejecting
attachment 177339
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ripts/update-webkit line 152. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource From
http://git.chromium.org/external/Webkit
9961a7b..ef0ca77 HEAD -> origin/HEAD error: Ref refs/remotes/origin/master is at ef0ca77a554ed2a07b001ee4935266d6301b2bf8 but expected 9961a7b754078ea893b086fbc787c57745c8a4db ! 9961a7b..ef0ca77 master -> origin/master (unable to update local ref) Died at Tools/Scripts/update-webkit line 152. Full output:
http://queues.webkit.org/results/15101901
Krzysztof Czech
Comment 3
2012-12-04 03:45:27 PST
We will not turn off accessibility (ATK) only by removing ATK from building process. There is still ATK dependant code in WebCore. Global HAVE_ACCESSIBILITY macro is always defined for EFL and some specific ATK code is covered by this macro. Turning off Accessibility completely requires some additional changes in WebCore. I started doing this part but there are some things that should be considered.
Laszlo Gombos
Comment 4
2012-12-04 14:04:25 PST
(In reply to
comment #3
)
> We will not turn off accessibility (ATK) only by removing ATK from building process. There is still ATK dependant code in WebCore. Global HAVE_ACCESSIBILITY macro is always defined for EFL and some specific ATK code is covered by this macro. Turning off Accessibility completely requires some additional changes in WebCore. I started doing this part but there are some things that should be considered.
Thanks Krzysztof, I just noticed as well that my patch is not complete. As the patch is not landed yet, I will make it obsolete and please feel free to pick it up from where I left it and make it complete. For my need the most important things is to have a build option to disable the ATK dependent parts (when ATK is not available). It is not a strictly needed to disable all the accessibility bits, but certainly that would be best solution.
Krzysztof Czech
Comment 5
2012-12-10 07:21:29 PST
Created
attachment 178547
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
WebKit Review Bot
Comment 6
2012-12-10 07:25:30 PST
Attachment 178547
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/PlatformEfl.cmake:349: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] Source/WebCore/accessibility/AccessibilityObject.h:785: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 7
2012-12-10 07:28:04 PST
Comment on
attachment 178547
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Attachment 178547
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/15230635
Early Warning System Bot
Comment 8
2012-12-10 07:30:30 PST
Comment on
attachment 178547
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Attachment 178547
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/15231505
Build Bot
Comment 9
2012-12-10 07:49:57 PST
Comment on
attachment 178547
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Attachment 178547
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/15228646
Krzysztof Czech
Comment 10
2012-12-10 07:53:28 PST
Created
attachment 178552
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
WebKit Review Bot
Comment 11
2012-12-10 07:56:58 PST
Attachment 178552
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1 Source/WebCore/PlatformEfl.cmake:349: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] Source/WebCore/accessibility/AccessibilityObject.h:785: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Krzysztof Czech
Comment 12
2012-12-10 08:00:23 PST
Created
attachment 178553
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Krzysztof Czech
Comment 13
2012-12-10 08:47:36 PST
I added some additional bits so that we can completely turn off the Accessibility feature for EFL. Modifications in WebCore/PlatformEfl.cmake may not give us desired effect. If we turn off a11y by only disabling it in WebCore/PLatformEfl.cmake, compilation errors appear due to the fact, there are ATK routines left in WebKit/accessibility/. I added additional macro ENABLE_ACCESSIBILITY_EFL so that we can play with those ATK routines.
Krzysztof Czech
Comment 14
2012-12-10 08:54:33 PST
Created
attachment 178562
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
chris fleizach
Comment 15
2012-12-10 08:55:31 PST
Comment on
attachment 178553
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. there's already a HAVE(ACCESSIBILITY) macro. can you use that?
Gyuyoung Kim
Comment 16
2012-12-10 19:45:10 PST
Comment on
attachment 178562
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. View in context:
https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> Source/cmake/OptionsEfl.cmake:217 > + add_definitions(-DENABLE_ACCESSIBILITY_EFL=1)
I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
Laszlo Gombos
Comment 17
2012-12-10 20:11:27 PST
Comment on
attachment 178562
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. View in context:
https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> Source/WebCore/accessibility/AXObjectCache.h:273 > +#if (PLATFORM(EFL) && !ENABLE(ACCESSIBILITY_EFL))
Would it be possible to simply use "#if !HAVE(ACCESSIBILITY)" here and combine this list with the list above" ?
> Source/cmake/OptionsEfl.cmake:51 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON) > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_3D_RENDERING ON) > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ANIMATION_API ON)
It would be good to keep this list sorted.
>> Source/cmake/OptionsEfl.cmake:217 >> + add_definitions(-DENABLE_ACCESSIBILITY_EFL=1) > > I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
I think this should be changed to add_definitions(-DHAVE_ACCESSIBILITY=1)
Krzysztof Czech
Comment 18
2012-12-11 03:36:31 PST
Created
attachment 178768
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Krzysztof Czech
Comment 19
2012-12-11 03:45:41 PST
(In reply to
comment #17
)
> (From update of
attachment 178562
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> > > Source/WebCore/accessibility/AXObjectCache.h:273 > > +#if (PLATFORM(EFL) && !ENABLE(ACCESSIBILITY_EFL)) > > Would it be possible to simply use "#if !HAVE(ACCESSIBILITY)" here and combine this list with the list above" ? > > > Source/cmake/OptionsEfl.cmake:51 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ACCESSIBILITY ON) > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_3D_RENDERING ON) > > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_ANIMATION_API ON) > > It would be good to keep this list sorted. > > >> Source/cmake/OptionsEfl.cmake:217 > >> + add_definitions(-DENABLE_ACCESSIBILITY_EFL=1) > > > > I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ? > > I think this should be changed to add_definitions(-DHAVE_ACCESSIBILITY=1)
You are right Laszlo, add_definitions(-DHAVE_ACCESSIBILITY=1) it's a good idea. Having this in OptionsEfl.cmake give us possibility to switch on/off WebKit-EFL accessibility. I moved its declaration for EFL platform to OptionsEfl.cmake.
Krzysztof Czech
Comment 20
2012-12-11 03:51:34 PST
(In reply to
comment #16
)
> (From update of
attachment 178562
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=178562&action=review
> > > Source/cmake/OptionsEfl.cmake:217 > > + add_definitions(-DENABLE_ACCESSIBILITY_EFL=1) > > I think ACCESSIBILITY_EFL is not needed. PLATFORM(EFL) && ENABLE(ACCESSIBILITY) is enough. BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
Yes I agree, ACCESSIBILITY_EFL is not needed. Laszlo pointed out that HAVE_ACCESSIBILITY can be used instead.
> BTW, why should we use them together ? Can't we use PLATFORM(EFL) as GTK port ?
In some of the places PLATFORM(EFL) is not enough since ATK library is external lib and in sources there are some ATK routines. That's way we got to have second guard.
Krzysztof Czech
Comment 21
2012-12-11 03:52:37 PST
(In reply to
comment #15
)
> (From update of
attachment 178553
[details]
) > there's already a HAVE(ACCESSIBILITY) macro. can you use that?
Yes I can use this macro. I corrected my patch.
Laszlo Gombos
Comment 22
2012-12-11 15:33:53 PST
Comment on
attachment 178768
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. View in context:
https://bugs.webkit.org/attachment.cgi?id=178768&action=review
Overall looks good to me, but it would be good to fix the remaining minor issues.
> Source/WTF/wtf/Platform.h:674 > -#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID)) || PLATFORM(EFL) > +#if PLATFORM(IOS) || PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && !OS(ANDROID))
I prefer to keep PLATFORM(EFL) listed here as that is still the default for EFL. We can still disable ACCESSIBILITY (see the comment below). If you remove this change than you do not need to change WTF.
> Source/WebCore/accessibility/AccessibilityObject.h:784 > -#elif !PLATFORM(CHROMIUM) > +#elif !PLATFORM(CHROMIUM)
Extra white space. This change is not needed.
> Source/cmake/OptionsEfl.cmake:218 > +if (ENABLE_ACCESSIBILITY) > + find_package(ATK REQUIRED) > + add_definitions(-DHAVE_ACCESSIBILITY=1) > +endif ()
I think it is better to have the default in .h file and have the non-typical option (the ability to disable) in the make system. What do you think of the following: if (ENABLE_ACCESSIBILITY) find_package(ATK REQUIRED) else () add_definitions(-DHAVE_ACCESSIBILITY=0) endif ()
Krzysztof Czech
Comment 23
2012-12-12 08:21:02 PST
Created
attachment 179052
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Krzysztof Czech
Comment 24
2012-12-12 08:53:48 PST
Created
attachment 179058
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL.
Laszlo Gombos
Comment 25
2012-12-12 09:01:51 PST
Comment on
attachment 179058
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. lgtm.
WebKit Review Bot
Comment 26
2012-12-12 09:32:02 PST
Comment on
attachment 179058
[details]
[EFL] Possibility to turn off accessibility feature for WebKit-EFL. Clearing flags on attachment: 179058 Committed
r137475
: <
http://trac.webkit.org/changeset/137475
>
WebKit Review Bot
Comment 27
2012-12-12 09:32:09 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug