[webkit-dev] Review tool changes

[Ojan Vafai]

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.


> > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100917/5831fa5d/attachment.html>