Hi Bertrand, On Mon, Dec 8, 2014 at 12:37 PM, Bertrand Delacretaz <bdelacre...@apache.org> wrote: > Hi Robert, > > On Mon, Dec 8, 2014 at 10:45 AM, Robert Munteanu <romb...@apache.org> wrote: >> ...I'd >> like to commit this today or tomorrow so any feedback is welcome.... > > Looks good to me except the SLING-4217 patch is missing the > Adaption*.java classes.
I continued the latest patches at [1], makes it easier to see what changes, including between iterations of the same patch. This includes the missing classes. > > Also, about the "TODO - is it possible to remove more than one factory > per service reference" comment - I'm not sure what the impact of this > is, is it just about being able to do something in an easier way, or > is there a risk of leaving unwanted things registered? There's a risk of leaving an unwanted 'Adaption' service registered, which would incorrectly signal that the AdapterFactory is still registered. Nonetheless, the AdapterManager would still function correctly, the Adaption service is just used for signalling consumers of adaptTo when a certain adaption is available. Note that in the latest iteration I simply log an ERROR in such a scenario to simplify the code ( see [2] ). [1]: https://reviews.apache.org/r/28751/diff/# [2]: https://reviews.apache.org/r/28751/diff/3/#chunk3.22 -- Sent from my (old) computer
