RFR: 7198: add websocket server that pushes data on selection updates [v6]

[Alex Ciminian]

On Tue, 14 Sep 2021 11:40:28 GMT, Brice Dutheil <github.com+803621+bric3 at openjdk.org> wrote:

>> Alex Ciminian has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Add tree and graph handlers to websocket server
>>  - Revert whitespace changes in core/features.xml
>
> application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrEditor.java line 134:
> 
>> 132: 		websocketServerEnabledListener = e -> {
>> 133: 			if (e.getProperty().equals(PreferenceKeys.PROPERTY_ENABLE_WEBSOCKET_SERVER)) {
>> 134: 				if ((Boolean) e.getNewValue()) {
> 
> For readability purpose maybe this code could benefit from an explicit equality check

This is refactored, we don't use a boolean setting to see if we should enable the server.

> application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/websocket/WebsocketServer.java line 23:
> 
>> 21: 
>> 22: public class WebsocketServer {
>> 23: 	private static int PORT = 8029;
> 
> I think the port should be configurable.

This is done now - instead of a checkbox to enable the server, users need to pick a port number with `0` being the default value. When `0` is selected the webserver is disabled.

We had a discussion about this and @jpbempel pointed out that 0 is a valid port number that means the operating system should pick an open port. Since we don't display the port number in the UI anywhere, binding to a random port will be confusing for the user because they won't know what to connect to ��  So I think it's ok to keep `0` as the disabled value, what do you think?

> application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/websocket/WebsocketServer.java line 37:
> 
>> 35: 		Server server = new Server();
>> 36: 		ServerConnector connector = new ServerConnector(server);
>> 37: 		connector.setPort(PORT);
> 
> I'm not quite sure how this feature will be used, but can it make sense to only bind to locahost ?

�� Yup, definitely! This was in the plan �� it's done now.

-------------

PR: https://git.openjdk.java.net/jmc/pull/306