[webkit-dev] Review tool changes

[Adam Barth]

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.

>    2) It’s kind of crazy that the review tool’s URL is "...&action=prettypatch". It was nice of you to leave the old review tool unchanged, at least in part to placate me, but you’re squatting on another feature’s territory! I think we want a plain old formatted diff for other purposes. We need to get your tool out of there!
>
>    3) I suggest you make your review tool the default at "...&action=review" and move the old style review tool to another URL; we can put a link in yours.

Ok.  I might need some help modifying bugzilla to make that happen,
but hopefully Julie Parent will be willing to point me in the right
direction.

> I haven’t tried to use it on an iPad yet.

The tool has some problems on iPad.  The issue is the bottom toolbar
uses position: fixed, which seems to be frozen to the initial viewport
on iPad.  When you scroll, the bar stays in the middle of the page.  I
presume there's some way to fix this.  It's on my list.

On Thu, Sep 16, 2010 at 6:36 PM, Alexey Proskuryakov <ap at webkit.org> wrote:
> It's only now that I realized there's a new review tool at action=prettypatch :-)

Hopefully that means I didn't disrupt your use of action=prettypatch.  :)

> 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.

> One thing I'd love to see added is a back-link to a bug. I find myself using that fairly often currently.

A couple other folks requested this as well.  The complication here is
that navigating away from the tool will lose your in-progress edits.
My plan was to implement auto-save using localStorage first so that
the tool can restore your comments when you return.

On Thu, Sep 16, 2010 at 6:48 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> I do really like the layout of the new page. Seems like it will be really
> good for reviews.

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.

> (Maybe the "Publish" button should be labeled "Preview" to
> reduce needless nervousness.)

Will do.

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.

Adam