77878 2012-02-06 08:49:13 -0800 [gtk] Accessibility: use find funtion in vector instead of for. 2012-02-22 01:28:49 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 gladhorn webkit-unassigned mario mrobinson pnormand webkit.review.bot oldest_to_newest 550033 0 gladhorn 2012-02-06 08:49:13 -0800 [gtk] Accessibility: use find funtion in vector instead of for. 550036 1 125658 gladhorn 2012-02-06 08:53:55 -0800 Created attachment 125658 Patch 550827 2 125658 mario 2012-02-07 04:16:10 -0800 Comment on attachment 125658 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; } 550892 3 125826 gladhorn 2012-02-07 05:30:29 -0800 Created attachment 125826 Patch 550893 4 gladhorn 2012-02-07 05:30:54 -0800 (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. 550898 5 mario 2012-02-07 05:37:41 -0800 (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! 560612 6 125826 morrita 2012-02-20 21:06:20 -0800 Comment on attachment 125826 Patch Looks good. And simple enough ;-) 561738 7 125826 webkit.review.bot 2012-02-22 01:28:42 -0800 Comment on attachment 125826 Patch Clearing flags on attachment: 125826 Committed r108463: <http://trac.webkit.org/changeset/108463> 561739 8 webkit.review.bot 2012-02-22 01:28:49 -0800 All reviewed patches have been landed. Closing bug. 125658 2012-02-06 08:53:55 -0800 2012-02-07 05:30:20 -0800 Patch bug-77878-20120206175352.patch text/plain 1799 gladhorn U3VidmVyc2lvbiBSZXZpc2lvbjogMTA2NDk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzhiNGNhNDljNjgwNThi OWVhNzQ0ZmUyN2ZiNDc4MzE0YzRmYjY4Yi4uMjkyOWYyZTk3YWY3MDZiZmE3YjE3MTA5NTlkZDlh YmY4NTA0YzFjMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAyLTA2ICBGcmVk ZXJpayBHbGFkaG9ybiAgPGZyZWRlcmlrLmdsYWRob3JuQG5va2lhLmNvbT4KKworICAgICAgICBb Z3RrXSBBY2Nlc3NpYmlsaXR5OiB1c2UgZmluZCBmdW50aW9uIGluIHZlY3RvciBpbnN0ZWFkIG9m IGZvci4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc3 ODc4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhp cyBpcyBvbmx5IGEgbWlub3IgY2xlYW51cCwgYnV0IEkgd2FzIHdvbmRlcmluZyB3aHkgdGhlCisg ICAgICAgIGZpbmQgZnVuY3Rpb24gd2FzIGJhc2ljYWxseSByZWltcGxlbWVudGVkIGhlcmUuCisK KyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L2d0ay9XZWJLaXRBY2Nlc3NpYmxlV3JhcHBlckF0ay5j cHA6CisgICAgICAgICh3ZWJraXRBY2Nlc3NpYmxlR2V0SW5kZXhJblBhcmVudCk6CisKIDIwMTIt MDItMDEgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgR2NjIGJ1 aWxkIGZpeCBhZnRlciByMTA2NDgyLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYWNjZXNz aWJpbGl0eS9ndGsvV2ViS2l0QWNjZXNzaWJsZVdyYXBwZXJBdGsuY3BwIGIvU291cmNlL1dlYkNv cmUvYWNjZXNzaWJpbGl0eS9ndGsvV2ViS2l0QWNjZXNzaWJsZVdyYXBwZXJBdGsuY3BwCmluZGV4 IDUyNWQyOGVhNTY2YzcyYTdmZTEwOTU4Mzk4OWEyNDRmMGY1MjUxOWEuLmIzYmYxM2MzYTMwNzI5 MDg3NTA1ZTdlOWY2ZDc2MzU2NDYzNWQ5YjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Fj Y2Vzc2liaWxpdHkvZ3RrL1dlYktpdEFjY2Vzc2libGVXcmFwcGVyQXRrLmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9XZWJLaXRBY2Nlc3NpYmxlV3JhcHBlckF0ay5j cHAKQEAgLTM5NiwxNCArMzk2LDcgQEAgc3RhdGljIGdpbnQgd2Via2l0QWNjZXNzaWJsZUdldElu ZGV4SW5QYXJlbnQoQXRrT2JqZWN0KiBvYmplY3QpCiAgICAgaWYgKHBhcmVudCAmJiBwYXJlbnQt PmlzVGFibGVSb3coKSAmJiBjb3JlT2JqZWN0LT5pc1RhYmxlQ2VsbCgpKQogICAgICAgICByZXR1 cm4gZ2V0SW5kZXhJblBhcmVudEZvckNlbGxJblJvdyhjb3JlT2JqZWN0KTsKIAotICAgIEFjY2Vz c2liaWxpdHlPYmplY3Q6OkFjY2Vzc2liaWxpdHlDaGlsZHJlblZlY3RvciBjaGlsZHJlbiA9IHBh cmVudC0+Y2hpbGRyZW4oKTsKLSAgICB1bnNpZ25lZCBjb3VudCA9IGNoaWxkcmVuLnNpemUoKTsK LSAgICBmb3IgKHVuc2lnbmVkIGkgPSAwOyBpIDwgY291bnQ7ICsraSkgewotICAgICAgICBpZiAo Y2hpbGRyZW5baV0gPT0gY29yZU9iamVjdCkKLSAgICAgICAgICAgIHJldHVybiBpOwotICAgIH0K LQotICAgIHJldHVybiAtMTsKKyAgICByZXR1cm4gcGFyZW50LT5jaGlsZHJlbigpLmZpbmQoY29y ZU9iamVjdCk7CiB9CiAKIHN0YXRpYyBBdGtBdHRyaWJ1dGVTZXQqIHdlYmtpdEFjY2Vzc2libGVH ZXRBdHRyaWJ1dGVzKEF0a09iamVjdCogb2JqZWN0KQo= 125826 2012-02-07 05:30:29 -0800 2012-02-22 01:28:42 -0800 Patch bug-77878-20120207143022.patch text/plain 1793 gladhorn U3VidmVyc2lvbiBSZXZpc2lvbjogMTA2NDk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzhiNGNhNDljNjgwNThi OWVhNzQ0ZmUyN2ZiNDc4MzE0YzRmYjY4Yi4uMDExZjZmZGFjMDYzZWZiZmY2MGY0YTk2ZmU2MTNj MTIyNTNmOGQxMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDEyLTAyLTA2ICBGcmVk ZXJpayBHbGFkaG9ybiAgPGZyZWRlcmlrLmdsYWRob3JuQG5va2lhLmNvbT4KKworICAgICAgICBb Z3RrXSBBY2Nlc3NpYmlsaXR5OiB1c2UgZmluZCBmdW50aW9uIGluIHZlY3RvciBpbnN0ZWFkIG9m IGZvci4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTc3 ODc4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWlu b3IgY2xlYW51cCwgdXNlIHRoZSBmaW5kIGZ1bmN0aW9uIGZyb20gdmVjdG9yLgorCisgICAgICAg ICogYWNjZXNzaWJpbGl0eS9ndGsvV2ViS2l0QWNjZXNzaWJsZVdyYXBwZXJBdGsuY3BwOgorICAg ICAgICAod2Via2l0QWNjZXNzaWJsZUdldEluZGV4SW5QYXJlbnQpOgorCiAyMDEyLTAyLTAxICBS eW9zdWtlIE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgogCiAgICAgICAgIEdjYyBidWlsZCBmaXgg YWZ0ZXIgcjEwNjQ4Mi4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkv Z3RrL1dlYktpdEFjY2Vzc2libGVXcmFwcGVyQXRrLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2FjY2Vz c2liaWxpdHkvZ3RrL1dlYktpdEFjY2Vzc2libGVXcmFwcGVyQXRrLmNwcAppbmRleCA1MjVkMjhl YTU2NmM3MmE3ZmUxMDk1ODM5ODlhMjQ0ZjBmNTI1MTlhLi43ZjZiODk0MTJlMjk1MDRjZDU4ZmRj ZTFiY2JiMDcyOTA1YmJkNzdmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmls aXR5L2d0ay9XZWJLaXRBY2Nlc3NpYmxlV3JhcHBlckF0ay5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvYWNjZXNzaWJpbGl0eS9ndGsvV2ViS2l0QWNjZXNzaWJsZVdyYXBwZXJBdGsuY3BwCkBAIC0z OTYsMTQgKzM5Niw4IEBAIHN0YXRpYyBnaW50IHdlYmtpdEFjY2Vzc2libGVHZXRJbmRleEluUGFy ZW50KEF0a09iamVjdCogb2JqZWN0KQogICAgIGlmIChwYXJlbnQgJiYgcGFyZW50LT5pc1RhYmxl Um93KCkgJiYgY29yZU9iamVjdC0+aXNUYWJsZUNlbGwoKSkKICAgICAgICAgcmV0dXJuIGdldElu ZGV4SW5QYXJlbnRGb3JDZWxsSW5Sb3coY29yZU9iamVjdCk7CiAKLSAgICBBY2Nlc3NpYmlsaXR5 T2JqZWN0OjpBY2Nlc3NpYmlsaXR5Q2hpbGRyZW5WZWN0b3IgY2hpbGRyZW4gPSBwYXJlbnQtPmNo aWxkcmVuKCk7Ci0gICAgdW5zaWduZWQgY291bnQgPSBjaGlsZHJlbi5zaXplKCk7Ci0gICAgZm9y ICh1bnNpZ25lZCBpID0gMDsgaSA8IGNvdW50OyArK2kpIHsKLSAgICAgICAgaWYgKGNoaWxkcmVu W2ldID09IGNvcmVPYmplY3QpCi0gICAgICAgICAgICByZXR1cm4gaTsKLSAgICB9Ci0KLSAgICBy ZXR1cm4gLTE7CisgICAgc2l6ZV90IGluZGV4ID0gcGFyZW50LT5jaGlsZHJlbigpLmZpbmQoY29y ZU9iamVjdCk7CisgICAgcmV0dXJuIChpbmRleCA9PSBXVEY6Om5vdEZvdW5kKSA/IC0xIDogaW5k ZXg7CiB9CiAKIHN0YXRpYyBBdGtBdHRyaWJ1dGVTZXQqIHdlYmtpdEFjY2Vzc2libGVHZXRBdHRy aWJ1dGVzKEF0a09iamVjdCogb2JqZWN0KQo=