[Webkit-unassigned] [Bug 77864] [Qt] Initial implementation of accessibility support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77864





--- Comment #18 from Mario Sanchez Prada <msanchez at igalia.com>  2012-02-07 07:17:12 PST ---
(In reply to comment #17)
>[...]
> For now (debugging and getting this to run at all, I consciously include everything.
> Filtering this in a sensible way is on my todo and I'll check the GTK implementation again.

Seems sensible to me. Just wanting to make sure it was done on purpose.

> Right now I feel that it's too much for one patch. This will come with implementing states and other interfaces which is also missing.

I also had that feeling, but I also understand that for start iterating over it having it this way is not that bad. Perhaps you could think of ways to splitting it in some way in the future... maybe one patch for the changes in WebCore and another one for the changes in the WebKit/qt layer?

> >> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:30
> >> +*/
> > 
> > Wrong coding style. Place a '*' at the beginning of every line.
> 
> I don't see this in the style guide lines. I don't see the point of adding those stars... I can if it makes you happy though.

I don't think this is in the coding style at all since, after all, C coments '/* ... */' are not the ones that should be used in WebKit's C++ code base in general.

I just pointed that out because that's what it's being done anyway in other files in WebCore (check headers in dom/ accessibility/ node/ ...  and so forth), and so I thought it was a good thing to do here as well, for the sake of consistency.

> >> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:54
> >> +    //     case AnnotationRole:
> > 
> > Only one space between the // and 'case'. Further commented lines should be the same way, respecting the indentation of course.
> 
> This is one of the next things to sort out anyway. I'd rather do it in a follow up commit though.

Ok.

> >> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:347
> >> +    case QAccessible::Name: {
> > 
> > Why are you using '{'/'}' in this 'case' entry?
> 
> Because I declare String axTitle inside. If you don't have the parenthesis, you get "jump to case label/crosses initialization".

When that kind of things happen, the usual the way to go is to pre-declare those variables you need before the switch statement, I think. I know it's probably not the ideal thing but violating the coding style is not a good option either.

However, in this case I'd say the right one after all would be to move all the code in 'case QAccessible::Name:' out to a separate private function, so you just call it from there. Also, if you move it out to a separate function you will probably gain another advantage: you can probably use early returns to get rid of the 3-level of indentation in those 'if' blocks in the middle. Looks like a win-win to me :)

> >> Source/WebKit/qt/Api/qwebviewaccessible.cpp:73
> >> +    WebCore::AccessibilityObject* a = frame()->d->frame->document()->axObjectCache()->rootObject();
> > 
> > You probably want to add a couple of checks here: at least one for the result of frame->document(), and another for document->axObjectCache().
> > 
> > Also, 'a' is not a good name for a variable in WebKit. You should use something meaningful instead. Looks like 'rootObject', 'rootAccessibilityObject' or 'axRootObject' (this 'ax' prefix is kind of a convention in a11y related code) might work fine.
> > 
> > Last, I have no idea about QWebFrame, but I wonder if the 'frame()->d' statement could be replaced for something more understandable :)
> 
> d is just the pimpl, anyone knowing Qt knows what it's about.

Ok, as I said I have no idea about QWebFrame, so it makes sense to you I'm fine too.

> I agree about the variable names and checks though.

Great.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.