Summary: | [GTK] Please expose the level of headings. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | apinheiro, commit-queue, walker.willie, xan.lopez | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | 21546 | ||||||||||
Bug Blocks: | 25531 | ||||||||||
Attachments: |
|
Description
Joanmarie Diggs
2009-04-26 16:41:51 PDT
Created attachment 29806 [details]
aforementioned test case
Created attachment 41355 [details]
proposed patch - take 1
I decided to take a stab at an easier one. :-)
This works for me. Xan, please review and give pointers. Thanks in advance!!
Comment on attachment 41355 [details] proposed patch - take 1 This patch is amazing, great job! A few small comments: > +static AtkAttributeSet* addAttributeToSet(AtkAttributeSet* attributeSet, const char* name, const char* value) > +{ > + AtkAttribute* attribute = (AtkAttribute*)g_malloc(sizeof(AtkAttribute)); WebKit style guide tells us to use C++ style castings, so you need to do something like: AtkAttribute* attribute = static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute))); > + attribute->name = g_strdup(name); > + attribute->value = g_strdup(value); > + attributeSet = g_slist_prepend(attributeSet, attribute); > + > + return attributeSet; > +} > + > +static AtkAttributeSet* webkit_accessible_get_attributes(AtkObject* object) > +{ > + AtkAttributeSet* attributeSet = NULL; > + > + int headingLevel = core(object)->headingLevel(); > + if (headingLevel) { > + String value = String::number(headingLevel); > + attributeSet = addAttributeToSet(attributeSet, (const char*)"level", returnString(value)); - Is the (const char*) casting for "level" really needed? I think it should be const char* already. - Using returnString() here is a tiny hack, since the function was really intended for return values in public APIs implementations. Do thinks work if you pass value.utf8().data() to the function? I'm marking it as r- while we work on those final details. Thanks for the feedback, Xan! Notes (mostly to myself so that I remember them after I get some sleep. :-) ) > WebKit style guide tells us to use C++ style castings, so you need to do > something like: > > AtkAttribute* attribute = > static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute))); When I try that I get an error: ~~~~~~ WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: In function ‘AtkAttributeSet* addAttributeToSet(AtkAttributeSet*, const char*, const char*)’: WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:179: error: no matching function for call to ‘_AtkAttribute::_AtkAttribute(void*)’ ~~~~~~ But I'm new to C++. I bet if I research it and think about it, it'll all become clear. I'll do that "tomorrow." > - Is the (const char*) casting for "level" really needed? I think it should be > const char* already. You are correct. My bad. > - Using returnString() here is a tiny hack, since the function was really > intended for return values in public APIs implementations. Do thinks work if > you pass value.utf8().data() to the function? They do indeed. Cool. > I'm marking it as r- while we work on those final details. Thanks much for the review! I'll look into the first issue/error after some sleep. :-) (In reply to comment #4) > Thanks for the feedback, Xan! > > Notes (mostly to myself so that I remember them after I get some sleep. :-) ) > > > WebKit style guide tells us to use C++ style castings, so you need to do > > something like: > > > > AtkAttribute* attribute = > > static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute))); > > When I try that I get an error: > > ~~~~~~ > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: In function > ‘AtkAttributeSet* addAttributeToSet(AtkAttributeSet*, const char*, const > char*)’: > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:179: error: no > matching function for call to ‘_AtkAttribute::_AtkAttribute(void*)’ > ~~~~~~ > > But I'm new to C++. I bet if I research it and think about it, it'll all become > clear. I'll do that "tomorrow." I'm sorry, there was a missing '*' in the static_cast, it should read static_cast<AtkAttribute*> :) Created attachment 41362 [details] proposed patch - take 2 (In reply to comment #5) > I'm sorry, there was a missing '*' in the static_cast, it should read > static_cast<AtkAttribute*> :) Heheh. Even *I* see that NOW. :-) I really, really needed to sleep. But if I didn't see it, I would have been embarrassed when that turned out to be the "issue," so thanks for providing it. :-) This version makes that change along with the two others you pointed out. Comment on attachment 41362 [details]
proposed patch - take 2
woot, r=me
Comment on attachment 41362 [details] proposed patch - take 2 Clearing flags on attachment: 41362 Committed r49743: <http://trac.webkit.org/changeset/49743> All reviewed patches have been landed. Closing bug. |