RFR: CODETOOLS-7901044: Add next/prev links to navigate between diffs

[John Coomes]

Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
> 
> Hi Daniel,
> 
> On 10/1/14 4:27 PM, Daniel Fuchs wrote:
> > Hi Bengt,
> >
> > The new version looks good to me, but I am not a reviewer
> > fore CODE_TOOLS. 
> 
> Thanks for looking at this Daniel!
> 
> > Hopefully someone who is will step in :-)
> 
> Yes, and I am not a committer in code-tools so I need a sponsor for the 
> patch too.

Looks good to me; I'll sponsor it.  Send me (privately) the result of
"hg export --git" and I'll push that.

-John

> > best regards,
> >
> > -- daniel
> >
> > On 01/10/14 15:57, Bengt Rutisson wrote:
> >>
> >> Hi Daniel,
> >>
> >> Thanks for looking at this!
> >>
> >> On 2014-10-01 15:53, Daniel Fuchs wrote:
> >>> Hi Bengt,
> >>>
> >>> This is a really cool feature!
> >>
> >> Thanks!
> >>
> >>>
> >>> I have downloaded your patch and applied it in my
> >>> local webrev repo - and played with the new
> >>> webrev in a couple of repositories where I have
> >>> pending uncommitted changes.
> >>>
> >>> This seems to work well.
> >>>
> >>> I have used:
> >>>
> >>>   webrev -fN from top level repo
> >>>   webrev -N  from jdk repo
> >>>   webrev -fN from jdk repo (with changes in test/closed)
> >>>
> >>
> >> Great that you tired it out.
> >>
> >>> I also had a cursory look at your patch. The only thing
> >>> I noticed is that 'typeset' is not consistently used in
> >>> the original webrev file. I'm not sure whether that's
> >>> intended or not.
> >>> I believe that's ok - but I've been bitten by not using
> >>> typeset before ;-( - maybe - for the sake of uniformity -
> >>> you could use typeset at least in those functions that
> >>> already use  it for their parameters? For instance
> >>> lines 532-534, and possibly line 560 as well?
> >>> Or did you intentionally not used it?
> >>
> >> I also noticed the inconsistent use of typeset. I fixed the lines you
> >> pointed out. I wasn't sure which way to go. But I think your suggestion
> >> is good.
> >>
> >>>
> >>> Also you should increment the version of webrev.
> >>> IIRC that would be 25.6:
> >>>
> >>> - WEBREV_UPDATED=25.5-hg+openjdk.java.net
> >>> + WEBREV_UPDATED=25.6-hg+openjdk.java.net
> >>
> >> Thanks, didn't think of that.
> >>
> >> New webrev:
> >> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.01/
> >>
> >> Diff compared to the first version:
> >> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00-01.diff/ 
> >>
> >>
> >> Thanks,
> >> Bengt
> >>
> >>>
> >>> best regards,
> >>>
> >>> -- daniel
> >>>
> >>> On 01/10/14 10:56, Bengt Rutisson wrote:
> >>>>
> >>>> Hi everyone,
> >>>>
> >>>> (This is a patch for webrev, but I am cross posting to the JDK 9 dev
> >>>> list since I'm not sure how many are on the webrev-dev list and 
> >>>> most JDK
> >>>> developers might have opinions about this change.)
> >>>>
> >>>> Can I get a couple of reviews for this small enhancement to the webrev
> >>>> tool?
> >>>>
> >>>> CODETOOLS-7901044: Add next/prev links to navigate between diffs
> >>>> https://bugs.openjdk.java.net/browse/CODETOOLS-7901044
> >>>>
> >>>> Webrev:
> >>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/
> >>>>
> >>>>
> >>>> Background:
> >>>> When browsing review requests that span many files it is quite time
> >>>> consuming to have to go back to the index page to view to the next or
> >>>> previous file. This patch adds links to the next and previous files in
> >>>> the diff views (including frames).
> >>>>
> >>>> The above webrev is generated with this new version, but since it only
> >>>> contains a single file it is not that interesting. Here is an 
> >>>> example of
> >>>> what the new webrev looks like for a larger change (the G1 class
> >>>> unloading change):
> >>>>
> >>>> http://cr.openjdk.java.net/~brutisso/CODETOOLS-7901044/webrev.00/example/ 
> >>>>
> >>>>
> >>>>
> >>>> Thanks,
> >>>> Bengt
> >>>>
> >>>
> >>
> >
>