Vojtech Szocs has posted comments on this change. Change subject: userportal, webadmin: introduce AssetProvider ......................................................................
Patch Set 7: (4 comments) Reviewed only core changes, but looks good in general. This refactor allows us to simplify code (less @Inject constructor parameters etc.) across entire class hierarchies, which is good. If static AssetProvider method calls will become an issue (e.g. need to mock it for testing), we can always extract the static call into method which can be stubbed if needed (e.g. Mockito.spy). Also, this means we're one step closer to getting rid of ClientGinjectorProvider. https://gerrit.ovirt.org/#/c/38724/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationTemplates.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationTemplates.java: Line 4: import com.google.gwt.safehtml.shared.SafeHtml; Line 5: Line 6: public interface CommonApplicationTemplates extends SafeHtmlTemplates { Line 7: Line 8: // TODO this doesn't belong here Indeed. Also, "public static final" are all redundant keywords here - in Java interfaces, any non-method members automatically become "public static final" fields. Line 9: public final static int TAB_BAR_HEIGHT = 24; Line 10: Line 11: @Template("<span><span style='position: relative; display: inline-block; vertical-align: top; height: 14px; line-height: 14px;'>{0}</span>" Line 12: + "<span style='position: relative; margin-left: 3px; margin-right: 3px; white-space: nowrap; height: 14px; line-height: 14px;'>{1}</span></span>") https://gerrit.ovirt.org/#/c/38724/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/AssetProvider.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/AssetProvider.java: Line 10: Line 11: /** Line 12: * Provides static access to resources, constants, templates, and messages. Line 13: */ Line 14: public class AssetProvider { Just a note - @Inject'ing Provider<X> instead of X directly is useful mostly when type X is bound as non-singleton. Constants, messages, templates and resources are all bound as singletons, so you could also @Inject them directly. But using Provider does no harm so I'm OK with current code too. Line 15: Line 16: @Inject Line 17: static Provider<CommonApplicationConstants> commonApplicationConstantsProvider; Line 18: https://gerrit.ovirt.org/#/c/38724/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/BaseSystemModule.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/BaseSystemModule.java: Line 79: Line 80: private void bindFrontendInfrastructure() { Line 81: bind(Frontend.class).in(Singleton.class); Line 82: requestStaticInjection(Frontend.InstanceHolder.class); Line 83: requestStaticInjection(AssetProvider.class); Hm, this method was meant to contain GIN bindings for "frontend communication infra" classes, so I'd rather put AssetProvider binding elsewhere, e.g. at the end of bindCommonInfrastructure method? Line 84: bind(VdcOperationManager.class).in(Singleton.class); Line 85: bind(OperationProcessor.class).in(Singleton.class); Line 86: bind(CommunicationProvider.class).to(GWTRPCCommunicationProvider.class).in(Singleton.class); Line 87: } https://gerrit.ovirt.org/#/c/38724/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/CommonGinUiBinderWidgets.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/gin/CommonGinUiBinderWidgets.java: Line 12: * <w:MyWidget /> Line 13: * Line 14: * public class MyWidget { Line 15: * @Inject Line 16: * public MyWidget(MyOtherDependency dep) { I really like the attention to detail, nice job keeping the Javadoc in sync. Line 17: * // ... Line 18: * } Line 19: * } Line 20: * </code> -- To view, visit https://gerrit.ovirt.org/38724 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifafe5a16068452dac46208c6f06b05c113403443 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@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
