Vojtech Szocs has posted comments on this change. Change subject: webadmin,userportal: Persistent client-side logging infrastructure ......................................................................
Patch Set 1: (2 comments) http://gerrit.ovirt.org/#/c/25444/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/logging/LocalStorageLogHandler.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/logging/LocalStorageLogHandler.java: Line 21: private static final String LOG_HEAD_KEY = "LogHead"; //$NON-NLS-1$ Line 22: private static final String LOG_SIZE_KEY = "LogSize"; //$NON-NLS-1$ Line 23: Line 24: // Maximum number of log records to keep in underlying storage Line 25: private static final int LOG_CAPACITY = 100; > Maybe do a check for IE8 and below as they don't support local storage and For really old browsers (like IE8) that don't support HTML5 local storage, our ClientStorage abstraction falls back to using cookies. http://gerrit.ovirt.org/#/c/25444/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/system/ClientStorage.java Since web browsers usually limit the max. number of cookies that can be set for given domain, as well as max. length of each cookie, the cookie fallback isn't really a viable alternative, especially when persisting diverse data (many storage keys). This is why I wrote in above change: TODO(vszocs) we should reconsider the use of cookie fall back due to above limitations I'd like to drop cookie-based client persistence fallback altogether. There is no viable alternative to HTML5 local storage. In other words, really old browsers simply won't have local preferences persisted at all. (I think that cookies were not designed as local preference persistence mechanism, anyway.) Line 26: Line 27: private final ClientStorage clientStorage; Line 28: private final AddOnlyRingBuffer<String> logBuffer; Line 29: http://gerrit.ovirt.org/#/c/25444/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/AddOnlyRingBuffer.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/AddOnlyRingBuffer.java: Line 64: // Buffer is full, need to move the head Line 65: else { Line 66: int index = head; Line 67: head = (head + 1) % capacity; Line 68: old = delegate.read(index); > The only strange thing here is that the return value is not what is added, > The only strange thing here is that the return value is not what is added, > but what is removed. Correct. This is due to the nature of implemented data structure: add-only ring buffer with limited capacity will eventually get full. Once full, adding new element causes the "oldest" element to be removed. The "oldest element being removed" is essentially the side effect of adding new element. I think it's common for abstract data structures _not_ to return the added element itself, but information about the _effect_ of adding given element instead. Some examples: * java.util.Collection#add -> returns true if the collection was modified * java.util.Map#put -> returns previous value associated with given key Line 69: delegate.write(index, element); Line 70: } Line 71: Line 72: return old; -- To view, visit http://gerrit.ovirt.org/25444 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3b0be449ab425b56a1d7c39efeb1793991e58fa7 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
