45687 2010-09-13 11:01:50 -0700 [GTK] Code simplification needed in Atk Wrapper 2010-09-13 20:16:08 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 0 mario webkit-unassigned commit-queue mrobinson oldest_to_newest 278314 0 mario 2010-09-13 11:01:50 -0700 The following piece of code in WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp should be simplified: // Set preliminar values for start and end offsets startOffset = nodeRangeStart.offsetInContainerNode(); endOffset = nodeRangeEnd.offsetInContainerNode(); // If the end node is different then the start node, iterate over // those among them to build the effective value for endOffset if (nodeRangeStart.anchorNode() != nodeRangeEnd.anchorNode()) { RefPtr<Range> nodeRange = Range::create(node->document(), nodeRangeStart, positionBeforeNode(nodeRangeEnd.anchorNode())); for (TextIterator it(nodeRange.get()); !it.atEnd(); it.advance()) { RefPtr<Range> range = it.range(); if (range->startContainer()->isTextNode()) endOffset += range->endOffset(); } } This could be simplified using TextIterator::rangeLength(), making the code cleaner and more readable 278316 1 67433 mario 2010-09-13 11:09:08 -0700 Created attachment 67433 Patch to fix this issue Attaching a patch to fix this "issue" by simplifying the code as expected: Use TextIterator::rangeLength() to calculate endOffset * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: (getSelectionOffsetsForObject): Don't manually iterate over a artificial new range but just call TextIterator::rangeLength on the range created between the start and end positions. Hence, asking for review (and for the cq+ flag as well ;-) 278321 2 67433 mrobinson 2010-09-13 11:16:53 -0700 Comment on attachment 67433 Patch to fix this issue Awesome. Thanks! 278671 3 67433 commit-queue 2010-09-13 20:16:03 -0700 Comment on attachment 67433 Patch to fix this issue Clearing flags on attachment: 67433 Committed r67435: <http://trac.webkit.org/changeset/67435> 278672 4 commit-queue 2010-09-13 20:16:08 -0700 All reviewed patches have been landed. Closing bug. 67433 2010-09-13 11:09:08 -0700 2010-09-13 20:16:03 -0700 Patch to fix this issue 0001-2010-09-13-Mario-Sanchez-Prada-msanchez-igalia.com.patch text/plain 3226 mario RnJvbSBhNjE2YjA1ZWFjYTBiMDA0ZGQ5YWMxMzBmYTkxYmMzYTZmOGUyYmFlIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJpbyBTYW5jaGV6IFByYWRhIDxtc2FuY2hlekBpZ2FsaWEu Y29tPgpEYXRlOiBNb24sIDEzIFNlcCAyMDEwIDIwOjA1OjA5ICswMjAwClN1YmplY3Q6IFtQQVRD SF0gMjAxMC0wOS0xMyAgTWFyaW8gU2FuY2hleiBQcmFkYSAgPG1zYW5jaGV6QGlnYWxpYS5jb20+ CgogICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKICAgICAgICBbR1RLXSBDb2Rl IHNpbXBsaWZpY2F0aW9uIG5lZWRlZCBpbiBBdGsgV3JhcHBlcgogICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NTY4NwoKICAgICAgICBVc2UgVGV4dEl0ZXJh dG9yOjpyYW5nZUxlbmd0aCgpIHRvIGNhbGN1bGF0ZSBlbmRPZmZzZXQKCiAgICAgICAgKiBhY2Nl c3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHA6CiAgICAgICAg KGdldFNlbGVjdGlvbk9mZnNldHNGb3JPYmplY3QpOiBEb24ndCBtYW51YWxseSBpdGVyYXRlIG92 ZXIgYQogICAgICAgIGFydGlmaWNpYWwgbmV3IHJhbmdlIGJ1dCBqdXN0IGNhbGwgVGV4dEl0ZXJh dG9yOjpyYW5nZUxlbmd0aCBvbgogICAgICAgIHRoZSByYW5nZSBjcmVhdGVkIGJldHdlZW4gdGhl IHN0YXJ0IGFuZCBlbmQgcG9zaXRpb25zLgotLS0KIFdlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgIHwgICAxNCArKysrKysrKysrKysrKwogLi4uL2d0ay9B Y2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHAgICAgICAgICAgfCAgIDE2ICsrKy0tLS0t LS0tLS0tLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTcgaW5zZXJ0aW9ucygrKSwgMTMgZGVsZXRpb25z KC0pCgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwpp bmRleCA3YWQ1ODdmLi5jNjBiODJmIDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysg Yi9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEwLTA5LTEzICBNYXJpbyBT YW5jaGV6IFByYWRhICA8bXNhbmNoZXpAaWdhbGlhLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbR1RLXSBDb2RlIHNpbXBsaWZpY2F0aW9uIG5l ZWRlZCBpbiBBdGsgV3JhcHBlcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9NDU2ODcKKworICAgICAgICBVc2UgVGV4dEl0ZXJhdG9yOjpyYW5nZUxlbmd0 aCgpIHRvIGNhbGN1bGF0ZSBlbmRPZmZzZXQKKworICAgICAgICAqIGFjY2Vzc2liaWxpdHkvZ3Rr L0FjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyQXRrLmNwcDoKKyAgICAgICAgKGdldFNlbGVjdGlv bk9mZnNldHNGb3JPYmplY3QpOiBEb24ndCBtYW51YWxseSBpdGVyYXRlIG92ZXIgYQorICAgICAg ICBhcnRpZmljaWFsIG5ldyByYW5nZSBidXQganVzdCBjYWxsIFRleHRJdGVyYXRvcjo6cmFuZ2VM ZW5ndGggb24KKyAgICAgICAgdGhlIHJhbmdlIGNyZWF0ZWQgYmV0d2VlbiB0aGUgc3RhcnQgYW5k IGVuZCBwb3NpdGlvbnMuCisKIDIwMTAtMDktMTMgIFl1cnkgU2VtaWtoYXRza3kgIDx5dXJ5c0Bj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgUGF2ZWwgRmVsZG1hbi4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9ndGsvQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBw ZXJBdGsuY3BwIGIvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0 V3JhcHBlckF0ay5jcHAKaW5kZXggOTgwZTE2ZS4uMDFkZGRkMSAxMDA2NDQKLS0tIGEvV2ViQ29y ZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0ay5jcHAKKysr IGIvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L2d0ay9BY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckF0 ay5jcHAKQEAgLTEzOTcsMjAgKzEzOTcsMTAgQEAgc3RhdGljIHZvaWQgZ2V0U2VsZWN0aW9uT2Zm c2V0c0Zvck9iamVjdChBY2Nlc3NpYmlsaXR5T2JqZWN0KiBjb3JlT2JqZWN0LCBWaXNpYmwKICAg ICBlbHNlCiAgICAgICAgIG5vZGVSYW5nZUVuZCA9IHNlbFJhbmdlLT5lbmRQb3NpdGlvbigpOwog Ci0gICAgLy8gU2V0IHByZWxpbWluYXIgdmFsdWVzIGZvciBzdGFydCBhbmQgZW5kIG9mZnNldHMK KyAgICAvLyBTZXQgdmFsdWVzIGZvciBzdGFydCBhbmQgZW5kIG9mZnNldHMKKyAgICBSZWZQdHI8 UmFuZ2U+IG5vZGVSYW5nZSA9IFJhbmdlOjpjcmVhdGUobm9kZS0+ZG9jdW1lbnQoKSwgbm9kZVJh bmdlU3RhcnQsIG5vZGVSYW5nZUVuZCk7CiAgICAgc3RhcnRPZmZzZXQgPSBub2RlUmFuZ2VTdGFy dC5vZmZzZXRJbkNvbnRhaW5lck5vZGUoKTsKLSAgICBlbmRPZmZzZXQgPSBub2RlUmFuZ2VFbmQu b2Zmc2V0SW5Db250YWluZXJOb2RlKCk7Ci0KLSAgICAvLyBJZiB0aGUgZW5kIG5vZGUgaXMgZGlm ZmVyZW50IHRoZW4gdGhlIHN0YXJ0IG5vZGUsIGl0ZXJhdGUgb3ZlcgotICAgIC8vIHRob3NlIGFt b25nIHRoZW0gdG8gYnVpbGQgdGhlIGVmZmVjdGl2ZSB2YWx1ZSBmb3IgZW5kT2Zmc2V0Ci0gICAg aWYgKG5vZGVSYW5nZVN0YXJ0LmFuY2hvck5vZGUoKSAhPSBub2RlUmFuZ2VFbmQuYW5jaG9yTm9k ZSgpKSB7Ci0gICAgICAgIFJlZlB0cjxSYW5nZT4gbm9kZVJhbmdlID0gUmFuZ2U6OmNyZWF0ZShu b2RlLT5kb2N1bWVudCgpLCBub2RlUmFuZ2VTdGFydCwgcG9zaXRpb25CZWZvcmVOb2RlKG5vZGVS YW5nZUVuZC5hbmNob3JOb2RlKCkpKTsKLSAgICAgICAgZm9yIChUZXh0SXRlcmF0b3IgaXQobm9k ZVJhbmdlLmdldCgpKTsgIWl0LmF0RW5kKCk7IGl0LmFkdmFuY2UoKSkgewotICAgICAgICAgICAg UmVmUHRyPFJhbmdlPiByYW5nZSA9IGl0LnJhbmdlKCk7Ci0gICAgICAgICAgICBpZiAocmFuZ2Ut PnN0YXJ0Q29udGFpbmVyKCktPmlzVGV4dE5vZGUoKSkKLSAgICAgICAgICAgICAgICBlbmRPZmZz ZXQgKz0gcmFuZ2UtPmVuZE9mZnNldCgpOwotICAgICAgICB9Ci0gICAgfQorICAgIGVuZE9mZnNl dCA9IHN0YXJ0T2Zmc2V0ICsgVGV4dEl0ZXJhdG9yOjpyYW5nZUxlbmd0aChub2RlUmFuZ2UuZ2V0 KCkpOwogfQogCiBzdGF0aWMgZ2ludCB3ZWJraXRfYWNjZXNzaWJsZV90ZXh0X2dldF9uX3NlbGVj dGlvbnMoQXRrVGV4dCogdGV4dCkKLS0gCjEuNy4wLjQKCg==