Bug 77864 – [Qt] Initial implementation of accessibility support

• Component: New Bugs
• Status: RESOLVED INVALID

[Frederik Gladhorn]

[Qt] Initial implementation of accessibility support

[Frederik Gladhorn]

Created attachment 125614 [details]
Patch

[WebKit Review Bot]

Attachment 125614 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebKit/qt/Api/qwebviewaccessible.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/Api/qwebview.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:20:  #ifndef header guard has wrong style, please use: qwebviewaccessible_p_h  [build/header_guard] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:38:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:49:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:61:  The parameter name "relation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:74:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/AccessibilityObject.h:49:  "AccessibilityObjectWrapper.h" already included at Source/WebCore/accessibility/AccessibilityObject.h:47  [build/include] [4]
Total errors found: 9 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 125614 [details]
Patch

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?

[Frederik Gladhorn]

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.

[Frederik Gladhorn]

Building with Qt 4 actually works.

[Simon Hausmann]

(In reply to comment #5)
> Building with Qt 4 actually works.

Wicked :). Even one that has accessibility in QT_CONFIG?

[Frederik Gladhorn]

Created attachment 125635 [details]
Patch

[WebKit Review Bot]

Attachment 125635 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebKit/qt/Api/qwebviewaccessible_p.h:20:  #ifndef header guard has wrong style, please use: qwebviewaccessible_p_h  [build/header_guard] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:36:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:47:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:65:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:75:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebviewaccessible_p.h:83:  The parameter name "view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:48:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:60:  The parameter name "relation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/accessibility/qt/AccessibilityObjectWrapper.h:71:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frederik Gladhorn]

