Reduce DOM nodes when updating reviewed status
There appears to be a memory leak associated with replacing an
existing Image widget in the hidden file list table. I think GWT is
not completely discarding the old checkmark Image when its replaced
by a new checkmark Image if the user re-reviews a file within the
patch set.
Minimize the leak by only making DOM changes if there really is a
modification in reviewed status.
Improve lookup performance on large patch sets by indexing all
Patch objects using a HashMap. This makes lookup for marking the
reviewed flag or updating draft counts to be O(1) rather than O(N),
which can help on larger patch sets.
Change-Id: I5530422bfb6b23cd5a990ac24f92e9f125dc7a1e
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java
index d00ef89..d47e2c1 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchTable.java
@@ -48,7 +48,9 @@
import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
public class PatchTable extends Composite {
public interface PatchValidator {
@@ -80,6 +82,7 @@
private String savePointerId;
private PatchSet.Id base;
private List<Patch> patchList;
+ private Map<Patch.Key, Integer> patchMap;
private ListenableAccountDiffPreference listenablePrefs;
private List<ClickHandler> clickHandlers;
@@ -97,18 +100,25 @@
}
public int indexOf(Patch.Key patch) {
- for (int i = 0; i < patchList.size(); i++) {
- if (patchList.get(i).getKey().equals(patch)) {
- return i;
+ Integer i = patchMap().get(patch);
+ return i != null ? i : -1;
+ }
+
+ private Map<Key, Integer> patchMap() {
+ if (patchMap == null) {
+ patchMap = new HashMap<Patch.Key, Integer>();
+ for (int i = 0; i < patchList.size(); i++) {
+ patchMap.put(patchList.get(i).getKey(), i);
}
}
- return -1;
+ return patchMap;
}
public void display(PatchSet.Id base, PatchSetDetail detail) {
this.base = base;
this.detail = detail;
this.patchList = detail.getPatches();
+ this.patchMap = null;
myTable = null;
final DisplayCommand cmd = new DisplayCommand(patchList, base);
@@ -326,36 +336,33 @@
}
void updateReviewedStatus(final Patch.Key patchKey, boolean reviewed) {
- final int row = findRow(patchKey);
- if (0 <= row) {
- final Patch patch = getRowItem(row);
- if (patch != null) {
- patch.setReviewedByCurrentUser(reviewed);
-
+ int idx = patchMap().get(patchKey);
+ if (0 <= idx) {
+ Patch patch = patchList.get(idx);
+ if (patch.isReviewedByCurrentUser() != reviewed) {
+ int row = idx + 1;
int col = C_SIDEBYSIDE + 2;
if (patch.getPatchType() == Patch.PatchType.BINARY) {
col = C_SIDEBYSIDE + 3;
}
-
if (reviewed) {
table.setWidget(row, col, new Image(Gerrit.RESOURCES.greenCheck()));
} else {
table.clearCell(row, col);
}
+ patch.setReviewedByCurrentUser(reviewed);
}
}
}
void notifyDraftDelta(final Patch.Key key, final int delta) {
- final int row = findRow(key);
- if (0 <= row) {
- final Patch p = getRowItem(row);
- if (p != null) {
- p.setDraftCount(p.getDraftCount() + delta);
- final SafeHtmlBuilder m = new SafeHtmlBuilder();
- appendCommentCount(m, p);
- SafeHtml.set(table, row, C_DRAFT, m);
- }
+ int idx = patchMap().get(key);
+ if (0 <= idx) {
+ Patch p = patchList.get(idx);
+ p.setDraftCount(p.getDraftCount() + delta);
+ SafeHtmlBuilder m = new SafeHtmlBuilder();
+ appendCommentCount(m, p);
+ SafeHtml.set(table, idx + 1, C_DRAFT, m);
}
}
