Bug 77878 - [gtk] Accessibility: use find funtion in vector instead of for.
Summary: [gtk] Accessibility: use find funtion in vector instead of for.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-06 08:49 PST by Frederik Gladhorn
Modified: 2012-02-22 01:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.76 KB, patch)
2012-02-06 08:53 PST, Frederik Gladhorn
no flags Details | Formatted Diff | Diff
Patch (1.75 KB, patch)
2012-02-07 05:30 PST, Frederik Gladhorn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frederik Gladhorn 2012-02-06 08:49:13 PST
[gtk] Accessibility: use find funtion in vector instead of for.
Comment 1 Frederik Gladhorn 2012-02-06 08:53:55 PST
Created attachment 125658 [details]
Patch
Comment 2 Mario Sanchez Prada 2012-02-07 04:16:10 PST
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;
  }
Comment 3 Frederik Gladhorn 2012-02-07 05:30:29 PST
Created attachment 125826 [details]
Patch
Comment 4 Frederik Gladhorn 2012-02-07 05:30:54 PST
(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.
Comment 5 Mario Sanchez Prada 2012-02-07 05:37:41 PST
(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!
Comment 6 Hajime Morrita 2012-02-20 21:06:20 PST
Comment on attachment 125826 [details]
Patch

Looks good. And simple enough ;-)
Comment 7 WebKit Review Bot 2012-02-22 01:28:42 PST
Comment on attachment 125826 [details]
Patch

Clearing flags on attachment: 125826

Committed r108463: <http://trac.webkit.org/changeset/108463>
Comment 8 WebKit Review Bot 2012-02-22 01:28:49 PST
All reviewed patches have been landed.  Closing bug.