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

[Daniel Fuchs]

Hi Bengt,

The new version looks good to me, but I am not a reviewer
fore CODE_TOOLS. Hopefully someone who is will step in :-)

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