(In reply to comment #6)
> (In reply to comment #5)
> > Building with Qt 4 actually works.
> 
> Wicked :). Even one that has accessibility in QT_CONFIG?

Hehe, yes, it is in there by default. But the qt version checks for 5 do the trick. I'm not entirely sure if Qt currently builds with QT_NO_ACCESSIBILITY. But that's something to fix in qtbase if not.

[Simon Hausmann]

Comment on attachment 125635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125635&action=review

>> Source/WebKit/qt/Api/qwebviewaccessible_p.h:83
>> +    QWebViewAccessible(QWebView* view);
> 
> The parameter name "view" adds no information, so it should be removed.  [readability/parameter_name] [5]

So the meaning of this warning from the style queue is that you should simply remove the view parameter name, i.e.

    QWebViewAccessible(QWebView*);

(and you can use the parameter in the _implementation_ if you need it of course)

The rationale is that the "View" part of the type name is "redundant" with the "view" parameter name, it is a view anyway.

[Frederik Gladhorn]

Created attachment 125638 [details]
Patch

[WebKit Review Bot]

Attachment 125638 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebKit/qt/Api/qwebviewaccessible_p.h:20:  #ifndef header guard has wrong style, please use: qwebviewaccessible_p_h  [build/header_guard] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Frederik Gladhorn]

Created attachment 125780 [details]
Patch

[Frederik Gladhorn]

Created attachment 125795 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 125795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125795&action=review

Awesome patch, Frederik. Just performing an informal review now.

See my (hopefully useful) comments below...

> Source/WebCore/accessibility/AXObjectCache.cpp:300
> +

I would not include this kind of fixes in this patch (unless they are in the scope of an actual change), since it alters the diff in an unrelated way.

If you feel like fixing this kind of things, it's better to file a bug for that.

> Source/WebCore/accessibility/AccessibilityObject.h:310
> +

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.h:663
> +    void setWrapper(AccessibilityObjectWrapper* wrapper)

I probably would not change this either, but as it's so close to an actual change (#elif), I think it makes sense.

> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:37
> +    return IncludeObject;

Are you sure you want to do this here? Normally, this function is meant to give flexibility to some ports that do not want to go ahead with the "default behavior" for some very specific cases, this falling back to DefaultBehavior if the AccessibilityObject does not match none of those very specific cases.

What you're saying here is that, basically, you want to expose *any* WebCore's Accessibility object in Qt, even those that should probably not be exposed (e.g. those returning 'true' in ariaIsHidden() and isPresentationalChildOfAriaRole()).

Maybe it's what you want, though. Pointing it out just in case, as it's normally not that way (if you check the mac port you'll see they also ignore some specific objects).

> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:40
> +

Unneeded extra empty line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:30
> +*/

Wrong coding style. Place a '*' at the beginning of every line.

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

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:255
> +

Unneeded empty line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:339
> +    }

You should use early returns here to remove unneeded levels of indentation. Also, you should probably add some extra nullchecks, like checking if the document returned by m_object->document() is a valid one.

Something like this, more or less:

    [...]
    // Check if this is the root object. If so, we need to return the Frame as parent
    if (m_object->parentObjectUnignored() || !m_object->isScrollView())
        return 0;

    AccessibilityObject* firstChild = m_object->firstChild();
    if (!firstChild || !firstChild->isWebArea())
        return 0;

    // We seem to be the root accessible, get the Frame
    Document* document = m_object->document();
    if (!document)
        return 0;

    HostWindow* hostWindow = document->view()->hostWindow();
    if (!hostWindow)
        return 0;

    QWebPageClient* pageClient = hostWindow->platformPageClient();
    if (!pageClient || !pageClient->isQWidgetClient())
        return 0;

    // FIXME: Writing the same for QGraphicsView should be trivial.
    QScopedPointer<QAccessibleInterface> webView(QAccessible::queryAccessibleInterface(pageClient->ownerWidget()));
    QScopedPointer<QAccessibleInterface> webPage(webView->child(0));
    QAccessibleInterface* webFrame = webPage->child(0);
        return webFrame;

    [...]

Yeah, I know in the ATK wrapper there's atkParentOfRootObject(). We should change that there at some point too :-)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:343
> +// FXIME: Create a helper used by gtk and here

Typo (FXIME) + missing period at the end of the line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:347
> +    case QAccessible::Name: {

Why are you using '{'/'}' in this 'case' entry?

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.h:18
> +*/

Missing '*' at the beginning of every line.

> Source/WebKit/qt/Api/qwebpage.h:451
> +

Unneeded empty line?

> Source/WebKit/qt/Api/qwebview.cpp:173
> +// The QAccessible interfaces changed API in Qt 5

Missing period at the end of the line.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:18
> +*/

Missing '*' at the beginning of every line.

> 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 :)

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:97
> +    WebCore::AccessibilityObject* a = frame()->d->frame->document()->axObjectCache()->rootObject();

Same here.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:18
> +*/

Missing '*' at the beginning of every line.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:30
> +

This empty line is not needed.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:35
> +

Same here.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:62
> +

...and here :-)

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:80
> +

...and here too

[Frederik Gladhorn]

Created attachment 125837 [details]
Patch

[Frederik Gladhorn]

>> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:37
>> +    return IncludeObject;
> 
> Are you sure you want to do this here? Normally, this function is meant to give flexibility to some ports that do not want to go ahead with the "default behavior" for some very specific cases, this falling back to DefaultBehavior if the AccessibilityObject does not match none of those very specific cases.
> 
> What you're saying here is that, basically, you want to expose *any* WebCore's Accessibility object in Qt, even those that should probably not be exposed (e.g. those returning 'true' in ariaIsHidden() and isPresentationalChildOfAriaRole()).
> 
> Maybe it's what you want, though. Pointing it out just in case, as it's normally not that way (if you check the mac port you'll see they also ignore some specific objects).

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

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

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

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

>> 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.
I agree about the variable names and checks though.

[Mario Sanchez Prada]

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

[Simon Hausmann]

Comment on attachment 125837 [details]
Patch

One thing I'm a bit uneasy about now is the ownership.

AccessibilityObject is refcounted and it seems that on mac/win/chromium the AccessibilityObject also keeps a strong reference on the wrapper object.

This pattern is completely ignored right now, the Qt version of AccessibilityObjectWrapper keeps a raw pointer to the AccessibilityObject, which means it can become dangling at any time.

WTF::RefCounted does not support weak pointers, but there's the AXObjectCache along with the objects having AXIDs. Is this perhaps what we should be using instead?

So that our implementation of QAccessibleInterface* - which btw should probably live in WebKit/qt/WebCoreSupport - stores a weak pointer to the QWebPage as well as the AXID. Then when we need an AccessibilityObject we can look it up from the document's AXObjectCache through the ID.

Does that make sense?

[Mario Sanchez Prada]

Comment on attachment 125837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125837&action=review

Hi again. Thanks for iterating over the patch, this one looks better indeed.

Still, some issues remain, please see my comments below...

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:6
> +

You're gonna hate me for being such a nitpicker, but the convention used all around WebKit is to put an * at the beginning of _every_ line between the '/*' and the '*/', even if it's just an empty line (so only the ' *' string would be there, The same issue seems to be present in the rest of new files you're adding with this patch.

Just check other files' headers and you'll see what I mean :-)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:338
> +    }

As I said in comment #15, you should use early returns here to ger rid of this 4 levels of indentation.

Also, you should null-check the result of m_object->document() with a simple "if (document) return 0;"

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:342
> +// FXIME: Create a helper used by gtk and here.

