Bug 37886 - Hack up PrettyDiff to allow line-by-line comments.
Summary: Hack up PrettyDiff to allow line-by-line comments.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
URL: http://ascherkus.appspot.com/bugzilla...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-20 13:35 PDT by Adam Barth
Modified: 2010-04-22 02:37 PDT (History)
5 users (show)

See Also:


Attachments
le patch (3.99 KB, patch)
2010-04-20 13:38 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2010-04-20 23:58 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (145.87 KB, patch)
2010-04-21 00:06 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (145.69 KB, patch)
2010-04-21 00:08 PDT, Adam Barth
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-04-20 13:35:29 PDT
Demo at http://ascherkus.appspot.com/bugzilla/index.html
Comment 1 Adam Barth 2010-04-20 13:38:43 PDT
Created attachment 53876 [details]
le patch
Comment 2 David Levin 2010-04-20 13:49:51 PDT
Comment on attachment 53876 [details]
le patch

Only a "drive by" comment.

> Index: BugsSite/PrettyPatch/PrettyPatch.rb
> +<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/prototype/1.6.1.0/prototype.js"></script> 
> +<script>
> +// Code to support inline comments in bugs.webkit.org.
> +(funtion() {

function
Comment 3 Andrew Scherkus 2010-04-20 14:05:05 PDT
One thing to note is that in my demo I don't include the patch text in the comment box.  It starts off empty assuming you'll be double clicking and adding comments that way.

Maybe add a clear button or something?  Or just assume people will clear it themselves?
Comment 4 Adam Barth 2010-04-20 15:07:42 PDT
Comment on attachment 53876 [details]
le patch

Thanks!
Comment 5 Adam Barth 2010-04-20 23:58:42 PDT
Created attachment 53924 [details]
Patch
Comment 6 Adam Barth 2010-04-21 00:06:16 PDT
Created attachment 53925 [details]
Patch
Comment 7 Adam Barth 2010-04-21 00:08:59 PDT
Created attachment 53926 [details]
Patch
Comment 8 Mark Rowe (bdash) 2010-04-21 04:54:53 PDT
Is using Prototype really necessary?  It’s more than 10x longer than the JavaScript itself.  It doesn’t seem like the code would be much larger if it just used the DOM directly.
Comment 9 Adam Barth 2010-04-21 08:14:33 PDT
Prototype.js is certainly not necessary.  If we decide we like this approach, we can rewrite it to use dom directly.  Maciej and I discussed this issue over irc and our thinking was that we'd like to play with the feature for a bit before investing more.
Comment 10 Darin Adler 2010-04-21 09:31:18 PDT
Comment on attachment 53926 [details]
Patch

Do we really need a copy of prototype?

Lets give this a try!
Comment 11 Adam Roben (:aroben) 2010-04-21 09:32:16 PDT
Comment on attachment 53926 [details]
Patch

This will dump this code into the diffs that run-webkit-tests (both new- and old-) generate, too. Maybe we need a switch?
Comment 12 Adam Barth 2010-04-21 09:38:34 PDT
I originally had it refer to a copy of prototype.js on the Ajax Apis site, but Maciej didn't like the external dependency.

Adding a switch is fine, but I have zero idea how to read or write ruby.

Btw, here's another option for a bugzilla integrated review tool:

http://blog.fishsoup.net/2009/09/23/splinter-patch-review/
Comment 13 Eric Seidel (no email) 2010-04-21 18:15:43 PDT
Attachment 53926 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Comment 14 Adam Barth 2010-04-22 02:12:32 PDT
Committed r58075: <http://trac.webkit.org/changeset/58075>
Comment 15 WebKit Review Bot 2010-04-22 02:37:36 PDT
http://trac.webkit.org/changeset/58075 might have broken Leopard Intel Debug (Tests)