Thanks Kurchi, looks fine. -Chris
Kurchi Hazra <kurchi.subhra.ha...@oracle.com> wrote: >On 7/11/12 4:24 PM, Chris Hegarty wrote: >> On 12 Jul 2012, at 00:15, Kurchi Hazra<kurchi.subhra.ha...@oracle.com> >> wrote: >> >>> Thanks for the review Alan. Updated webrev: >>> http://cr.openjdk.java.net/~khazra/7160252/webrev.01/ >> Looks fine. >> >> Trivially, is there an opportunity to make any fields final since initFields >> is replaced with a constructor? > >Thanks for pointing that out. How about: >http://cr.openjdk.java.net/~khazra/7160252/webrev.02/ > >- Kurchi > >> >> -Chris. >> >>> - Kurchi >>> >>> On 7/11/12 5:45 AM, Alan Bateman wrote: >>>> On 26/06/2012 22:57, Kurchi Hazra wrote: >>>>> Hi, >>>>> >>>>> On Mac OS X, for Preferences, a new child added event was not being >>>>> delivered to a NodeChangeListener since the existing code depended on the >>>>> return value of addNode() in the native code, which returns true if a new >>>>> node is added. However, since addNode() was being called erroneously after >>>>> a child node is already added to an existing node, addNode() would always >>>>> return false, resulting in thw new node event never being delivered. >>>>> >>>>> This fix propagates the required information of whether a node is added >>>>> from >>>>> the method adding the child node itself. In addition, I cleaned up the >>>>> constructors in MacOSXPreferences.java and added a test >>>>> (AddNodeChangeListener.java) to cover this case. >>>>> >>>>> Finally, there were two prefs tests in ProblemList.txt which are now >>>>> passing, >>>>> I have removed these from the ProblemList too. >>>>> >>>>> >>>>> >>>>> Bug: http://bugs.sun.com/view_bug.do?bug_id=7160252 >>>>> Webrev: http://cr.openjdk.java.net/~khazra/7160252/webrev.00/ >>>>> >>>>> Thanks, >>>>> Kurchi >>>> It doesn't look you got a reviewer for this one. >>>> >>>> I looked at the changes and the fix seems okay to me. The only odd thing >>>> is that now that you have the 5-arg constructor then it can initialize all >>>> the fields allowing you to get rid of initFields and also setNew. Also I >>>> assume it means you can close 7150557. >>>> >>>> -Alan. >>>> >
