Bug 77878 – [gtk] Accessibility: use find funtion in vector instead of for.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Frederik Gladhorn]

[gtk] Accessibility: use find funtion in vector instead of for.

[Frederik Gladhorn]

Created attachment 125658 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 125658 [details]
Patch

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

Thanks for the patch, Frederik. Just informally reviewing now. See some comments below...

> Source/WebCore/ChangeLog:9
> +        find function was basically reimplemented here.

No, there was no good reason for implementing that part that way, other than probably not realizing of the find() function being there. 

Anyway, I'm not sure this "but I was wondering why" part is something that should be in the ChangeLog. I would probably leave it as "This is only a minor cleanup, as the find function was basically being reimplemented." (I'd avoid "here" unless you're using this sentence below, in the part of changelog where the function name is explicitly listed.

> Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:399
> +    return parent->children().find(coreObject);

I'm fine with the change. Just a minor nitpick: WTF::Vector::find() returns notFound (from NotFound.h) instead of plainly returning -1 so, even if it's actually the same thing in practise, I think it'd be better to replace this with something like this:

    [...]
    size_t indexFound = parent->children().find(coreObject);
    if (indexFound == WTF::notFound)
      return -1;

    return indexFound;
  }

[Frederik Gladhorn]

Created attachment 125826 [details]
Patch

[Frederik Gladhorn]

(In reply to comment #2)
> (From update of attachment 125658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125658&action=review
> 
> Thanks for the patch, Frederik. Just informally reviewing now. See some comments below...
> 
> > Source/WebCore/ChangeLog:9
> > +        find function was basically reimplemented here.
> 
> No, there was no good reason for implementing that part that way, other than probably not realizing of the find() function being there. 
> 
> Anyway, I'm not sure this "but I was wondering why" part is something that should be in the ChangeLog. I would probably leave it as "This is only a minor cleanup, as the find function was basically being reimplemented." (I'd avoid "here" unless you're using this sentence below, in the part of changelog where the function name is explicitly listed.

Sure, changelog cleaned up.

> 
> > Source/WebCore/accessibility/gtk/WebKitAccessibleWrapperAtk.cpp:399
> > +    return parent->children().find(coreObject);
> 
> I'm fine with the change. Just a minor nitpick: WTF::Vector::find() returns notFound (from NotFound.h) instead of plainly returning -1 so, even if it's actually the same thing in practise, I think it'd be better to replace this with something like this:
> 
>     [...]
>     size_t indexFound = parent->children().find(coreObject);
>     if (indexFound == WTF::notFound)
>       return -1;
> 
>     return indexFound;
>   }
It's your code ;) I just thought it might be nice to have it a little easier to read. I personally would prefer the one liner, but I guess that's a matter of taste.
Also mind that I didn't compile it since I don't even have the dependencies to build the gtk port.

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> It's your code ;) I just thought it might be nice to have it a little easier to read. I personally would prefer the
> one liner, but I guess that's a matter of taste. 

Actually it is not :-). That code comes from the former, old and big, AccessibilityObjectWrapperAtk.cpp file that we used to have prior to the refactoring, and that specific implementation is not mind, IIRC, so that's why it got into this new file without me noticing.

In any case, I appreciate very much this refactoring since it's definitely easier to read, but still I personally think it would be better to explicitly consider this WTF:notFound value, as it's done in other places, such as AccessibilityObject.cpp.

> Also mind that I didn't compile it since I don't even have the dependencies to build the gtk port.

Well, the EWS says it's building fine, and the change seems to be pretty harmless :-)

Anyway, I'm not a reviewer yet, so I'm CCing people who is to see if we can get this in.

Thank you!

[Hajime Morrita]

Comment on attachment 125826 [details]
Patch

Looks good. And simple enough ;-)

[WebKit Review Bot]

Comment on attachment 125826 [details]
Patch

Clearing flags on attachment: 125826

Committed r108463: <http://trac.webkit.org/changeset/108463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.