[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 #20 from Mario Sanchez Prada <msanchez at igalia.com>  2012-02-08 07:07:16 PST ---
(From update of attachment 125837)
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.

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