Bug 51057 - add ability to view for file context to the review tool
Summary: add ability to view for file context to the review tool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-14 14:31 PST by Ojan Vafai
Modified: 2010-12-15 14:51 PST (History)
2 users (show)

See Also:


Attachments
Patch (16.10 KB, patch)
2010-12-14 14:35 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
screenshot with expand links and one expanded section (81.93 KB, image/png)
2010-12-14 14:37 PST, Ojan Vafai
no flags Details
Patch (15.71 KB, patch)
2010-12-14 15:59 PST, Ojan Vafai
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-12-14 14:31:50 PST
add ability to view for file context to the review tool
Comment 1 Ojan Vafai 2010-12-14 14:35:14 PST
Created attachment 76570 [details]
Patch
Comment 2 Ojan Vafai 2010-12-14 14:37:24 PST
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.
Comment 3 Ojan Vafai 2010-12-14 14:37:53 PST
Sorry for the size of this patch.
Comment 4 Eric Seidel (no email) 2010-12-14 14:39:16 PST
Seems it's time to have more than one javascript file. :)
Comment 5 Ojan Vafai 2010-12-14 14:43:27 PST
(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.
Comment 6 Adam Barth 2010-12-14 14:45:46 PST
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?
Comment 7 Ojan Vafai 2010-12-14 15:59:31 PST
Created attachment 76588 [details]
Patch
Comment 8 Ojan Vafai 2010-12-14 16:00:02 PST
Used jQuery for xhr and hasClass.
Comment 9 Adam Barth 2010-12-15 10:27:21 PST
Screen shot?  You're spoiling me in the other bug.
Comment 10 Ojan Vafai 2010-12-15 10:33:34 PST
(In reply to comment #9)
> Screen shot?  You're spoiling me in the other bug.

Already attached.
Comment 11 Adam Barth 2010-12-15 10:40:19 PST
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?
Comment 12 Ojan Vafai 2010-12-15 10:44:00 PST
Thanks for the quick review. All your review comments make sense. I'll make the changes and commit shortly.
Comment 13 Ojan Vafai 2010-12-15 14:51:56 PST
Committed r74142: <http://trac.webkit.org/changeset/74142>