Vojtech Szocs has posted comments on this change. Change subject: userportal, webadmin: workaround for Firefox favicon bug ......................................................................
Patch Set 5: Code-Review+2 (1 comment) Nice patch! Just one small nit-pick regarding final field, otherwise looks great. .................................................... File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/place/ApplicationPlaceManager.java Line 23: private static final Logger logger = Logger.getLogger(ApplicationPlaceManager.class.getName()); Line 24: Line 25: private final CurrentUser user; Line 26: private final PlaceRequest defaultLoginSectionRequest; Line 27: protected ClientAgentType clientAgentType; I'd suggest to make this field final, just like any other dependency obtained via constructor injection. Some background on this: This is because a dependency such as ClientAgentType is essentially a class invariant [1], i.e. "clientAgentType != null" is (typically) always expected and making the field final reduces the chance to break this invariant (especially in cases where the field is protected and some subclass could re-set its value). [1] http://en.wikipedia.org/wiki/Class_invariant Line 28: Line 29: private PlaceRequest autoLoginRequest; Line 30: Line 31: public ApplicationPlaceManager(EventBus eventBus, TokenFormatter tokenFormatter, -- To view, visit http://gerrit.ovirt.org/20798 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4b0d8a0176eb9299aec01ae6772297409ef3108 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> 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
