Bug 158624

Comments from Petr Hejl:

PH01: There seems to be methods with duplicated functionality in NativeProcess. What is the semantical difference
between pairs like cancel() - destroy() and waitFor() - waitResult()?
PH02: NativeProcess.Listener is not used anywhere in NativeProcess so I think it should be defined in
NativeProcessBuilder where it is actually used.
PH03: The old and new state has to be delivered to NativeProcess.Listener. This can complicate things terribly
especially in multithreaded environments. If you don't have strong justification to do so, please remove states from the
interface (implementation can get new state via direct call to NativeProcess). In such case you could even use the
ChangeListener defined in JDK and delete your own interface completely - PH02 is not valid in such case ;) .
PH04: I think ExecutionEnvironment should be merged to process builder (perhaps a method setHost(user, host, port) in
builder would do the same as the whole ExecutionEnvironment from the API point of view).
PH05: I'm not sure about the util package. Are all the classes API? What is the suggested use case?
PH06: HostNotConnectedException - do not inherit from Throwable, use Exception or better IOException. Anyway I think
java.net.ConnectException or java.net.UnknowHostException can do the same job for you.
PH07: Implementation note - I would use package org.netbeans.modules.nativeexecution instead of
org.netbeans.modules.nativeexecution.api.impl and package org.netbeans.modules.nativeexecution.api.util instead of
org.netbeans.modules.nativeexecution.util. 

> PH01: There seems to be methods with duplicated functionality in NativeProcess. What is the semantical difference 
> between pairs like cancel() - destroy() and waitFor() - waitResult()?

This has been redesigned. So no valid anymore.

> PH02: NativeProcess.Listener is not used anywhere in NativeProcess so I think it should be defined in 
> NativeProcessBuilder where it is actually used.

This has been redesigned. So no valid anymore.

> PH03: The old and new state has to be delivered to NativeProcess.Listener. This can complicate things terribly 
> especially in multithreaded environments. If you don't have strong justification to do so, please remove states from 
> the interface (implementation can get new state via direct call to NativeProcess). In such case you could even use 
> the ChangeListener defined in JDK and delete your own interface completely - PH02 is not valid in such case ;) .

Now ChangeListener is used.

> PH04: I think ExecutionEnvironment should be merged to process builder (perhaps a method setHost(user, host, port) in 
> builder would do the same as the whole ExecutionEnvironment from the API point of view).

It is still a separate class as it is a convenient way to pass information about *where* to start a process between parties.

> PH05: I'm not sure about the util package. Are all the classes API? What is the suggested use case?

There are some usecases described in documentation.

> PH06: HostNotConnectedException - do not inherit from Throwable, use Exception or better IOException. Anyway I think 
> java.net.ConnectException or java.net.UnknowHostException can do the same job for you.

ConnectException is used now.

> PH07: Implementation note - I would use package org.netbeans.modules.nativeexecution instead of 
> org.netbeans.modules.nativeexecution.api.impl and package org.netbeans.modules.nativeexecution.api.util instead of 
> org.netbeans.modules.nativeexecution.util. 

Packages renamed.

In case we all agree on friend module, API_REVIEW_FAST can be done... So, adding this keyword.
If not, we may go through the full review. In this case I need a list of reviewers.

Internal SWAN links for an API review of an open source project?

Weird...

Created attachment 77037 [details]
Architecture Questionary Answers

Created attachment 77038 [details]
Java Doc

Owwww.
And I was just about to start the API review for my RichExecution.
I'm leaving for skiing tomorrow til the end of the week so I'll just
quickly attach the latest javadoc for it so you can get a flavor for it.

Created attachment 77056 [details]
RicExecution module javadoc

Will anyone complete the review?