add ability to view for file context to the review tool
Created attachment 76570 [details] Patch
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.
Sorry for the size of this patch.
Seems it's time to have more than one javascript file. :)
(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 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?
Created attachment 76588 [details] Patch
Used jQuery for xhr and hasClass.
Screen shot? You're spoiling me in the other bug.
(In reply to comment #9) > Screen shot? You're spoiling me in the other bug. Already attached.
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?
Thanks for the quick review. All your review comments make sense. I'll make the changes and commit shortly.
Committed r74142: <http://trac.webkit.org/changeset/74142>