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

[Jonathan Gibbons]

Bengt,

There may be problems with the latest version of webrev.

Eric McCorkle is reporting syntax errors from his version of ksh.

  patch cdiffs udiffs sdiffs frames old new
src/jdk.javadoc/share/classes/com/sun/tools/javadoc/SeeTagImpl.java
                 ../../tl/make/scripts/webrev.ksh[2241]: 
build_old_new[1714]: build_old_new_mercurial[1666]: [: : arithmetic 
syntax error

-- Jon

On 10/03/2014 12:58 AM, Bengt Rutisson wrote:
>
> On 2014-10-02 23:39, John Coomes wrote:
>> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>>> On 2014-10-02 01:08, John Coomes wrote:
>>>> 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.
>>> Thanks for looking at this, John! And thanks for sponsoring the change.
>>> I'll send you the export in a separate email.
>> Got it; it's been pushed.
>
> Thanks for pushing this, John!
>
> Bengt
>
>>
>> -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
>>>>>>>>>
>