Alexander Wels has posted comments on this change.
Change subject: userportal, webadmin: branding support[WIP].
......................................................................
Patch Set 6: (3 inline comments)
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingTheme.java
Line 64:
Line 65: /**
Line 66: * The key of the web admin css in the branding properties.
Line 67: */
Line 68: private static final String WEB_ADMIN_CSS_KEY = "web_admin_css";
//$NON-NLS-1$
Yes, good point, I will make that change.
Line 69:
Line 70: /**
Line 71: * The key for the resource bundle name.
Line 72: */
Line 108: path = brandingPath.substring(
Line 109: brandingRootPath.getAbsolutePath().
Line 110: length());
Line 111: filePath = brandingPath;
Line 112: available = true;
I disagree with some of the notes here:
You always assume failure, and set the result to success at the end. I assume
success and only change the status in case of failure. That is an identical
pattern just reversed.
I don't like using exceptions for flow control which is more or less what your
third suggestion is doing.
I do however like your second suggestion. Using load to determine success or
not, it eliminates one member variable, as well as making the class more
serializable in case we ever want to do that.
Line 113: FileInputStream propertiesFile;
Line 114: try {
Line 115: propertiesFile = new FileInputStream(brandingPath
Line 116: + "/" + BRANDING_PROPERTIES_NAME); //$NON-NLS-1$
Line 219: builder.append(getPath());
Line 220: builder.append(", User portal css: "); //$NON-NLS-1$
Line 221: builder.append(getUserPortalStyleSheetName());
Line 222: builder.append(", Web admin css: "); //$NON-NLS-1$
Line 223: builder.append(getWebadminStyleSheetName());
Also a lot slower than using a builder. I did some research and the formatter
is an order of magnitude slower than the builder. On the other hand this should
not be called very often so performance is not a big consideration.
I also learned that since java 1.5 simple concatenation is the same as using a
string builder as the compiler optimizes this.
Line 224: return builder.toString();
Line 225: }
--
To view, visit http://gerrit.ovirt.org/13181
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a8a426ce7d688d33c5ae2b70632c836843106b2
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alex Lourie <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Eyal Edri <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Moran Goldboim <[email protected]>
Gerrit-Reviewer: Sahina Bose <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
