Re: [webkit-dev] Review tool changes

[Chris Marrin]

On Sep 20, 2010, at 11:36 AM, Maciej Stachowiak wrote:

> 
> On Sep 20, 2010, at 11:32 AM, Darin Adler wrote:
> 
>> On Sep 20, 2010, at 10:22 AM, Darin Fisher wrote:
>> 
>>> How about this?
>>> 
>>> If any annotations were made to the patch, then "the button" gets named 
>>> Preview.  Else, the button is named "Publish" and when clicked performs its 
>>> work in one shot.
>>> 
>>> Was there a strong outcry for removing the preview step?  I only found it 
>>> bothersome when I wanted to issue a quick r=me on a patch that didn't 
>>> require any additional changes.
>> 
>> Good idea. Here’s another idea:
>> 
>> The button starts out named Preview and works with a preview.
>> 
>> If the review or commit-queue flag is altered then the button is changed to 
>> “Publish” and works without a preview.
>> 
>> Thus if I like the preview work flow, I don’t set those flags until I get to 
>> the preview page. If I like the faster work flow, I can set a flag and then 
>> push Publish.
>> 
>> One type of person who is not served by my proposal is someone who wants to 
>> add comments only and not set a flag but wants the faster work flow.
> 
> 
> Changing the button based on seemingly unrelated flag settings does not 
> strike me as clear UI design.
> 
> If people really want to be able to review a patch without the extra preview 
> step, then I think two buttons would be preferable to a solution where the 
> button changes based on something you did while reviewing. But I also think 
> the always-preview workflow is entirely reasonable.

Yeah, I much prefer previewing. It gives you a chance to check your work and to 
see what exactly will be posted. And it gives you a chance to add general 
comments to the review. I think it's a mistake to allow blind publishing of a 
review. It will cause lots of mistakes to get added to the bug traffic and 
increase the noise level.

-
~Chris
cmar...@apple.com




___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Chris Marrin]

On Sep 20, 2010, at 11:36 AM, Adam Roben wrote:

> On Sep 20, 2010, at 2:34 PM, Oliver Hunt wrote:
> 
>> I really would like to be able to select some text and add a comment that 
>> uses the selection as context, a single line of context is frequently 
>> insufficient, this is about the only thing that still makes the new review 
>> tool less effective than the old review mechanism (for me at least).
> 
> This already works. Just click-and-drag on the line numbers of the lines you 
> want to include in the context.

Ah, I didn't know about this. Very cool. This will make comments so much more 
clear!

-
~Chris
cmar...@apple.com




___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Maciej Stachowiak]

On Sep 20, 2010, at 11:32 AM, Darin Adler wrote:

> On Sep 20, 2010, at 10:22 AM, Darin Fisher wrote:
> 
>> How about this?
>> 
>> If any annotations were made to the patch, then "the button" gets named 
>> Preview.  Else, the button is named "Publish" and when clicked performs its 
>> work in one shot.
>> 
>> Was there a strong outcry for removing the preview step?  I only found it 
>> bothersome when I wanted to issue a quick r=me on a patch that didn't 
>> require any additional changes.
> 
> Good idea. Here’s another idea:
> 
> The button starts out named Preview and works with a preview.
> 
> If the review or commit-queue flag is altered then the button is changed to 
> “Publish” and works without a preview.
> 
> Thus if I like the preview work flow, I don’t set those flags until I get to 
> the preview page. If I like the faster work flow, I can set a flag and then 
> push Publish.
> 
> One type of person who is not served by my proposal is someone who wants to 
> add comments only and not set a flag but wants the faster work flow.


Changing the button based on seemingly unrelated flag settings does not strike 
me as clear UI design.

If people really want to be able to review a patch without the extra preview 
step, then I think two buttons would be preferable to a solution where the 
button changes based on something you did while reviewing. But I also think the 
always-preview workflow is entirely reasonable.

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Adam Roben]

On Sep 20, 2010, at 2:34 PM, Oliver Hunt wrote:

> I really would like to be able to select some text and add a comment that 
> uses the selection as context, a single line of context is frequently 
> insufficient, this is about the only thing that still makes the new review 
> tool less effective than the old review mechanism (for me at least).

This already works. Just click-and-drag on the line numbers of the lines you 
want to include in the context.

