[Webkit-unassigned] [Bug 41486] New: rietveld review output is difficult to read

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41486

           Summary: rietveld review output is difficult to read
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Mac OS X 10.5
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Tools / Tests
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: ojan at chromium.org
                CC: eric at webkit.org, darin at apple.com, abarth at webkit.org,
                    jparent at google.com


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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.