Typo still here.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:346
> +    case QAccessible::Name: {

As I said earlier in comment #18, you should move this (too big) piece of code here to an external function, and use early returns there.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:45
> +
> +
> +

Two many empty lines here. Keep just one.

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

Thanks for adding the extra checks here, but you're calling axObjectCache() twice here and that's something that is normally avoided by putting the result in a variable and doing the check separately. Something like this would be better (even though it's longer):

   WebCore::Document* document = frame()->d->frame->document();
   if (!document)
      return 0;

   AXObjectCache* axObjectCache = document->axObjectCache();
   if (!axObjectCache)
      return 0;

   WebCore::AccessibilityObject* rootAccessible = axObjectCache->rootObject();

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

Ditto.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:167
> +
> +
> +

Two many empty lines.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:61
> +
> +

Remove one empty line.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:79
> +
> +

Ditto.

[Simon Hausmann]

Comment on attachment 125837 [details]
Patch

I agree with Mario and I had a chat with Frederik about the memory management. So let's do another iteration :)

[Frederik Gladhorn]

Thanks for all the reviewing so far :)
I also found that I had the HAVE_ACCESSIBILITY define in the wrong place so it was still not picked up actually.
I'll try to figure out the proper way to handle the memory management issues next.

[Mario Sanchez Prada]

Hi again, just wondering if there's any plan to keep working on this patch.

AFAIK Frederik started working on this on his spare time (please correct me if I'm wrong), so no intention here to put any pressure or so.

Just wondering, because this bug came to my mind recently due to other reasons. 

That's it :-)

[Simon Hausmann]

*** Bug 30626 has been marked as a duplicate of this bug. ***

[Laszlo Gombos]

*** Bug 36693 has been marked as a duplicate of this bug. ***

[Frederik Gladhorn]

Created attachment 180345 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180345 [details]
Patch

Attachment 180345 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15451074

[Early Warning System Bot]

Comment on attachment 180345 [details]
Patch

Attachment 180345 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15451075

[Simon Hausmann]

Comment on attachment 180345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180345&action=review

> Source/WTF/WTF.pri:42
> +haveQt(5): contains(QT_CONFIG,accessibility): DEFINES += HAVE_ACCESSIBILITY=1

This should probably go into features.pri.

> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:40
> +void AccessibilityObject::setWrapper(AccessibilityObjectWrapper *wrapper)

* placement

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:126
> +                    QScopedPointer<QAccessibleInterface> webView(QAccessible::queryAccessibleInterface(pageClient->ownerWidget()));

How do the other ports handle this? Is there a simpler or common way?

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.h:28
> +class QWebFrame;

That doesn't seem to be needed

> Source/WebKit/qt/WidgetApi/qwebviewaccessible.cpp:34
> +    if (!WebCore::AXObjectCache::accessibilityEnabled())

This class (qwebviewaccessible) needs to move from QtWebKitWidgets into QtWebKit. Try compiling with make && make to see when this is needed (instead of build-webkit)

(This is a flaw in our current build process)

[Mario Sanchez Prada]

Comment on attachment 180345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180345&action=review

Thanks Frederik for working on this patch. Guess it's not coincidence the release of Qt5 happened two days ago, huh? :-)

In any case, overall the patch looks good to me providing it works (I have no much knowledge about Qt) and that you fix both those build issues spotted by the EWS and the things both Simon and me commented on.

> Source/WebCore/Target.pri:56
> +    accessibility/AXObjectCacheQt.cpp \

Why not placing AXObjectCacheQt.cpp inside of accessibility/qt, as other ports do?

> Source/WebCore/accessibility/AccessibilityObject.cpp:78
> +#if PLATFORM(GTK) || (PLATFORM(EFL) && HAVE(ACCESSIBILITY)) || (PLATFORM(QT) && HAVE(ACCESSIBILITY))

You could refactor this boolean condition to something shorter:

  #if PLATFORM(GTK) || ((PLATFORM(EFL) || PLATFORM(QT)) && HAVE(ACCESSIBILITY))

...or maybe just to leave it as "#if HAVE(ACCESSIBILITY)" (not 100% sure though)

>> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:40
>> +void AccessibilityObject::setWrapper(AccessibilityObjectWrapper *wrapper)
> 
> * placement

Misplaced "*"

[Frederik Gladhorn]

Created attachment 181907 [details]
Patch

[Frederik Gladhorn]

I need to still look into this comment from Simon:
> This class (qwebviewaccessible) needs to move from QtWebKitWidgets into QtWebKit. Try compiling with make && make to see when this is needed (instead of build-webkit)

And accessibility needs to be enabled only for Qt 5.1, so there should probably be a qt version check in the .pri file.

I just thought it made sense to update the patch with what I currently have.

[Early Warning System Bot]

Comment on attachment 181907 [details]
Patch

Attachment 181907 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15758543

[Early Warning System Bot]

Comment on attachment 181907 [details]
Patch

Attachment 181907 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15755586

[Simon Hausmann]

Comment on attachment 181907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181907&action=review

Some early comments

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:51
> +    Q_ASSERT(m_document);
> +    Q_ASSERT(m_id);

We use WTF/WebKit types and macros usually, so in this case just ASSERT() :)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:102
> +    if (accessibilityObject()) {
> +        const AccessibilityObjectWrapper* childWrapper = static_cast<const AccessibilityObjectWrapper*>(childIface);
> +        if (AccessibilityObject* childObject = childWrapper->accessibilityObject()) {
> +            AccessibilityObject* parentObject = childObject->parentObjectUnignored();
> +            if (parentObject)
> +                return parentObject->children().find(childObject);
> +        }
> +    }
> +    return -1;

I've become a fan of the early-return style of code formatting that reduces the nesting required. So

if (!accessibiltyObject())
   return -1;

<other code with one level of indentation less>

> Source/WebKit/qt/WidgetApi/qwebviewaccessible.cpp:64
> +    if (index)

Would it perhaps be more readable to write if (index != 0) ?

> Tools/qmake/mkspecs/features/features.prf:124
> +    contains(QT_CONFIG,accessibility) {

Yeah so it looks like here you need to do a Qt version check :)

[Anders Carlsson]

Comment on attachment 181907 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.