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

[Bengt Rutisson]

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