-Adam

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Oliver Hunt]

I really would like to be able to select some text and add a comment that uses 
the selection as context, a single line of context is frequently insufficient, 
this is about the only thing that still makes the new review tool less 
effective than the old review mechanism (for me at least).

--Oliver

On Sep 20, 2010, at 11:32 AM, Darin Adler wrote:

> On Sep 20, 2010, at 10:22 AM, Darin Fisher wrote:
> 
>> How about this?
>> 
>> If any annotations were made to the patch, then "the button" gets named 
>> Preview.  Else, the button is named "Publish" and when clicked performs its 
>> work in one shot.
>> 
>> Was there a strong outcry for removing the preview step?  I only found it 
>> bothersome when I wanted to issue a quick r=me on a patch that didn't 
>> require any additional changes.
> 
> Good idea. Here’s another idea:
> 
> The button starts out named Preview and works with a preview.
> 
> If the review or commit-queue flag is altered then the button is changed to 
> “Publish” and works without a preview.
> 
> Thus if I like the preview work flow, I don’t set those flags until I get to 
> the preview page. If I like the faster work flow, I can set a flag and then 
> push Publish.
> 
> One type of person who is not served by my proposal is someone who wants to 
> add comments only and not set a flag but wants the faster work flow.
> 
>-- Darin
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Maciej Stachowiak]

On Sep 20, 2010, at 10:22 AM, Darin Fisher wrote:

> On Mon, Sep 20, 2010 at 10:10 AM, Adam Barth  wrote:
> On Mon, Sep 20, 2010 at 8:37 AM, Alexey Proskuryakov  wrote:
> > 16.09.2010, в 18:39, Darin Fisher написал(а):
> >> Push the publish button to review your comments :-)
> >
> > Alas, not any more!
> >
> > https://bugs.webkit.org/show_bug.cgi?id=46074
> 
> Yeah.  The machinery is still there for the preview, I'm just not sure
> what the best UI is for triggering it.
> 
> Adam
> 
> 
> How about this?
> 
> If any annotations were made to the patch, then "the button" gets named 
> Preview.  Else, the button is named "Publish" and when clicked performs its 
> work in one shot.
> 
> Was there a strong outcry for removing the preview step?  I only found it 
> bothersome when I wanted to issue a quick r=me on a patch that didn't require 
> any additional changes.


How about having both [Publish] and [Preview] buttons?

Or honestly, I think it would be fine to have just [Preview]. If a blog can 
require you to check what you posted before it appears in comments, then I 
think it is a reasonable requirement for a patch review system.

Regards,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Darin Adler]

On Sep 20, 2010, at 10:22 AM, Darin Fisher wrote:

> How about this?
> 
> If any annotations were made to the patch, then "the button" gets named 
> Preview.  Else, the button is named "Publish" and when clicked performs its 
> work in one shot.
> 
> Was there a strong outcry for removing the preview step?  I only found it 
> bothersome when I wanted to issue a quick r=me on a patch that didn't require 
> any additional changes.

Good idea. Here’s another idea:

The button starts out named Preview and works with a preview.

If the review or commit-queue flag is altered then the button is changed to 
“Publish” and works without a preview.

Thus if I like the preview work flow, I don’t set those flags until I get to 
the preview page. If I like the faster work flow, I can set a flag and then 
push Publish.

One type of person who is not served by my proposal is someone who wants to add 
comments only and not set a flag but wants the faster work flow.

-- Darin

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Darin Fisher]

On Mon, Sep 20, 2010 at 10:10 AM, Adam Barth  wrote:

> On Mon, Sep 20, 2010 at 8:37 AM, Alexey Proskuryakov 
> wrote:
> > 16.09.2010, в 18:39, Darin Fisher написал(а):
> >> Push the publish button to review your comments :-)
> >
> > Alas, not any more!
> >
> > https://bugs.webkit.org/show_bug.cgi?id=46074
>
> Yeah.  The machinery is still there for the preview, I'm just not sure
> what the best UI is for triggering it.
>
> Adam



How about this?

If any annotations were made to the patch, then "the button" gets named
Preview.  Else, the button is named "Publish" and when clicked performs its
work in one shot.

Was there a strong outcry for removing the preview step?  I only found it
bothersome when I wanted to issue a quick r=me on a patch that didn't
require any additional changes.

-Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Adam Barth]

On Mon, Sep 20, 2010 at 8:37 AM, Alexey Proskuryakov  wrote:
> 16.09.2010, в 18:39, Darin Fisher написал(а):
>> Push the publish button to review your comments :-)
>
> Alas, not any more!
>
> https://bugs.webkit.org/show_bug.cgi?id=46074

Yeah.  The machinery is still there for the preview, I'm just not sure
what the best UI is for triggering it.

Adam
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Alexey Proskuryakov]

16.09.2010, в 18:39, Darin Fisher написал(а):
> Push the publish button to review your comments :-)
> 

Alas, not any more!

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

- WBR, Alexey Proskuryakov


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Adam Barth]

On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler  wrote:
>    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.

I've moved the new tool from Pretty Diff to Review Patch.  Let me know
if you still want a link to the old tool.

Adam
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[David Kilzer]

On Sep 17, 2010, at 6:38 AM, Mike Pinkerton  wrote:

> On Fri, Sep 17, 2010 at 2:39 AM, Adam Barth  wrote:
> 
>> 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.
> 
> A lot of sites have this problem, it seems. What is it about the iPad
> in particular that causes this issue? I doubt it's a webkit "bug" but
> it manifests itself as only happening on the iPad so most site
> designers end up not caring to fix, or not being able to diagnose the
> problem for lack of hardware. I frequent at least two sites with this
> behavior.

It has to do with how iOS WebKit defines the viewport.



Dave
--
Sent from my iPhone 4

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Eric Uhrhane]

On Thu, Sep 16, 2010 at 11:58 PM, Ojan Vafai  wrote:
> On Fri, Sep 17, 2010 at 4:39 PM, Adam Barth  wrote:
>>
>> On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler  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  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@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Mike Pinkerton]

On Fri, Sep 17, 2010 at 2:39 AM, Adam Barth  wrote:

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

A lot of sites have this problem, it seems. What is it about the iPad
in particular that causes this issue? I doubt it's a webkit "bug" but
it manifests itself as only happening on the iPad so most site
designers end up not caring to fix, or not being able to diagnose the
problem for lack of hardware. I frequent at least two sites with this
behavior.

-- 
Mike Pinkerton
Mac Weenie
pinker...@google.com
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Kenneth Christiansen]

I just want to say that I absolutely love the new review tool and the
ability to select multiply lines of contents!

Kenneth

On Fri, Sep 17, 2010 at 4:11 AM, Darin Fisher  wrote:
> On Fri, Sep 17, 2010 at 12:05 AM, Alexey Proskuryakov  wrote:
>>
>> 16.09.2010, в 23:39, Adam Barth написал(а):
>>
>> > 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.
>>
>> It seems that for most easy reviews, the need to click twice will be a
>> nuisance once you know how the tool works. But for larger reviews, having a
>> look at the final text can be beneficial (just like you look over a patch
>> just before uploading it).
>
> Yes, this describes my current experience with the tool.  If I set the
> review flag to r+ and have made no comments on the patch, then I'd like a
> button to press that will just submit my r+ without any preview or further
> confirmation step.
> That said, this is just a small nit-pick.  Personally, I find the existing
> tool to be a vast improvement already over what we had before.
> -Darin
>
>>
>> >> 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.
>>
>>
>> I only want this in order to open the bug in a new tab, so a _blank target
>> would sidestep the problem.
>>
>> - WBR, Alexey Proskuryakov
>>
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
>



-- 
Kenneth Rohde Christiansen
Technical Lead / Senior Software Engineer
Qt Labs Americas, Nokia Technology Institute, INdT
Phone  +55 81 8895 6002 / E-mail kenneth.christiansen at openbossa.org
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Darin Fisher]

On Fri, Sep 17, 2010 at 12:05 AM, Alexey Proskuryakov  wrote:

>
> 16.09.2010, в 23:39, Adam Barth написал(а):
>
> > 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.
>
> It seems that for most easy reviews, the need to click twice will be a
> nuisance once you know how the tool works. But for larger reviews, having a
> look at the final text can be beneficial (just like you look over a patch
> just before uploading it).
>

