Created attachment 111308 [details]
usage patch 

Usage in web.project, web.jsf modules.

DK01 - adding WebModuleExtender.save() method with empty body into abstract public API class is not 100% backward compatible because there can exist a client which already extended WebModuleExtender and defined a method with signature which may clash with the one you are adding, eg. with different return type; or a private one. There are few ways to make this change backward compatible, for example you could add

    public abstract class SavableWebModuleExtender extends WebModuleExtender {
        abstract void save();
    }

and in the JSF module check implementation class and based on that cast it to either WebModuleExtender or SavableWebModuleExtender. Or you could add inner class to WebModuleExtender:

    public interface Savable {
        void save();
    }

Using interface is better I think as it gives you more flexibility in future when you need to add other methods to WebModuleExtender. With abstract classes you may in long term end up with eg. SavablePrintableUpdatableXxxableWebModuleProvider.

Created attachment 111344 [details]
apichange patch v2

DK01: fixed - I preferred interface as well, thanks

Created attachment 111346 [details]
usage patch v2

Usage patch with few improvements.

DK01 - it is binary compatible. It might not be source compatible, but an implementor affected by that could simply rename their preexisting method. Demo: create

---%<--- AbstractClassTest.java
public class AbstractClassTest {
    public static void main(String[] args) {
        A a = new I();
        a.run();
    }
}
---%<--- A.java
public abstract class A {
    public abstract void run();
}
---%<--- I.java
public class I extends A {
    @Override public void run() {
        System.err.println(save());
    }
    private String save() {
        return "I.save";
    }
}
---%<---

and run. Now add to AbstractClassTest.java:

        a.save();

and to A.java:

    public void save() {
        System.err.println("A.save");
    }

and run again (without recompiling I.class). You get

I.save
A.save

as expected.

Anyway, if you do want to introduce a new method and do not want to add it to the existing SPI class, adding a nested interface suffers from the potential problem that it is not well discoverable. In this case it may not matter - e.g. if most impls will not in fact be savable, and you are adding the method only for one or two special cases - but in general it does, in which case you would just create WebModuleProvider2. While this could _extend_ WebModuleProvider, that still does not make it clear that you ought to be extending the new interface; so it would be recommended to make a fresh interface (not abstract class) and simply deprecate the old one. Then it is clear to all existing implementors that they are using an older version of the API and should migrate; and the lookup code need not do instanceof checks, but merely find all WMP2's and then secondarily all WMP's.

Savable interface is reasonable comprise I think. Doing it properly and introducing WebModuleProvider2 interface would also require changing WebFrameworkProvider (another abstract SPI class producing WebModuleProvider*) which is too much work not worth it.

Although Jesse's solution is smoother, my best opinion is to choose nested interface too. Its usage will be probably quite rare because nobody missed the save() method for about 7 years of existence of this API. So since it would require a lot of rewrite in that API, the discoverability has not the biggest priority in this case, I mean.

I'm going to integrate patch v2 tomorrow if no additional comments appear.

Jesse, David, thanks for your review.

Integrated in web-main #549cabbebb4b.

Integrated into 'main-golden'
Changeset: http://hg.netbeans.org/main-golden/rev/549cabbebb4b
User: Martin Fousek <marfous@netbeans.org>
Log: #202818 - API review: Introduced WebModuleExtender.save() method