[webkit-reviews] review granted: [Bug 52019] side-by-side diffs in the code review tool : [Attachment 78282] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 52019: side-by-side diffs in the code review tool
https://bugs.webkit.org/show_bug.cgi?id=52019

Attachment 78282: Patch
https://bugs.webkit.org/attachment.cgi?id=78282&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78282&action=review

Yay!  Some minor jQuery nits below.

> Websites/bugs.webkit.org/code-review.js:502
> +    // Use textContent instead of innerHTML to avoid evaluting HTML.
> +    $('.text', line)[0].textContent = contents;

I think you can just write $('.text', line).text(contents) to avoid the nasty
[0] business.

> Websites/bugs.webkit.org/code-review.js:509
> +    var line = $('<div class="ExpansionLine"></div>')[0];
> +    line.appendChild(lineSide('from', contents, true, line_number));
> +    line.appendChild(lineSide('to', contents, true, line_number));

can you write line.append(lineSide('from', contents, true, line_number)) to
avoid the [0] business?

> Websites/bugs.webkit.org/code-review.js:534
> +    $('.text', line_side)[0].textContent = contents;

$('.text', line_side).text(contents)

> Websites/bugs.webkit.org/code-review.js:597
> -    return $('.text', line).text();
> +    // Just get the first match since a side-by-side diff has two lines with
text inside them for
> +    // unmodified lines in the diff.
> +    return $('.text', line)[0].textContent;

Interesting.

> Websites/bugs.webkit.org/code-review.js:766
> +    var new_line = $('<div ' + container_id + ' class="' + container_class +
'"></div>')[0];
> +    new_line.appendChild(lineSide('from', from_contents, false, from,
from_id, from_color_class));
> +    new_line.appendChild(lineSide('to', to_contents, false, to, to_id,
to_color_class));

$(yyy).append(xxx) might save the [0] here again.

> Websites/bugs.webkit.org/code-review.js:822
> +  $('.side-by-side-link').live('click', handleSideBySideLinkClick);
> +
> +  $('.ExpandLink').live('click', handleExpandLinkClick);
> +
>    $('.comment .discard').live('click', discardComment);
>  
>    $('.comment .ok').live('click', function() {

We can probably remove some of these blank lines.