Yes, this describes my current experience with the tool.  If I set the
review flag to r+ and have made no comments on the patch, then I'd like a
button to press that will just submit my r+ without any preview or further
confirmation step.

That said, this is just a small nit-pick.  Personally, I find the existing
tool to be a vast improvement already over what we had before.

-Darin



>
> >> 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.
>
>
> I only want this in order to open the bug in a new tab, so a _blank target
> would sidestep the problem.
>
> - WBR, Alexey Proskuryakov
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Alexey Proskuryakov]

16.09.2010, в 23:39, Adam Barth написал(а):

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

It seems that for most easy reviews, the need to click twice will be a nuisance 
once you know how the tool works. But for larger reviews, having a look at the 
final text can be beneficial (just like you look over a patch just before 
uploading it).

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


I only want this in order to open the bug in a new tab, so a _blank target 
would sidestep the problem.

- WBR, Alexey Proskuryakov

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Ojan Vafai]

On Fri, Sep 17, 2010 at 4:39 PM, Adam Barth  wrote:

> On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler  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  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@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Julie Parent]

On Fri, Sep 17, 2010 at 4:39 PM, Adam Barth  wrote:

> On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler  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 
> 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  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  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@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Adam Barth]

On Thu, Sep 16, 2010 at 5:33 PM, Darin Adler  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  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  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  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@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Ryosuke Niwa]

On Thu, Sep 16, 2010 at 6:48 PM, Maciej Stachowiak  wrote:

> On Sep 16, 2010, at 6:25 PM, Ojan Vafai wrote:
>
> On Fri, Sep 17, 2010 at 10:33 AM, Darin Adler  wrote:
>
>> 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!
>>
>
> What's crazy about this? What are the other uses of the plain formatted
> diff that aren't met by this? I agree with the new tool replacing the old
> one. I just don't see the benefits of keeping both the new review tool and
> the formatted diff page.
>
> Sometimes you just want a formatted view of the patch. To give one example,
> Cmd+A Cmd+V does not do the right thing on this page for someone who just
> wants the patch.
>

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

I do really like the layout of the new page. Seems like it will be really
> good for reviews. (Maybe the "Publish" button should be labeled "Preview" to
> reduce needless nervousness.)
>

I second that.

- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Maciej Stachowiak]

On Sep 16, 2010, at 6:25 PM, Ojan Vafai wrote:

> On Fri, Sep 17, 2010 at 10:33 AM, Darin Adler  wrote:
> 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!
> 
> What's crazy about this? What are the other uses of the plain formatted diff 
> that aren't met by this? I agree with the new tool replacing the old one. I 
> just don't see the benefits of keeping both the new review tool and the 
> formatted diff page.

Sometimes you just want a formatted view of the patch. To give one example, 
Cmd+A Cmd+V does not do the right thing on this page for someone who just wants 
the patch. For another, the row of controls at the bottom may be distracting. 
In brief, sometimes you want "more" instead of "vi".

I do really like the layout of the new page. Seems like it will be really good 
for reviews. (Maybe the "Publish" button should be labeled "Preview" to reduce 
needless nervousness.)

Cheers,
Maciej

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Darin Fisher]

Push the publish button to review your comments :-)

On Sep 16, 2010 6:36 PM, "Alexey Proskuryakov"  wrote:
>
> 16.09.2010, в 17:33, Darin Adler написал(а):
>
>> 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.
>
>
> It's only now that I realized there's a new review tool at
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.
>
> One thing I'd love to see added is a back-link to a bug. I find myself
using that fairly often currently.
>
> - WBR, Alexey Proskuryakov
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Alexey Proskuryakov]

16.09.2010, в 17:33, Darin Adler написал(а):

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


It's only now that I realized there's a new review tool at 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.

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

- WBR, Alexey Proskuryakov

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

Re: [webkit-dev] Review tool changes

[Ojan Vafai]

On Fri, Sep 17, 2010 at 10:33 AM, Darin Adler  wrote:

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

What's crazy about this? What are the other uses of the plain formatted diff
that aren't met by this? I agree with the new tool replacing the old one. I
just don't see the benefits of keeping both the new review tool and the
formatted diff page.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev

[webkit-dev] Review tool changes

[Darin Adler]

Hi Adam.

A few thoughts on the review tools on bugs.webkit.org.

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.

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.

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

-- Darin

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev