[gtk] Accessibility: use find funtion in vector instead of for.
Created attachment 125658 [details] Patch
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; }
Created attachment 125826 [details] Patch
(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.
(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 on attachment 125826 [details] Patch Looks good. And simple enough ;-)
Comment on attachment 125826 [details] Patch Clearing flags on attachment: 125826 Committed r108463: <http://trac.webkit.org/changeset/108463>
All reviewed patches have been landed. Closing bug.