52253 2011-01-11 14:14:35 -0800 remember diffstate for review tool 2011-01-11 15:57:01 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan ojan abarth eric mihaip oldest_to_newest 332647 0 ojan 2011-01-11 14:14:35 -0800 remember diffstate for review tool 332648 1 78598 ojan 2011-01-11 14:16:27 -0800 Created attachment 78598 Patch 332656 2 ojan 2011-01-11 14:26:39 -0800 This builds on the patch in bug 52226. 332667 3 78598 abarth 2011-01-11 14:36:34 -0800 Comment on attachment 78598 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78598&action=review > Websites/bugs.webkit.org/code-review.js:399 > + localStorage.diffstate = difftype; Can we use setItem and getItem? Also, we should use a name like code-review-diffstate that scopes the state to the code review tool. 332689 4 78598 abarth 2011-01-11 14:50:35 -0800 Comment on attachment 78598 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78598&action=review This patch is fine. It could use slightly more polish though. > Websites/bugs.webkit.org/code-review.js:744 > + updateDiffState(); updateDiffState sounds like I can call this whenever I like, but this is really a load-time function. Maybe loadDiffState? > Websites/bugs.webkit.org/code-review.js:749 > + if (localStorage.diffstate != 'sidebyside') > + return; It seems like we should be more explicit here. Maybe check that we've gotten one of the two expected values and then take action. > Websites/bugs.webkit.org/code-review.js:753 > + $('.FileDiff').each(function() { > + convertFileDiff('sidebyside', this); > + }); This chunk of code seems like it should be shared with the side-by-side link, right? 332754 5 ojan 2011-01-11 15:57:01 -0800 Committed r75565: <http://trac.webkit.org/changeset/75565> 78598 2011-01-11 14:16:27 -0800 2011-01-11 14:50:34 -0800 Patch bug-52253-20110111141626.patch text/plain 1990 ojan ZGlmZiAtLWdpdCBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cgYi9XZWJzaXRl cy9idWdzLndlYmtpdC5vcmcvQ2hhbmdlTG9nCmluZGV4IGY2N2Q0NTg1OWU3YmUzMzA1MWUxNjk0 MDkzZWVkMGFhZDlmNjEzZjEuLjA1NzhkZTdmZmE4ZTAxYTJhNjc4ZWM0ZGMzYWIzNzhkNGJkODRk OGQgMTAwNjQ0Ci0tLSBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9DaGFuZ2VMb2cKKysrIGIv V2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL0NoYW5nZUxvZwpAQCAtMiw2ICsyLDE5IEBACiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCiAKKyAgICAgICAgcmVtZW1iZXIgZGlm ZnN0YXRlIGZvciByZXZpZXcgdG9vbAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NTIyNTMKKworICAgICAgICBJZiB5b3UgdXNlIHRoZSBnbG9iYWwgc2lk ZWJ5c2lkZS91bmlmaWVkIGxpbmtzLCBzdG9yZSB0aGUgZGlmZiB0eXBlCisgICAgICAgIGluIGxv Y2Fsc3RvcmFnZS4gVGhlbiwgb25sb2FkLCB1c2UgdGhhdCBkaWZmIHR5cGUuIFRoYXQgd2F5LCBw ZW9wbGUKKyAgICAgICAgd2hvIHByZWZlciBvbmUgZGlmZiB0eXBlIG9yIHRoZSBvdGhlciBjYW4g YWx3YXlzIGdldCB0aGF0IGJ5IGRlZmF1bHQuCisKKyAgICAgICAgKiBjb2RlLXJldmlldy5qczoK KworMjAxMS0wMS0xMSAgT2phbiBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCiAgICAgICAgIGFsbG93IHNpZGVieXNpZGVp ZnlpbmcgaW5kaXZpZHVhbCBmaWxlcyBpbiB0aGUgY29kZSByZXZpZXcgdG9vbAogICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTIyMjYKIApkaWZmIC0tZ2l0 IGEvV2Vic2l0ZXMvYnVncy53ZWJraXQub3JnL2NvZGUtcmV2aWV3LmpzIGIvV2Vic2l0ZXMvYnVn cy53ZWJraXQub3JnL2NvZGUtcmV2aWV3LmpzCmluZGV4IGZjZjQxYTI1YjM2MjUzMjMxNWJiYmEx OTJhNjU4ZDQxYWUyNmVjYzYuLjc4MmMyNjY4ZDc1ODBmYTZiZTgwYzkzNzUyZjY0M2NhNzZhNDE1 ZGMgMTAwNjQ0Ci0tLSBhL1dlYnNpdGVzL2J1Z3Mud2Via2l0Lm9yZy9jb2RlLXJldmlldy5qcwor KysgYi9XZWJzaXRlcy9idWdzLndlYmtpdC5vcmcvY29kZS1yZXZpZXcuanMKQEAgLTM5NSw4ICsz OTUsMTAgQEAKIAogICBmdW5jdGlvbiBjb252ZXJ0RGlmZihkaWZmdHlwZSwgY29udmVydF9saW5r KSB7CiAgICAgdmFyIGZpbGVfZGlmZnMgPSAkKGNvbnZlcnRfbGluaykucGFyZW50cygnLkZpbGVE aWZmJyk7Ci0gICAgaWYgKCFmaWxlX2RpZmZzLnNpemUoKSkKKyAgICBpZiAoIWZpbGVfZGlmZnMu c2l6ZSgpKSB7CisgICAgICBsb2NhbFN0b3JhZ2UuZGlmZnN0YXRlID0gZGlmZnR5cGU7CiAgICAg ICBmaWxlX2RpZmZzID0gJCgnLkZpbGVEaWZmJyk7CisgICAgfQogCiAgICAgZmlsZV9kaWZmcy5l YWNoKGZ1bmN0aW9uKCkgewogICAgICAgY29udmVydEZpbGVEaWZmKGRpZmZ0eXBlLCB0aGlzKTsK QEAgLTczOSw4ICs3NDEsMTggQEAKICAgICAkKHJlc2l6ZV9pZnJhbWVbMF0uY29udGVudFdpbmRv dykuYmluZCgncmVzaXplJywgb25Cb2R5UmVzaXplKTsKIAogICAgIHVwZGF0ZVRvb2xiYXJBbmNo b3JTdGF0ZSgpOworICAgIHVwZGF0ZURpZmZTdGF0ZSgpOwogICB9KTsKIAorICBmdW5jdGlvbiB1 cGRhdGVEaWZmU3RhdGUoKSB7CisgICAgaWYgKGxvY2FsU3RvcmFnZS5kaWZmc3RhdGUgIT0gJ3Np ZGVieXNpZGUnKQorICAgICAgcmV0dXJuOworCisgICAgJCgnLkZpbGVEaWZmJykuZWFjaChmdW5j dGlvbigpIHsKKyAgICAgIGNvbnZlcnRGaWxlRGlmZignc2lkZWJ5c2lkZScsIHRoaXMpOworICAg IH0pOworICB9CisKICAgZnVuY3Rpb24gaXNEaWZmU2lkZUJ5U2lkZShmaWxlX2RpZmYpIHsKICAg ICByZXR1cm4gZGlmZlN0YXRlKGZpbGVfZGlmZikgPT0gJ3NpZGVieXNpZGUnOwogICB9Cg==