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

[bugzilla-daemon at webkit.org]

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


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #125614|review?                     |review-
               Flag|                            |




--- Comment #3 from Simon Hausmann <hausmann at webkit.org>  2012-02-06 04:50:24 PST ---
(From update of attachment 125614)
View in context: https://bugs.webkit.org/attachment.cgi?id=125614&action=review

This is a nice start. Let's iterate :)

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

This line should be removed. You're just using re-implementing a missing platform interface.

> Source/WebKit/ChangeLog:8
> +        * WebKit.pri:

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?

> Source/WebCore/accessibility/AccessibilityObject.h:723
> +    AccessibilityObjectWrapper *m_wrapper;

Coding style, the * placement is wrong.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:40
> +        //     case WebCore::AnnotationRole:
> +        //         return QAccessible::;

I suppose you could remove one level of indentation here :)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:282
> +int AccessibilityObjectWrapper::indexOfChild(const QAccessibleInterface *childIface) const

Coding style (* placement)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:284
> +    const AccessibilityObjectWrapper *childWrapper = static_cast<const AccessibilityObjectWrapper*>(childIface);

Ditto.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:293
> +    // FIXME use hit-testing code from AccessibilityObject

Missing period at the end of the sentence. Use // FIXME: Full sentence here.

> 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?

Who owns the returned object?

> 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.

> 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/qwebview.cpp:1113
> +

Stray newline?

> Source/WebKit/qt/Api/qwebview.h:40
> +
> +
> +

Stray newlines?

> 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.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:50
> +    QWebFrameAccessible(QWebFrame *frame)

Coding style (* placement).

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:57
> +    QWebFrame *frame() const

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:67
> +    QAccessibleInterface *parent() const

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:72
> +    QAccessibleInterface *child(int index) const

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:85
> +        WebCore::AccessibilityObject *a = frame()->d->frame->document()->axObjectCache()->rootObject();

Ditto.

> 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?

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