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

[Bengt Rutisson]

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.

Thanks,
Bengt



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