Vojtech Szocs has posted comments on this change. Change subject: frontend: Pass JSESSIONID in VV file ......................................................................
Patch Set 2: (14 comments) http://gerrit.ovirt.org/#/c/33770/2//COMMIT_MSG Commit Message: Line 8: Line 9: Moved the RestApiSessionManager to gwt-common project so it would be Line 10: accessible from both webadmin and userportal. Line 11: Line 12: Due to the fact that uicommon does not see the generated gwt-p events If you need some GwtEvent in UiCommon project, you'll need to add it manually, for example see GridTimerStateChangeEvent. This is because UiCommon project's pom.xml doesn't have GWTP annotation processors (gwtp-processors) set up. These are set up only for gwt-common, userportal-gwtp and webadmin projects. Line 13: and handlers we need to attach the listener for RestApiSessionAcquiredEvent Line 14: in UiCommonDefaultTypeResolver. To prevent excessive number of event Line 15: listeners attached we cache the created SpiceNativeImpl and Line 16: VncNativeImpl. Line 12: Due to the fact that uicommon does not see the generated gwt-p events Line 13: and handlers we need to attach the listener for RestApiSessionAcquiredEvent Line 14: in UiCommonDefaultTypeResolver. To prevent excessive number of event Line 15: listeners attached we cache the created SpiceNativeImpl and Line 16: VncNativeImpl. Nice. Any reason not to cache other type instances within UiCommonDefaultTypeResolver as well? Line 17: Line 18: The JSESSIONID is output in the VV file in the format: Line 19: jsessionid=123456789 Line 20: http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/restapi/EngineSessionTimeoutData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/restapi/EngineSessionTimeoutData.java: Line 4: Line 5: /** Line 6: * Overlay type for {@code engineSessionTimeout} global JS object. Line 7: */ Line 8: public final class EngineSessionTimeoutData extends JavaScriptObject { By moving this to gwt-common, you should ensure that GWT host page for of both WebAdmin and UserPortal contains "engineSessionTimeout" global JS object. To do so, move WebAdminHostPageServlet.ATTR_ENGINE_SESSION_TIMEOUT to GwtDynamicHostPageServlet along with related code. When verifying this patch, ensure that HTML source of both WebAdmin and UserPortal contains "engineSessionTimeout" global JS object. Line 9: Line 10: protected EngineSessionTimeoutData() { Line 11: } Line 12: http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/BaseApplicationInit.java: Line 37: * Login model type. Line 38: */ Line 39: public abstract class BaseApplicationInit<T extends LoginModel> implements Bootstrapper, LogoutHandler { Line 40: Line 41: protected final RestApiSessionManager restApiSessionManager; Any reason why is this protected and not private? Line 42: private final ITypeResolver typeResolver; Line 43: private final FrontendEventsHandlerImpl frontendEventsHandler; Line 44: protected final FrontendFailureEventListener frontendFailureEventListener; Line 45: Line 53: Line 54: private final LockInteractionManager lockInteractionManager; Line 55: Line 56: public BaseApplicationInit(ITypeResolver typeResolver, Line 57: FrontendEventsHandlerImpl frontendEventsHandler, I'd suggest to retain original indentation, this doesn't look good to me. Line 58: FrontendFailureEventListener frontendFailureEventListener, Line 59: CurrentUser user, EventBus eventBus, Line 60: Provider<T> loginModelProvider, Line 61: LockInteractionManager lockInteractionManager, http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/UiCommonDefaultTypeResolver.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/UiCommonDefaultTypeResolver.java: Line 50: } Line 51: Line 52: @SuppressWarnings("rawtypes") Line 53: @Override Line 54: public Object resolve(Class type) { See my comments below, in general I think that resolve() should not create new instance on each invocation, we could fix this by using lazy singletons managed by GIN. Line 55: if (type == Configurator.class) { Line 56: return configurator; Line 57: } else if (type == ILogger.class) { Line 58: return logger; Line 84: Line 85: throw new RuntimeException("UiCommon Resolver cannot resolve type: " + type); //$NON-NLS-1$ Line 86: } Line 87: Line 88: private ISpiceNative getSpiceNative() { We could bind ISpiceNative to SpiceNativeImpl as GIN (lazy) singleton, inject Provider<ISpiceNative> here and call provider.get() to access the singleton. What do you think? Line 89: if (spiceNative != null) { Line 90: return spiceNative; Line 91: } Line 92: Line 95: Line 96: return spiceNative; Line 97: } Line 98: Line 99: private IVncNative getVncNative() { Same question as above. Line 100: if (vncNative != null) { Line 101: return vncNative; Line 102: } Line 103: Line 107: return vncNative; Line 108: } Line 109: Line 110: private void attachRestApiSessionIdHandler(final RestApiSessionIdAware pluginImpl) { Line 111: eventBus.addHandler(RestApiSessionAcquiredEvent.getType(), new RestApiSessionAcquiredEvent.RestApiSessionAcquiredHandler() { > Is this guaranteed to happen soon enough? Because if this happens after the Tomas raised a good point. If ISpiceNative / IVncNative instances would be declared as eager GIN singletons, we could guarantee that this would always happen soon enough. Line 112: @Override Line 113: public void onRestApiSessionAcquired(RestApiSessionAcquiredEvent event) { Line 114: pluginImpl.setRestApiSessionId(event.getSessionId()); Line 115: } http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ISpiceNative.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ISpiceNative.java: Line 2: Line 3: import org.ovirt.engine.ui.uicommonweb.restapi.RestApiSessionIdAware; Line 4: Line 5: /** Line 6: * Marking interface Could you please improve this Javadoc as well? Something like "Marking interface for native SPICE implementations", or similar. Line 7: * Line 8: */ Line 9: public interface ISpiceNative extends ISpice, RestApiSessionIdAware { Line 10: http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/IVncNative.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/IVncNative.java: Line 1: package org.ovirt.engine.ui.uicommonweb.models.vms; Line 2: Line 3: import org.ovirt.engine.ui.uicommonweb.restapi.RestApiSessionIdAware; Line 4: Line 5: public interface IVncNative extends IVnc, RestApiSessionIdAware { Please document this interface, in consistence with ISpiceNative. Line 6: http://gerrit.ovirt.org/#/c/33770/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/SpiceConsoleModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/SpiceConsoleModel.java: Line 55: public class SpiceConsoleModel extends ConsoleModel implements IFrontendMultipleQueryAsyncCallback { Line 56: Line 57: Line 58: Line 59: public enum ClientConsoleMode { Native, Plugin, Auto, Html5;} In my opinion, the trailing " ; " here is just redundant. It makes sense when enum has other stuff (beyond its members), for example: public enum Foo { // members A, B; // other stuff private String x; Foo(String x) { this.x = x; } public String getX() { return x } } But when there's no "other stuff", trailing " ; " is just redundant. Also, why double newline above? Line 60: public static EventDefinition spiceDisconnectedEventDefinition; Line 61: Line 62: public static EventDefinition spiceConnectedEventDefinition; Line 63: public static EventDefinition spiceMenuItemSelectedEventDefinition; Line 67: private SpiceMenu menu; Line 68: Line 69: private ISpice privatespice; Line 70: private ClientConsoleMode consoleMode; Line 71: private String restApiSessionId; > I guess you can delete this +1 Line 72: Line 73: private void setspice(ISpice value) { Line 74: privatespice = value; Line 75: } Line 408: .getConstants() Line 409: .usbDevicesNoUsbdevicesClientSpiceUsbRedirectorNotInstalled() }); Line 410: Line 411: if (restApiSessionId != null) { Line 412: > I guess you can delete this +1 Line 413: } Line 414: Line 415: // Create menu. Line 416: int id = 1; -- To view, visit http://gerrit.ovirt.org/33770 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id5c6d2c91b99240527760b57a6079b5f986aff5b Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
