[webkit-reviews] review denied: [Bug 77864] [Qt] Initial implementation of accessibility support : [Attachment 125614] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Frederik Gladhorn
<frederik.gladhorn at nokia.com>'s request for review:
Bug 77864: [Qt] Initial implementation of accessibility support
https://bugs.webkit.org/show_bug.cgi?id=77864

Attachment 125614: Patch
https://bugs.webkit.org/attachment.cgi?id=125614&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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?