[webkit-reviews] review granted: [Bug 51057] add ability to view for file context to the review tool : [Attachment 76588] 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 51057: add ability to view for file context to the review tool
https://bugs.webkit.org/show_bug.cgi?id=51057

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

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

This patch is extremely impressive.  Thanks Ojan.  Overall, it looks like
you're fighting jQuery a bit.  I've noted a couple places where you're doing
things manually that jQuery should be able to automate for you.

The only thing that really bothers me about this patch is the place where you
stick something in the global scope.  We shouldn't need to do that if we use
the DOM to attach the events.  There are two approaches we can use for that:

1) Add the events to the DOM elements as we're constructing the context
expanding links.
2) Using the "live" feature of jQuery to attach the events preemptively.

In other places in this tool, we've used approach (2).	If you need to pass
additional information (e.g., arguments) to the handler, you can do that using
HTML5 "data-" attributes.  I'm giving this patch an R+ because it's so
impressive, but I'd like you to address these two points before landing.

Thanks!

> BugsSite/PrettyPatch/PrettyPatch.rb:116
> +color: #039;

Missing indent.

> BugsSite/code-review.js:295
> +    var contexts = file_diff.getElementsByClassName('context');
> +    var context_line;
> +    while (context_line = contexts[0]) {
> +	 context_line.parentNode.removeChild(context_line);
> +    }

This looks like something we should be able to do with a jQuery selector.

> BugsSite/code-review.js:309
> +    var section_separators = file_diff.getElementsByTagName('br');
> +    var separator;
> +    while (separator = section_separators[0]) {
> +	 separator.parentNode.replaceChild(createExpandBar(file_name,
expand_bar_index++), separator);
> +    }

Here too?

> BugsSite/code-review.js:352
> +  function expandLinkHtml(file_name, direction, expand_bar_index, amount) {
> +    return "<a href='javascript:' onclick='handleExpand(\"" +
> +	   file_name + "\", \"" + direction + "\", " + expand_bar_index + ", "
+ amount + ")'>" +
> +	   (amount ? amount + " " : "") + direction + "</a>";
> +  }
> +
> +  // Use window's namespace so the expand links can call this function.
> +  window['handleExpand'] = function (file_name, direction, expand_bar_index,
amount) {

If we build this using DOM instead of strings, we can use attach the event
using the DOM and then we don't need to pollute the global scope.

> BugsSite/code-review.js:383
> +    alert("Can't expand " + file_name + ". Is this a new or deleted file?");


Can we show this error in the page instead of using alert?