53554 2011-02-01 16:54:26 -0800 make draft comments focusable 2011-02-01 22:03:17 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan ojan abarth oldest_to_newest 343677 0 ojan 2011-02-01 16:54:26 -0800 make draft comments focusable 343679 1 80856 ojan 2011-02-01 16:55:57 -0800 Created attachment 80856 Patch 343831 2 80856 abarth 2011-02-01 21:30:05 -0800 Comment on attachment 80856 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80856&action=review This is fine, but consider the comments below. BTW, I love j/k. :) > Websites/bugs.webkit.org/code-review.js:1280 > - if (node.hasClass(className)) { > + if (focus_type == FOCUS_TYPE.COMMENT && (node.hasClass('frozenComment') || node.hasClass('previousComment')) || > + focus_type == FOCUS_TYPE.DIFF_BLOCK && node.hasClass('DiffBlock')) { I'd just pass in a function that you call at this branch. That stops this code from having to know about all the different types. 343849 3 ojan 2011-02-01 22:00:34 -0800 Committed r77363: <http://trac.webkit.org/changeset/77363> 343851 4 80856 ojan 2011-02-01 22:03:17 -0800 Comment on attachment 80856 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80856&action=review >> Websites/bugs.webkit.org/code-review.js:1280 >> + focus_type == FOCUS_TYPE.DIFF_BLOCK && node.hasClass('DiffBlock')) { > > I'd just pass in a function that you call at this branch. That stops this code from having to know about all the different types. Done. That's much better. 80856 2011-02-01 16:55:57 -0800 2011-02-01 22:03:17 -0800 Patch bug-53554-20110202115554.patch text/plain 3335 ojan ZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cgYi9XZWJzaXRl cy9idWdzLndlYmtpdC5vcmcvQ2hhbmdlTG9nCmluZGV4IDAzOWEzNmQzZDVmNTk4N2QzZDliYjkz ODhjYmZhMzFmNmYzMDc3ZmEuLjRlNjY4YTJjNGI0ODAwMDM0YTNjZTgyYTNlNTA4NDBkNTIzMGU5 NWYgMTAwNjQ0Ci0tLSBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cKKysrIGIv V2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE3IEBACiAyMDEx LTAyLTAxICBPamFuIFZhZmFpICA8b2phbkBjaHJvbWl1bS5vcmc+CiAKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgbWFrZSBkcmFmdCBjb21tZW50cyBmb2N1 c2FibGUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUz NTU0CisKKyAgICAgICAgTWFrZXMgZnJvemVuIGRyYWZ0IGNvbW1lbnRzIGZvY3VzYWJsZS4gVGhl IG9uZXMgdGhhdCBhcmUgY3VycmVudGx5IGJlaW5nIGVkaXRlZCBhcmUgbm90LgorICAgICAgICBJ J20gb24gdGhlIGZlbmNlIHdoZXRoZXIgdGhleSBzaG91bGQgYmUsIGJ1dCB0aGlzIHNlZW1zIGdv b2QgZW5vdWdoIGZvciBub3cuCisKKyAgICAgICAgKiBjb2RlLXJldmlldy5qczoKKworMjAxMS0w Mi0wMSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgorCiAgICAgICAgIEluY3JlbWVu dCB2ZXJzaW9uIG51bWJlciB0byBjYWNoZS1idXN0IGNvZGUtcmV2aWV3LmpzLgogCiAgICAgICAg ICogUHJldHR5UGF0Y2gvUHJldHR5UGF0Y2gucmI6CmRpZmYgLS1naXQgYS9XZWJzaXRlcy9idWdz LndlYmtpdC5vcmcvY29kZS1yZXZpZXcuanMgYi9XZWJzaXRlcy9idWdzLndlYmtpdC5vcmcvY29k ZS1yZXZpZXcuanMKaW5kZXggZDYyNmQzYWQ2NTA3NzkxOTVjZTI2NTY5NTZhNjljMjIyOGUyNzEw OC4uMDY4MTA1MzllZTcwMjAwYjNlNzJjYzM5NDZkZGRkOTdjNzA2YTMwZiAxMDA2NDQKLS0tIGEv V2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL2NvZGUtcmV2aWV3LmpzCisrKyBiL1dlYnNpdGVzL2J1 Z3Mud2Via2l0Lm9yZy9jb2RlLXJldmlldy5qcwpAQCAtMTI2MiwxMSArMTI2MiwxMiBAQCB2YXIg Q09ERV9SRVZJRVdfVU5JVFRFU1Q7CiAgICAgJChkb2N1bWVudCkuc2Nyb2xsVG9wKG5vZGUuYWRk Q2xhc3MoJ2ZvY3VzZWQnKS5wb3NpdGlvbigpLnRvcCAtIHdpbmRvdy5pbm5lckhlaWdodCAvIDIp OwogICB9CiAKLSAgZnVuY3Rpb24gZm9jdXNOZXh0KGNsYXNzTmFtZSwgaXNfYmFja3dhcmQpIHsK LSAgICB2YXIgZm9jdXNhYmxlX25vZGVzID0gJCgnLnByZXZpb3VzQ29tbWVudCwuRGlmZkJsb2Nr JykuZmlsdGVyKGZ1bmN0aW9uKCkgewotICAgICAgcmV0dXJuICgkKHRoaXMpLmhhc0NsYXNzKCdw cmV2aW91c0NvbW1lbnQnKSB8fCAkKCcuYWRkLC5yZW1vdmUnLCB0aGlzKS5zaXplKCkpOworICBm dW5jdGlvbiBmb2N1c05leHQoZm9jdXNfdHlwZSwgZGlyZWN0aW9uKSB7CisgICAgdmFyIGZvY3Vz YWJsZV9ub2RlcyA9ICQoJy5mcm96ZW5Db21tZW50LC5wcmV2aW91c0NvbW1lbnQsLkRpZmZCbG9j aycpLmZpbHRlcihmdW5jdGlvbigpIHsKKyAgICAgIHJldHVybiAoJCh0aGlzKS5oYXNDbGFzcygn ZnJvemVuQ29tbWVudCcpIHx8ICQodGhpcykuaGFzQ2xhc3MoJ3ByZXZpb3VzQ29tbWVudCcpIHx8 ICQoJy5hZGQsLnJlbW92ZScsIHRoaXMpLnNpemUoKSk7CiAgICAgfSk7CiAKKyAgICB2YXIgaXNf YmFja3dhcmQgPSBkaXJlY3Rpb24gPT0gRElSRUNUSU9OLkJBQ0tXQVJEOwogICAgIHZhciBpbmRl eCA9IGZvY3VzYWJsZV9ub2Rlcy5pbmRleCgkKCcuZm9jdXNlZCcpKTsKICAgICBpZiAoaW5kZXgg PT0gLTEgJiYgaXNfYmFja3dhcmQpCiAgICAgICBpbmRleCA9IGZvY3VzYWJsZV9ub2Rlcy5sZW5n dGg7CkBAIC0xMjc1LDEzICsxMjc2LDE3IEBAIHZhciBDT0RFX1JFVklFV19VTklUVEVTVDsKICAg ICB2YXIgZW5kID0gaXNfYmFja3dhcmQgPyAtMSA6IGZvY3VzYWJsZV9ub2Rlcy5zaXplKCk7CiAg ICAgZm9yICh2YXIgaSA9IGluZGV4ICsgb2Zmc2V0OyBpICE9IGVuZDsgaSA9IGkgKyBvZmZzZXQp IHsKICAgICAgIHZhciBub2RlID0gJChmb2N1c2FibGVfbm9kZXNbaV0pOwotICAgICAgaWYgKG5v ZGUuaGFzQ2xhc3MoY2xhc3NOYW1lKSkgeworICAgICAgaWYgKGZvY3VzX3R5cGUgPT0gRk9DVVNf VFlQRS5DT01NRU5UICYmIChub2RlLmhhc0NsYXNzKCdmcm96ZW5Db21tZW50JykgfHwgbm9kZS5o YXNDbGFzcygncHJldmlvdXNDb21tZW50JykpIHx8CisgICAgICAgICAgZm9jdXNfdHlwZSA9PSBG T0NVU19UWVBFLkRJRkZfQkxPQ0sgJiYgbm9kZS5oYXNDbGFzcygnRGlmZkJsb2NrJykpIHsKICAg ICAgICAgZm9jdXNPbihub2RlKTsKICAgICAgICAgcmV0dXJuOwogICAgICAgfQogICAgIH0KICAg fQogCisgIHZhciBGT0NVU19UWVBFID0ge0NPTU1FTlQ6IDEsIERJRkZfQkxPQ0s6IDJ9OworICB2 YXIgRElSRUNUSU9OID0ge0ZPUldBUkQ6IDEsIEJBQ0tXQVJEOiAyfTsKKwogICB2YXIga0NoYXJD b2RlRm9yTiA9ICduJy5jaGFyQ29kZUF0KDApOwogICB2YXIga0NoYXJDb2RlRm9yUCA9ICdwJy5j aGFyQ29kZUF0KDApOwogICB2YXIga0NoYXJDb2RlRm9ySiA9ICdqJy5jaGFyQ29kZUF0KDApOwpA QCAtMTI5NSwxOSArMTMwMCwxOSBAQCB2YXIgQ09ERV9SRVZJRVdfVU5JVFRFU1Q7CiAKICAgICBz d2l0Y2ggKGV2ZW50LmNoYXJDb2RlKSB7CiAgICAgY2FzZSBrQ2hhckNvZGVGb3JOOgotICAgICAg Zm9jdXNOZXh0KCdwcmV2aW91c0NvbW1lbnQnLCBmYWxzZSk7CisgICAgICBmb2N1c05leHQoRk9D VVNfVFlQRS5DT01NRU5ULCBESVJFQ1RJT04uRk9SV0FSRCk7CiAgICAgICBicmVhazsKIAogICAg IGNhc2Uga0NoYXJDb2RlRm9yUDoKLSAgICAgIGZvY3VzTmV4dCgncHJldmlvdXNDb21tZW50Jywg dHJ1ZSk7CisgICAgICBmb2N1c05leHQoRk9DVVNfVFlQRS5DT01NRU5ULCBESVJFQ1RJT04uQkFD S1dBUkQpOwogICAgICAgYnJlYWs7CiAKICAgICBjYXNlIGtDaGFyQ29kZUZvcko6Ci0gICAgICBm b2N1c05leHQoJ0RpZmZCbG9jaycsIGZhbHNlKTsKKyAgICAgIGZvY3VzTmV4dChGT0NVU19UWVBF LkRJRkZfQkxPQ0ssIERJUkVDVElPTi5GT1JXQVJEKTsKICAgICAgIGJyZWFrOwogCiAgICAgY2Fz ZSBrQ2hhckNvZGVGb3JLOgotICAgICAgZm9jdXNOZXh0KCdEaWZmQmxvY2snLCB0cnVlKTsKKyAg ICAgIGZvY3VzTmV4dChGT0NVU19UWVBFLkRJRkZfQkxPQ0ssIERJUkVDVElPTi5CQUNLV0FSRCk7 CiAgICAgICBicmVhazsKICAgICB9CiAgIH0pOwo=