[webkit-dev] Review tool changes

[Eric Uhrhane]

On Thu, Sep 16, 2010 at 11:58 PM, Ojan Vafai <ojan at chromium.org> wrote:
> On Fri, Sep 17, 2010 at 4:39 PM, Adam Barth <abarth at webkit.org> wrote:
>>
>> On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler <darin at apple.com> wrote:
>> >    1) I am happy with the review tool. I have been using it for a lot of
>> > reviews. There may be no one left who prefers the old review page.
>>
>> Thanks.  Please let me know if you have ideas for how to improve the
>> tool.  One thing Ojan suggested was the ability to expand the context.
>>  That would be tricky to implement, but it might be possible now that
>> http://svn.webkit.org/repository/webkit/ has CORS enabled.
>
> I also want the option to see side-by-side diffs. There are some patches
> where side-by-side is immensely easier to make sense of.

+1

Great work so far, though; this is so much nicer already.

>> > Is there a way to preview comments before publishing? I'm a little
>> > hesitant to push that button without seeing what will happen.
>> As mentioned above, the "publish" button actually brings up a
>> confirmation screen.  My original plan was to eventually remove the
>> confirmation screen, since it's fully redundant, but I can leave it if
>> folks find it useful.
>
> I prefer avoiding the confirmation screen and instead having a preview
> button. It's only confusing the first or second time you use the tool.
> Whereas needing to do two clicks quickly becomes annoying.
>>
>> One suggestion I've received is to put the "overall comments" box on
>> the toolbar so that you can accumulate overall comments as you read
>> through the patch.  My feeling is that might make the toolbar too
>> tall, but I'd welcome other thoughts on that topic.
>
> This also was my suggestion. I think this would work as long as there is a
> button or something to collapse the overall comments box.
>
>>
>> > (Maybe the "Publish" button should be labeled "Preview" to
>> > reduce needless nervousness.)
>>
>> Will do.
>
> Two buttons at the same time!
>
>>
>> On Thu, Sep 16, 2010 at 7:46 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>> > Yeah I often use that to get a part of patch that I posted on bugzilla.
>> >  e.g. I post some work in progress in bugzilla but decide to   change my
>> > approach.  But I can still make a use of some changes in my original
>> > patch,
>> > so I just copy & paste from pretty diff and then remove line numbers on
>> > TextMate and paste it on XCode.  (Let me know if there's a better way of
>> > doing this sort of stuff).
>>
>> The root problem here is the way the PrettyPatch DOM is structured the
>> line element contains both the code and the line number.  If all the
>> code was in one container and all the line numbers in another
>> container, you could copy the code without copying the line numbers.
>
> I'd love it if we changed this.
> Ojan
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>