Bug 52019 – side-by-side diffs in the code review tool

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

side-by-side diffs in the code review tool

[Ojan Vafai]

Created attachment 78168 [details]
Patch

[Ojan Vafai]

Screenshots coming shortly...

[Ojan Vafai]

Comment on attachment 78168 [details]
Patch

Whoops. I have some style issues...new patch coming shortly.

[Ojan Vafai]

Created attachment 78171 [details]
Patch

[Ojan Vafai]

Created attachment 78172 [details]
screenshot

[Adam Barth]

Is this optional or required?

[Ojan Vafai]

(In reply to comment #6)
> Is this optional or required?

We still load the unified diff. If you click the side-by-side link in the top-right corner, then you get the side-by-side diff. I'll upload another screenshot of the initial view.

[Ojan Vafai]

Created attachment 78180 [details]
screenshot of top

[Adam Barth]

Comment on attachment 78171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78171&action=review

> Websites/bugs.webkit.org/code-review.js:395
> +    sideBySideify();

I've been using the "ify" for functions that take their parameter is the |this| variable.  This one seems to act globally.

> Websites/bugs.webkit.org/code-review.js:545
> +          '<span class="text">' + contents + '</span>' +

Is this going to cause problems if contents contains < or other HTML control characters?  Generally, it's better to build these things up using DOM instead of as strings.

> Websites/bugs.webkit.org/code-review.js:559
> +      // Use textContent instead of innerHTML to avoid evaluting HTML. Alternately, we could HTML escape.
> +      var contents = patched_file_contents[file_name][start_line_num + i];
> +      $('.text', line).each(function(index, element) { element.textContent = contents; });

Maybe you've solved that problem here?  I'm not sure I fully understand.

> Websites/bugs.webkit.org/code-review.js:703
> +            '<a href="javascript:" class="DiffLink" data-action="side-by-side">side-by-side</a>' +

This seems like a really strange way of making a link and adding an event handler.  Why not just use a class and a live event?

> Websites/bugs.webkit.org/code-review.js:740
> +    if (url_fragment['sidebyside'])
> +      return;
> +
> +    url_fragment['sidebyside'] = true;

Why not actually store this in location.hash?

> Websites/bugs.webkit.org/code-review.js:755
> +      var contents = lineHtmlContents(line);

Can we actually just move the DOM nodes over instead of converting them to strings and then back into DOM nodes?

> Websites/bugs.webkit.org/code-review.js:792
> +    var expansion_lines = $('.ExpansionLine').toArray();
> +    for (var i = 0, len = expansion_lines.length; i < len; i++) {

Why not use each() for these iteration patterns?

> Websites/bugs.webkit.org/code-review.js:814
> +      if (num_active_comments > 1)
> +        console.log("There is more than one active comment for " + line_id);

We probably should avoid spamming the console.

[Adam Barth]

I think this is a cool feature.  I'm slightly worried that we're sending things through the HTML parser too liberally, especially the contents of the patch.  I'd rather we used DOM more, especially text() / textContent because those are more predictable.

Also, I'd prefer to use the "jQuery way" as much as possible rather than stepping back into raw DOM / loops.

[Ojan Vafai]

Comment on attachment 78171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78171&action=review

>> Websites/bugs.webkit.org/code-review.js:395

> 
> I've been using the "ify" for functions that take their parameter is the |this| variable.  This one seems to act globally.

OK. I'll think of another name.

>> Websites/bugs.webkit.org/code-review.js:545

> 
> Is this going to cause problems if contents contains < or other HTML control characters?  Generally, it's better to build these things up using DOM instead of as strings.

I was careful that we only innerHTML html-escaped text, but I would feel safer never innerHTML'ing patch lines. I'll rework it.

>> Websites/bugs.webkit.org/code-review.js:703

> 
> This seems like a really strange way of making a link and adding an event handler.  Why not just use a class and a live event?

Sorry, I forget about jquery's crazy live events. Will fix.

>> Websites/bugs.webkit.org/code-review.js:740

> 
> Why not actually store this in location.hash?

Even if we put it in the hash, I think we'll want a parsed data structure for this. I'm picturing that we'll be adding a number of things to the hash here. In the spirit of keeping this patch small, I figured I could put that off for a future patch.

>> Websites/bugs.webkit.org/code-review.js:755

> 
> Can we actually just move the DOM nodes over instead of converting them to strings and then back into DOM nodes?

Ironically, I was trying to be more jquery friendly by doing it this way. I'll rework it to be more DOMy.

>> Websites/bugs.webkit.org/code-review.js:792

> 
> Why not use each() for these iteration patterns?

Habit. I forget jquery has this.

>> Websites/bugs.webkit.org/code-review.js:814

> 
> We probably should avoid spamming the console.

This should never happen. So, it's really logging an error. I'll change the log text to "Error: ..."

[Adam Barth]

Looks like you might have a bug about quoting context when replying to comments.  :)

[Ojan Vafai]

Created attachment 78281 [details]
Patch

[Ojan Vafai]

(In reply to comment #9)
> (From update of attachment 78171 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78171&action=review
> 
> > Websites/bugs.webkit.org/code-review.js:395
> > +    sideBySideify();
> 
> I've been using the "ify" for functions that take their parameter is the |this| variable.  This one seems to act globally.

I made these functions operate on each FileDiff, so the 'ify' makes sense now. :) I was planning on heading in that direction anyways since I want to make it possible to side-by-sideify individual FileDiffs.

> > Websites/bugs.webkit.org/code-review.js:703
> > +            '<a href="javascript:" class="DiffLink" data-action="side-by-side">side-by-side</a>' +
> 
> This seems like a really strange way of making a link and adding an event handler.  Why not just use a class and a live event?

Fixed this and the ExpandLink case.

> > Websites/bugs.webkit.org/code-review.js:740
> > +    if (url_fragment['sidebyside'])
> > +      return;
> > +
> > +    url_fragment['sidebyside'] = true;
> 
> Why not actually store this in location.hash?

Still left this as a FIXME, although now I store a bit for each FileDiff section. This is laying the groundwork for being able to convert just a single FileDiff to be side-by-side.

> > Websites/bugs.webkit.org/code-review.js:755
> > +      var contents = lineHtmlContents(line);
> 
> Can we actually just move the DOM nodes over instead of converting them to strings and then back into DOM nodes?

I don't think this buys us much and it complicates the code because we would need to treat the case of expansion lines (where we just have raw text) differently.

(In reply to comment #10)
> I think this is a cool feature.  I'm slightly worried that we're sending things through the HTML parser too liberally, especially the contents of the patch.  I'd rather we used DOM more, especially text() / textContent because those are more predictable.

I used textContent for all the actual content, but still used HTML for the structural bits. Definitely better than what I had before. Doing the whole construction in the DOM is a bit too cumbersome and inscrutable IMO.

> Also, I'd prefer to use the "jQuery way" as much as possible rather than stepping back into raw DOM / loops.

Tried to do this.

(In reply to comment #12)
> Looks like you might have a bug about quoting context when replying to comments.  :)

Fixed in a separate patch.

[Ojan Vafai]

Created attachment 78282 [details]
Patch

[Adam Barth]

Comment on attachment 78282 [details]
Patch

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.

[Adam Barth]

You should also bump the version parameter in the URL.

[Ojan Vafai]

Committed r75295: <http://trac.webkit.org/changeset/75295>