Bug 37886 – Hack up PrettyDiff to allow line-by-line comments.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Barth]

Demo at http://ascherkus.appspot.com/bugzilla/index.html

[Adam Barth]

Created attachment 53876 [details]
le patch

[David Levin]

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

[Andrew Scherkus]

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?

[Adam Barth]

Comment on attachment 53876 [details]
le patch

Thanks!

[Adam Barth]

Created attachment 53924 [details]
Patch

[Adam Barth]

Created attachment 53925 [details]
Patch

[Adam Barth]

Created attachment 53926 [details]
Patch

[Mark Rowe (bdash)]

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.

[Adam Barth]

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.

[Darin Adler]

Comment on attachment 53926 [details]
Patch

Do we really need a copy of prototype?

Lets give this a try!

[Adam Roben (:aroben)]

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?

[Adam Barth]

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/

[Eric Seidel (no email)]

Attachment 53926 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.

[Adam Barth]

Committed r58075: <http://trac.webkit.org/changeset/58075>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58075 might have broken Leopard Intel Debug (Tests)