RESOLVED FIXED 41486
rietveld review output is difficult to read 
https://bugs.webkit.org/show_bug.cgi?id=41486
Summary rietveld review output is difficult to read
Ojan Vafai
Reported 2010-07-01 14:30:24 PDT
See https://bugs.webkit.org/show_bug.cgi?id=30116#c26 as an example. Darin says that's difficult to read. Is there something we could do to make this output more readable? There's a couple issues I see with the output: 1. Lack of context in the bug. To see the comments in context, you would need to click on one of the links. The downside is that you need to load a new page, the plus is that you get a lot more context that way. 2. The output is too verbose. Are there other issues with it? Not sure exactly what we could do to provide more context. I suppose we could modify the rietveld UI to allow you to specify a comment applies to a number of lines instead of just one. That's certainly possible, but would require non-trivial rietveld hacking. I'd be happy to guide someone who wants to do some web development hacking on how to make such a change. We could always include a couple lines of context, but that would make the output even more verbose. As far as verbosity, currently we output the file and then the individual comments. We could remove the file output and just put the individual comments. Something like: http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode13 LayoutTests/editing/inserting/return-key-in-hidden-field.html:13: } Nit: technically by webkit style this (one-line if) shouldn't have curly braces. http://wkrietveld.appspot.com/30116/diff/2001/3003#newcode30 LayoutTests/editing/inserting/return-key-in-hidden-field.html:30: layoutTestController.dumpAsText(); 28-30 duplicates 20-22, was that intentional? I think you can just remove this. http://wkrietveld.appspot.com/30116/diff/2001/3004#newcode7 WebCore/ChangeLog:7: Maybe add a comment saying that this causes text insertions on visibility:hidden elements to get ignored and add a link to bug 40342? http://wkrietveld.appspot.com/30116/diff/2001/3005#newcode97 WebCore/editing/InsertLineBreakCommand.cpp:97: VisiblePosition caret(pos); I thought the conclusion was to make this a null-check with a comment pointing to bug 40342 instead? Even better would be if we could post HTML to bugzilla comments. Then we could do away with those long links. Another option for the verbosity issue would be to just have a link per file, not per comment. Something like: LayoutTests/editing/inserting/return-key-in-hidden-field.html http://wkrietveld.appspot.com/30116/diff/2001/3003 Line 13: } Nit: technically by webkit style this (one-line if) shouldn't have curly braces. Line 30: layoutTestController.dumpAsText(); 28-30 duplicates 20-22, was that intentional? I think you can just remove this. WebCore/ChangeLog http://wkrietveld.appspot.com/30116/diff/2001/3004 Line 7: Maybe add a comment saying that this causes text insertions on visibility:hidden elements to get ignored and add a link to bug 40342? File WebCore/editing/InsertLineBreakCommand.cpp http://wkrietveld.appspot.com/30116/diff/2001/3005 Line 97: VisiblePosition caret(pos); I thought the conclusion was to make this a null-check with a comment pointing to bug 40342 instead?
Attachments
Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 2010-08-23 15:44:25 PDT
I've changed the output so it will look like the following (grabbed off my local server). I hope this addresses all of Darin's concerns. There's a few integration issues with the upload bot. Once those are resolved, we should be able to test out people optionally using rietveld again. codereview/views.py(right) http://localhost:8080/1000/diff/1/2 Lines 941-942: > html = "<body style='margin:0'><a href='%s?webkit_patch_id=%s'>%s</a></body>" % ( > issue_url, webkit_patch_id, "Rietveld Review") This is gross. Don't put HTML into your python. Make this a template instead. Lines 971-976: > > raw_webkit_patch_id = form.cleaned_data['webkit_patch_id'] > if raw_webkit_patch_id: > webkit_patch_id = str(raw_webkit_patch_id) > else: > form.errors['webkit_patch_id'] = ['Need a patch id for webkit patches.'] This is a comment. Line 996: > issue = _make_new(request, form, issue_id, webkit_patch_id) This is a one line comment. upload.py(right) http://localhost:8080/1000/diff/1/6 Line 510: > group.add_option("--webkit_patch_id", action="store", dest="webkit_patch_id", This is a second file comment. Line 1706: > form_fields.append(("webkit_patch_id", str(options.webkit_patch_id))) Here's a comment in another file.
Note You need to log in before you can comment on or make changes to this bug.