Summary: | Accessibility: Implement isSelected in DRT for GTK | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||
Component: | Accessibility | Assignee: | Joanmarie Diggs <jdiggs> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, jmalonzo, mario, webkit.review.bot, xan.lopez | ||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | 38648 | ||||||||||||
Bug Blocks: | 25531 | ||||||||||||
Attachments: |
|
Description
chris fleizach
2009-11-02 10:31:00 PST
(In reply to comment #0) > This will allow the aria-tab-controls.html layout test to pass Hmmm... do you mean accessibility/aria-controls-with-tabs.html? Can't find aria-tab-controls.html anywhere yes that is what i meant Created attachment 55266 [details] Patch to fix this bug (In reply to comment #2) > yes that is what i meant Ok, attaching then a patch that would fix the issue. Please notice that fixing blocker bug 38648 first is mandatory for this to work (otherwise, the isSelected implementation would be always returning false, as m_element wouldn't be a valid AtkObject) Attachment 55266 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:367: Declaration has space between type name and * in AtkStateSet *stateSet [whitespace/declaration] [3]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:373: Extra space before ( in function call [whitespace/parens] [4]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:374: Extra space before ( in function call [whitespace/parens] [4]
WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:375: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 4 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 55268 [details] Patch to fix this bug (In reply to comment #4) > Attachment 55266 [details] did not pass style-queue: > Sorry, forgot to check style. Uploading new patch Comment on attachment 55268 [details]
Patch to fix this bug
LGTM.
Don't we have some sort of GRefPtr so we don't have to manually call g_object_unref?
Comment on attachment 55268 [details]
Patch to fix this bug
Actually, we don't do c-style forward declarations of variables. Please declare them inline.
Created attachment 55526 [details] Patch to fix this bug (In reply to comment #7) > (From update of attachment 55268 [details]) > Actually, we don't do c-style forward declarations of variables. Please declare them inline. Done. (In reply to comment #6) > (From update of attachment 55268 [details]) > LGTM. > > Don't we have some sort of GRefPtr so we don't have to manually call g_object_unref? I'm not sure that would be aplicable on here, since the extra ref comes from ATK lib, but not a strong opinion anyway. Glad to hear suggestions :-) Comment on attachment 55526 [details]
Patch to fix this bug
re GRefPtr, it does not really matter that you are not adding the ref yourself, what GRefPtr does is get rid of the reference when the object goes out of scope. You'd use it like:
GRefPtr<AtkStateSet> stateSet = atk_object_ref_state_set() ...
and then you can get rid of the g_object_unref.
I think it makes sense to use it, so r- because of that. Also, you can probably add the bug URL and title to the ChangeLog in the layout tests.
Created attachment 55581 [details] Patch to fix this bug (In reply to comment #9) > (From update of attachment 55526 [details]) > re GRefPtr, it does not really matter that you are not adding the ref yourself, what GRefPtr does is get rid of the reference when the object goes out of scope. You'd use it like: > > GRefPtr<AtkStateSet> stateSet = atk_object_ref_state_set() ... > > and then you can get rid of the g_object_unref. > > I think it makes sense to use it, so r- because of that. Also, you can probably add the bug URL and title to the ChangeLog in the layout tests. Done. Comment on attachment 55581 [details]
Patch to fix this bug
Looks good to me.
Comment on attachment 55581 [details] Patch to fix this bug Rejecting patch 55581 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55581, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: 5581&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=31018&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 55581 from bug 31018. NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Comment on attachment 55581 [details]
Patch to fix this bug
I believe this should work now.
Comment on attachment 55581 [details] Patch to fix this bug Clearing flags on attachment: 55581 Committed r59531: <http://trac.webkit.org/changeset/59531> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/59531 might have broken GTK Linux 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/59530 http://trac.webkit.org/changeset/59531 http://trac.webkit.org/changeset/59532 http://trac.webkit.org/changeset/59533 |