[webkit-dev] Review tool changes

[Julie Parent]

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

Yup, this should be easy.  See http://trac.webkit.org/changeset/59265 for an
example of adding a new action and associated template.  Feel free to ping
me if you have any questions.


> > 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
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20100917/c5cbacba/attachment.html>