Bug 51057 – add ability to view for file context to the review tool

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

add ability to view for file context to the review tool

[Ojan Vafai]

Created attachment 76570 [details]
Patch

[Ojan Vafai]

Created attachment 76571 [details]
screenshot with expand links and one expanded section

Note that you cannot leave comments on the expanded context. We could change that in the future, but this seems like a good first step. As a result, the expanded areas are grey to indicate the inability to comment.

[Ojan Vafai]

Sorry for the size of this patch.

[Eric Seidel (no email)]

Seems it's time to have more than one javascript file. :)

[Ojan Vafai]

(In reply to comment #4)
> Seems it's time to have more than one javascript file. :)

I thought about that. Each file we add slows down the load of the review tool though. Not sure it's worth it.

[Adam Barth]

Comment on attachment 76570 [details]
Patch

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

Mind = blown.  Will look in detail later.

> BugsSite/code-review.js:61
> +  function xhr(url, onLoad, onError) {
> +    var req = new XMLHttpRequest();
> +    req.open('GET', url, true);
> +    req.onreadystatechange = function() {
> +      if (req.readyState == 4) {
> +        if(req.status == 200)
> +          onLoad(req.responseText);
> +        else
> +          onError(url);
> +      }
> +    };
> +    req.send(null);
> +  }

Shouldn't we just use jQuery for this?

[Ojan Vafai]

Created attachment 76588 [details]
Patch

[Ojan Vafai]

Used jQuery for xhr and hasClass.

[Adam Barth]

Screen shot?  You're spoiling me in the other bug.

[Ojan Vafai]

(In reply to comment #9)
> Screen shot?  You're spoiling me in the other bug.

Already attached.

[Adam Barth]

Comment on attachment 76588 [details]
Patch

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?

[Ojan Vafai]

Thanks for the quick review. All your review comments make sense. I'll make the changes and commit shortly.

[Ojan Vafai]

Committed r74142: <http://trac.webkit.org/changeset/74142>