[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 #4 from Frederik Gladhorn <frederik.gladhorn at nokia.com>  2012-02-06 05:40:29 PST ---
Thanks for the review. I fixed the trivial stuff and I'll have to check a few points below.

> Append to the line that you added new files to the build.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:667
> > +#elif PLATFORM(QT)
> > +    AccessibilityObjectWrapper* wrapper() const { return m_wrapper; }
> > +    void setWrapper(AccessibilityObjectWrapper* wrapper)
> > +    {
> > +        m_wrapper = wrapper;
> > +    }
> 
> Where do you call setWrapper? What is the ownership model?

Actually I'm not using it at all. I might be able to remove it completely since currently I only wrap the WebCore::AccessibilityObject into the QAccessibleInterface.
Qt manages the QAccessibleInterfaces and doesn't keep them persistent.


> > Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:323
> > +                    QAccessibleInterface* webView = QAccessible::queryAccessibleInterface(pageClient->ownerWidget());
> > +                    QAccessibleInterface* webPage = webView->child(0);
> > +                    QAccessibleInterface* webFrame = webPage->child(0);
> > +
> > +                    delete webView;
> > +                    delete webPage;
> 
> Would it perhaps be nicer to put webView and webPage into an OwnPtr or a QScopedPointer?
> 
> > Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:330
> > +    return new AccessibilityObjectWrapper(m_object->parentObjectUnignored()); // FIXME 0 is wrong
> 
> What's 0?
This was an old comment when I was debugging the initial version.
> 
> Who owns the returned object?
I don't like the way it's done in the Qt apis, but the caller owns the object, it's the same for parent/child and other calls.

> 
> > Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:334
> > +// this function is based on the gtk version of the same
> > +QString AccessibilityObjectWrapper::text(QAccessible::Text t) const
> 
> I think this code should be moved somewhere where it can be shared then, taking a platform-independent enum as argument and returning a String. Then this function here can call the new shared helper function and the String will be automatically converted to a QString.

I agree.

> > Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:37
> > +#include <qglobal.h>
> 
> Is it necessary to include qglobal.h? Doesn't qaccessible.h do that implicitly?
> 
> > Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:70
> > +    virtual void setText(QAccessible::Text, const QString&)
> > +    {
> > +    }
> 
> Instead of an inline implementation, should this perhaps be placed in the .cpp with notImplemented(); in it?
> 

> > Source/WebKit/qt/Api/qwebviewaccessible_p.h:48
> > +class QWebFrameAccessible : public QAccessibleObject {
> 
> I suggest to place the implementation of the class into a .cpp file instead of it being entirely inline.
Yes, now that it's working that makes a lot of sense.

> > Source/api.pri:174
> > +contains(QT_CONFIG, accessibility) {
> > +    DEFINES += ACCESSIBILITY=1
> > +}
> 
> Did you check if the code still compiles with Qt 4.8? Could it be that you want a haveQt(5) or so before the contains?
In theory it's no big deal to get it working with Qt 4 also, but I don't think it's as interesting as moving to WK2.
So I'll put the haveQt(5) in there for now.

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