[webkit-reviews] review denied: [Bug 91073] Add accessible for QWebView. : [Attachment 151926] 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 91073: Add accessible for QWebView.
https://bugs.webkit.org/show_bug.cgi?id=91073

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151926&action=review


A few comments. I like the approach of doing this in small steps, this being
the first one. Let's do one more round because of the coding style and the
accesslbeInterfaceFactory function placement. I think ideally it should be
static somewhere, alternatively it should be namespaced.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:28
> +QAccessibleInterface* accessibleInterfaceFactory(const QString& key,
QObject* object)

Would it make perhaps sense to move this into qwebpage.cpp where it is
called/used and make it a static function there?

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:32
> +    if (QWebPage *page = qobject_cast<QWebPage*>(object))

Coding style nitpick, * placement :)

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:35
> +    if (QWebView *view = qobject_cast<QWebView*>(object))

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:38
> +    if (QWebFrame *frame = qobject_cast<QWebFrame*>(object))

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:71
> +//	 WebCore::Document* document = frame()->d->frame->document();
> +//	 if (!document || !document->axObjectCache())
> +//	     return 0;
> +//	 WebCore::AccessibilityObject* rootAccessible =
document->axObjectCache()->rootObject();
> +//	 return rootAccessible ? 1 : 0;

We probably shouldn't land the code that is commented out.