SideBySide2: Chunk and comment padding cleanup

This is a large rewrite of the way padding is created to occupy
vertical space on the other side CM when a comment is inserted
or a region of lines are inserted, deleted, or unevenly replaced.

Since monospace fonts are used with no line wrapping, the height of
any given line is predictable.  A diff chunk can be padded on the
opposite side simply by creating a <div style="height: Nem"/> element,
where N is the number of additional lines on the non-padded side.

To ensure underscores are visible an extra 1px padding-bottom is added
to every line.  This used to be expressed as a fractional 0.11em, but
the fraction gets confused with any browser zoom level other than
100%.  The padding on the opposite side includes an additional Npx of
padding-bottom to ensure it consumes the correct vertical height.

Scrolling works better with fixed height line padding.  CM3 is able to
compute the height of each line widget before display, and the heights
do not change.  The fixed height fixes a number of alignment glitches
that appeared during scrolling.

All comment widgets (PublishedBox and DraftBox) on a line of code are
now grouped together into a single uber-widget for CM3.  Using a
single group widget simplifies the padding compution for the other
side.  It also allows the border and drop shadow to be applied to the
entire group instead of per-comment, reducing the vertical space
required for 2 comments (e.g.  reviewer note, author reply).

The group widget can also now be responsible for group operations like
ordering a DraftBox immediately after the PublishedBox it is replying
to, and opening/closing boxes within the group.

CommentManager now tracks all published and draft comments using the
Gerrit server format, which is 1-based.  The group widgets are held in
a pair of TreeMaps indexed by 1-based line number.  This cleans up the
code and prepares it support searching for the next/previous comment
from the current cursor position.

Updating padding while growing the text area in a DraftBox has been
improved by running a timer in the background after the height has
been identified as changed.  The timer reduces the time a user can
observe the sides unaligned if the mouse is dragged outside of the
bounds of the DraftBox and continues to grow the text area.

Horizontal scrolling of comments wider than the CM3 instance is now
done within the CommentBox instead of the line widget wrapper supplied
by CM3.  This allows the text to always stay on the yellow comment
background, and keeps visible the author image, name, and date during
horizontal scrolling.  It also simplifies the height computation by
always including the scrollbar height as part of getOffsetHeight()
without having to dig up the DOM to identify a parent element.

File level comments are now displayed in a group above line 1.
Placing them here allows the comment widgets to be smoothly scrolled
as part of the CM3, instead of suddenly disappearing when headers are
hidden as the viewport moves further down the file.  A consequence of
this placement is line 1 must be unhidden if it was skipped as common
on both sides.  Placing file comments within the CM3 view simplifies a
lot of code, removing several special case conditions for editors
placed outside of a CM3.  It also allows file comments to appear in
the side overview gutter, at the top of the page.

CommentInput was removed as it is redundant with CommentInfo.  This
could be its own change, but was folded into this commit as many of
the regions that use CommentInput/CommentInfo were written anyway by
the file comment changes.

Change-Id: I8df8815a997adea8321085bbf9a69888176188cf

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/History.java

@@ -104,7 +104,7 @@ class History extends FlowPanel {
 
     for (String path : map.keySet()) {
       for (CommentInfo c : Natives.asList(map.get(path))) {
-        c.setPath(path);
+        c.path(path);
         if (c.author() != null) {
           AuthorRevision k = new AuthorRevision(c.author(), id);
           List<CommentInfo> l = byAuthor.get(k);

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ReplyBox.java

@@ -428,7 +428,7 @@ class ReplyBox extends Composite {
 
   private static List<CommentInfo> copyPath(String path, JsArray<CommentInfo> l) {
     for (int i = 0; i < l.length(); i++) {
-      l.get(i).setPath(path);
+      l.get(i).path(path);
     }
     return Natives.asList(l);
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentApi.java

@@ -43,13 +43,13 @@ public class CommentApi {
     revision(id, "drafts").id(draftId).get(cb);
   }
 
-  public static void createDraft(PatchSet.Id id, CommentInput content,
+  public static void createDraft(PatchSet.Id id, CommentInfo content,
       AsyncCallback<CommentInfo> cb) {
     revision(id, "drafts").put(content, cb);
   }
 
   public static void updateDraft(PatchSet.Id id, String draftId,
-      CommentInput content, AsyncCallback<CommentInfo> cb) {
+      CommentInfo content, AsyncCallback<CommentInfo> cb) {
     revision(id, "drafts").id(draftId).put(content, cb);
   }
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInfo.java

@@ -23,42 +23,65 @@ import com.google.gwtjsonrpc.client.impl.ser.JavaSqlTimestamp_JsonSerializer;
 import java.sql.Timestamp;
 
 public class CommentInfo extends JavaScriptObject {
-  public static CommentInfo createRange(String path, Side side, int line,
+  public static CommentInfo create(String path, Side side,
-      String in_reply_to, String message, CommentRange range) {
+      int line, CommentRange range) {
-    CommentInfo info = createFile(path, side, in_reply_to, message);
+    CommentInfo n = createObject().cast();
-    info.setRange(range);
+    n.path(path);
-    info.setLine(range == null ? line : range.end_line());
+    n.side(side);
-    return info;
+    if (range != null) {
+      n.line(range.end_line());
+      n.range(range);
+    } else if (line > 0) {
+      n.line(line);
+    }
+    return n;
   }
 
-  public static CommentInfo createFile(String path, Side side,
+  public static CommentInfo createReply(CommentInfo r) {
-      String in_reply_to, String message) {
+    CommentInfo n = createObject().cast();
-    CommentInfo info = createObject().cast();
+    n.path(r.path());
-    info.setPath(path);
+    n.side(r.side());
-    info.setSide(side);
+    n.in_reply_to(r.id());
-    info.setInReplyTo(in_reply_to);
+    if (r.has_range()) {
-    info.setMessage(message);
+      n.line(r.range().end_line());
-    return info;
+      n.range(r.range());
+    } else if (r.has_line()) {
+      n.line(r.line());
+    }
+    return n;
   }
 
-  private final native void setId(String id) /*-{ this.id = id; }-*/;
+  public static CommentInfo copy(CommentInfo s) {
-  public final native void setPath(String path) /*-{ this.path = path; }-*/;
+    CommentInfo n = createObject().cast();
-
+    n.path(s.path());
-  private final void setSide(Side side) {
+    n.side(s.side());
-    setSideRaw(side.toString());
+    n.id(s.id());
+    n.in_reply_to(s.in_reply_to());
+    n.message(s.message());
+    if (s.has_range()) {
+      n.line(s.range().end_line());
+      n.range(s.range());
+    } else if (s.has_line()) {
+      n.line(s.line());
+    }
+    return n;
   }
-  private final native void setSideRaw(String side) /*-{ this.side = side; }-*/;
 
-  private final native void setLine(int line) /*-{ this.line = line; }-*/;
+  public final native void path(String p) /*-{ this.path = p }-*/;
+  public final native void id(String i) /*-{ this.id = i }-*/;
+  public final native void line(int n) /*-{ this.line = n }-*/;
+  public final native void range(CommentRange r) /*-{ this.range = r }-*/;
+  public final native void in_reply_to(String i) /*-{ this.in_reply_to = i }-*/;
+  public final native void message(String m) /*-{ this.message = m }-*/;
 
-  private final native void setInReplyTo(String in_reply_to) /*-{
+  public final void side(Side side) {
-    this.in_reply_to = in_reply_to;
+    sideRaw(side.toString());
-  }-*/;
+  }
+  private final native void sideRaw(String s) /*-{ this.side = s }-*/;
 
-  private final native void setMessage(String message) /*-{ this.message = message; }-*/;
+  public final native String path() /*-{ return this.path }-*/;
-
+  public final native String id() /*-{ return this.id }-*/;
-  public final native String id() /*-{ return this.id; }-*/;
+  public final native String in_reply_to() /*-{ return this.in_reply_to }-*/;
-  public final native String path() /*-{ return this.path; }-*/;
 
   public final Side side() {
     String s = sideRaw();
@@ -68,10 +91,6 @@ public class CommentInfo extends JavaScriptObject {
   }
   private final native String sideRaw() /*-{ return this.side }-*/;
 
-  public final native int line() /*-{ return this.line || 0; }-*/;
-  public final native String in_reply_to() /*-{ return this.in_reply_to; }-*/;
-  public final native String message() /*-{ return this.message; }-*/;
-
   public final Timestamp updated() {
     Timestamp r = updatedTimestamp();
     if (r == null) {
@@ -83,17 +102,16 @@ public class CommentInfo extends JavaScriptObject {
     }
     return r;
   }
-  private final native String updatedRaw() /*-{ return this.updated; }-*/;
+  private final native String updatedRaw() /*-{ return this.updated }-*/;
   private final native Timestamp updatedTimestamp() /*-{ return this._ts }-*/;
   private final native void updatedTimestamp(Timestamp t) /*-{ this._ts = t }-*/;
 
-  public final native AccountInfo author() /*-{ return this.author; }-*/;
+  public final native AccountInfo author() /*-{ return this.author }-*/;
-
+  public final native int line() /*-{ return this.line || 0 }-*/;
-  public final native boolean has_line() /*-{ return this.hasOwnProperty('line'); }-*/;
+  public final native boolean has_line() /*-{ return this.hasOwnProperty('line') }-*/;
-
+  public final native boolean has_range() /*-{ return this.hasOwnProperty('range') }-*/;
-  public final native CommentRange range() /*-{ return this.range; }-*/;
+  public final native CommentRange range() /*-{ return this.range }-*/;
-
+  public final native String message() /*-{ return this.message }-*/;
-  public final native void setRange(CommentRange range) /*-{ this.range = range; }-*/;
 
   protected CommentInfo() {
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/CommentInput.java

@@ -1,82 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.client.changes;
-
-import com.google.gerrit.client.diff.CommentRange;
-import com.google.gerrit.common.changes.Side;
-import com.google.gwt.core.client.JavaScriptObject;
-import com.google.gwtjsonrpc.client.impl.ser.JavaSqlTimestamp_JsonSerializer;
-
-import java.sql.Timestamp;
-
-public class CommentInput extends JavaScriptObject {
-  public static CommentInput create(CommentInfo original) {
-    CommentInput input = createObject().cast();
-    input.setId(original.id());
-    input.setPath(original.path());
-    input.setSide(original.side());
-    if (original.has_line()) {
-      input.setLine(original.line());
-    }
-    input.setRange(original.range());
-    input.setInReplyTo(original.in_reply_to());
-    input.setMessage(original.message());
-    return input;
-  }
-
-  public final native void setId(String id) /*-{ this.id = id; }-*/;
-  public final native void setPath(String path) /*-{ this.path = path; }-*/;
-
-  public final void setSide(Side side) {
-    setSideRaw(side.toString());
-  }
-  private final native void setSideRaw(String side) /*-{ this.side = side; }-*/;
-
-  public final native void setLine(int line) /*-{ this.line = line; }-*/;
-
-  public final native void setInReplyTo(String in_reply_to) /*-{
-    this.in_reply_to = in_reply_to;
-  }-*/;
-
-  public final native void setMessage(String message) /*-{ this.message = message; }-*/;
-  public final native String id() /*-{ return this.id; }-*/;
-  public final native String path() /*-{ return this.path; }-*/;
-
-  public final Side side() {
-    String s = sideRaw();
-    return s != null
-        ? Side.valueOf(s)
-        : Side.REVISION;
-  }
-  private final native String sideRaw() /*-{ return this.side }-*/;
-
-  public final native int line() /*-{ return this.line; }-*/;
-  public final native String in_reply_to() /*-{ return this.in_reply_to; }-*/;
-  public final native String message() /*-{ return this.message; }-*/;
-
-  public final Timestamp updated() {
-    return JavaSqlTimestamp_JsonSerializer.parseTimestamp(updatedRaw());
-  }
-  private final native String updatedRaw() /*-{ return this.updated; }-*/;
-
-  public final native boolean has_line() /*-{ return this.hasOwnProperty('line'); }-*/;
-
-  public final native CommentRange range() /*-{ return this.range; }-*/;
-
-  public final native void setRange(CommentRange range) /*-{ this.range = range; }-*/;
-
-  protected CommentInput() {
-  }
-}

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/ChunkManager.java

@@ -16,26 +16,24 @@ package com.google.gerrit.client.diff;
 
 import com.google.gerrit.client.diff.DiffInfo.Region;
 import com.google.gerrit.client.diff.DiffInfo.Span;
-import com.google.gerrit.client.diff.PaddingManager.LinePaddingWidgetWrapper;
 import com.google.gerrit.client.rpc.Natives;
 import com.google.gwt.core.client.JsArray;
 import com.google.gwt.core.client.JsArrayString;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.Style.Unit;
+import com.google.gwt.user.client.DOM;
 
 import net.codemirror.lib.CodeMirror;
 import net.codemirror.lib.CodeMirror.LineClassWhere;
-import net.codemirror.lib.CodeMirror.LineHandle;
 import net.codemirror.lib.Configuration;
 import net.codemirror.lib.LineCharacter;
+import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.TextMarker;
 
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
 /** Colors modified regions for {@link SideBySide2}. */
 class ChunkManager {
@@ -48,7 +46,7 @@ class ChunkManager {
   private List<DiffChunkInfo> chunks;
   private List<TextMarker> markers;
   private List<Runnable> undo;
-  private Map<LineHandle, LinePaddingWidgetWrapper> paddingOnOtherSide;
+  private List<LineWidget> padding;
 
   ChunkManager(SideBySide2 host,
       CodeMirror cmA,
@@ -85,8 +83,8 @@ class ChunkManager {
     for (Runnable r : undo) {
       r.run();
     }
-    for (LinePaddingWidgetWrapper x : paddingOnOtherSide.values()) {
+    for (LineWidget w : padding) {
-      x.getWidget().clear();
+      w.clear();
     }
   }
 
@@ -94,7 +92,7 @@ class ChunkManager {
     chunks = new ArrayList<DiffChunkInfo>();
     markers = new ArrayList<TextMarker>();
     undo = new ArrayList<Runnable>();
-    paddingOnOtherSide = new HashMap<LineHandle, LinePaddingWidgetWrapper>();
+    padding = new ArrayList<LineWidget>();
 
     String diffColor = diff.meta_a() == null || diff.meta_b() == null
         ? DiffTable.style.intralineBg()
@@ -126,16 +124,18 @@ class ChunkManager {
     colorLines(cmB, color, startB, bLen);
     markEdit(cmA, startA, a, region.edit_a());
     markEdit(cmB, startB, b, region.edit_b());
+    addPadding(cmA, startA + aLen - 1, bLen - aLen);
+    addPadding(cmB, startB + bLen - 1, aLen - bLen);
     addGutterTag(region, startA, startB);
     mapper.appendReplace(aLen, bLen);
 
     int endA = mapper.getLineA() - 1;
     int endB = mapper.getLineB() - 1;
     if (aLen > 0) {
-      addDiffChunkAndPadding(cmB, endB, endA, aLen, bLen > 0);
+      addDiffChunk(cmB, endB, endA, aLen, bLen > 0);
     }
     if (bLen > 0) {
-      addDiffChunkAndPadding(cmA, endA, endB, bLen, aLen > 0);
+      addDiffChunk(cmA, endA, endB, bLen, aLen > 0);
     }
   }
 
@@ -202,13 +202,37 @@ class ChunkManager {
     }
   }
 
-  private void addDiffChunkAndPadding(CodeMirror cmToPad, int lineToPad,
+  /**
+   * Insert a new padding div below the given line.
+   *
+   * @param cm parent CodeMirror to add extra space into.
+   * @param line line to put the padding below.
+   * @param len number of lines to pad. Padding is inserted only if
+   *        {@code len >= 1}.
+   */
+  private void addPadding(CodeMirror cm, int line, int len) {
+    if (0 < len) {
+      // DiffTable adds 1px bottom padding to each line to preserve
+      // sufficient space for underscores commonly appearing in code.
+      // Padding should be 1em + 1px high for each line. Add within
+      // the browser using height + padding-bottom.
+      Element pad = DOM.createDiv();
+      pad.setClassName(DiffTable.style.padding());
+      pad.getStyle().setHeight(len, Unit.EM);
+      pad.getStyle().setPaddingBottom(len, Unit.PX);
+      padding.add(cm.addLineWidget(
+        line == -1 ? 0 : line,
+        pad,
+        Configuration.create()
+          .set("coverGutter", true)
+          .set("noHScroll", true)
+          .set("above", line == -1)));
+    }
+  }
+
+  private void addDiffChunk(CodeMirror cmToPad, int lineToPad,
       int lineOnOther, int chunkSize, boolean edit) {
-    CodeMirror otherCm = host.otherCm(cmToPad);
+    chunks.add(new DiffChunkInfo(host.otherCm(cmToPad).side(),
-    paddingOnOtherSide.put(otherCm.getLineHandle(lineOnOther),
-        new LinePaddingWidgetWrapper(host.addPaddingWidget(cmToPad,
-            lineToPad, 0, Unit.EM, null), lineToPad, chunkSize));
-    chunks.add(new DiffChunkInfo(otherCm.side(),
         lineOnOther - chunkSize + 1, lineOnOther, edit));
   }
 
@@ -291,62 +315,4 @@ class ChunkManager {
     }
     return null;
   }
-
-  void resizePadding(final CodeMirror cm,
-      final LineHandle line,
-      final DisplaySide side) {
-    if (paddingOnOtherSide.containsKey(line)) {
-      host.defer(new Runnable() {
-        @Override
-        public void run() {
-          resizePaddingOnOtherSide(side, cm.getLineNumber(line));
-        }
-      });
-    }
-  }
-
-  void resizePaddingOnOtherSide(DisplaySide mySide, int line) {
-    CodeMirror cm = host.getCmFromSide(mySide);
-    LineHandle handle = cm.getLineHandle(line);
-    final LinePaddingWidgetWrapper otherWrapper = paddingOnOtherSide.get(handle);
-    double myChunkHeight = cm.heightAtLine(line + 1) -
-        cm.heightAtLine(line - otherWrapper.getChunkLength() + 1);
-    Element otherPadding = otherWrapper.getElement();
-    int otherPaddingHeight = otherPadding.getOffsetHeight();
-    CodeMirror otherCm = host.otherCm(cm);
-    int otherLine = otherWrapper.getOtherLine();
-    LineHandle other = otherCm.getLineHandle(otherLine);
-    if (paddingOnOtherSide.containsKey(other)) {
-      LinePaddingWidgetWrapper myWrapper = paddingOnOtherSide.get(other);
-      Element myPadding = paddingOnOtherSide.get(other).getElement();
-      int myPaddingHeight = myPadding.getOffsetHeight();
-      myChunkHeight -= myPaddingHeight;
-      double otherChunkHeight = otherCm.heightAtLine(otherLine + 1) -
-          otherCm.heightAtLine(otherLine - myWrapper.getChunkLength() + 1) -
-          otherPaddingHeight;
-      double delta = myChunkHeight - otherChunkHeight;
-      if (delta > 0) {
-        if (myPaddingHeight != 0) {
-          myPadding.getStyle().setHeight((double) 0, Unit.PX);
-          myWrapper.getWidget().changed();
-        }
-        if (otherPaddingHeight != delta) {
-          otherPadding.getStyle().setHeight(delta, Unit.PX);
-          otherWrapper.getWidget().changed();
-        }
-      } else {
-        if (myPaddingHeight != -delta) {
-          myPadding.getStyle().setHeight(-delta, Unit.PX);
-          myWrapper.getWidget().changed();
-        }
-        if (otherPaddingHeight != 0) {
-          otherPadding.getStyle().setHeight((double) 0, Unit.PX);
-          otherWrapper.getWidget().changed();
-        }
-      }
-    } else if (otherPaddingHeight != myChunkHeight) {
-      otherPadding.getStyle().setHeight(myChunkHeight, Unit.PX);
-      otherWrapper.getWidget().changed();
-    }
-  }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBox.java

@@ -15,7 +15,6 @@
 package com.google.gerrit.client.diff;
 
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.diff.PaddingManager.PaddingWidgetWrapper;
 import com.google.gerrit.client.diff.SidePanel.GutterWrapper;
 import com.google.gwt.event.dom.client.MouseOutEvent;
 import com.google.gwt.event.dom.client.MouseOutHandler;
@@ -34,23 +33,17 @@ abstract class CommentBox extends Composite {
     Resources.I.style().ensureInjected();
   }
 
-  private PaddingManager widgetManager;
+  private final CommentGroup group;
-  private PaddingWidgetWrapper selfWidgetWrapper;
-  private CommentManager commentManager;
-  private CodeMirror cm;
-  private DiffChunkInfo diffChunkInfo;
   private GutterWrapper gutterWrapper;
   private FromTo fromTo;
   private TextMarker rangeMarker;
   private TextMarker rangeHighlightMarker;
 
-  CommentBox(CommentManager commentManager, CodeMirror cm, CommentInfo info) {
+  CommentBox(CommentGroup group, CommentRange range) {
-    this.commentManager = commentManager;
+    this.group = group;
-    this.cm = cm;
-    CommentRange range = info.range();
     if (range != null) {
       fromTo = FromTo.create(range);
-      rangeMarker = cm.markText(
+      rangeMarker = group.getCm().markText(
           fromTo.getFrom(),
           fromTo.getTo(),
           Configuration.create()
@@ -70,62 +63,21 @@ abstract class CommentBox extends Composite {
     }, MouseOutEvent.getType());
   }
 
-  @Override
-  protected void onLoad() {
-    resizePaddingWidget();
-  }
-
-  void resizePaddingWidget() {
-    if (!getCommentInfo().has_line()) {
-      return;
-    }
-    commentManager.getSideBySide2().defer(new Runnable() {
-      @Override
-      public void run() {
-        assert selfWidgetWrapper != null;
-        selfWidgetWrapper.getWidget().changed();
-        if (diffChunkInfo != null) {
-          commentManager.getSideBySide2().getChunkManager()
-            .resizePaddingOnOtherSide(cm.side(), diffChunkInfo.getEnd());
-        } else {
-          assert widgetManager != null;
-          widgetManager.resizePaddingWidget();
-        }
-      }
-    });
-  }
-
   abstract CommentInfo getCommentInfo();
   abstract boolean isOpen();
 
   void setOpen(boolean open) {
-    resizePaddingWidget();
+    group.resize();
     setRangeHighlight(open);
     getCm().focus();
   }
 
+  CommentGroup getCommentGroup() {
+    return group;
+  }
+
   CommentManager getCommentManager() {
-    return commentManager;
+    return group.getCommentManager();
-  }
-
-  PaddingManager getPaddingManager() {
-    return widgetManager;
-  }
-
-  void setPaddingManager(PaddingManager manager) {
-    widgetManager = manager;
-  }
-
-  void setSelfWidgetWrapper(PaddingWidgetWrapper wrapper) {
-    selfWidgetWrapper = wrapper;
-  }
-
-  PaddingWidgetWrapper getSelfWidgetWrapper() {
-    return selfWidgetWrapper;
-  }
-
-  void setDiffChunkInfo(DiffChunkInfo info) {
-    this.diffChunkInfo = info;
   }
 
   void setGutterWrapper(GutterWrapper wrapper) {
@@ -135,7 +87,7 @@ abstract class CommentBox extends Composite {
   void setRangeHighlight(boolean highlight) {
     if (fromTo != null) {
       if (highlight && rangeHighlightMarker == null) {
-        rangeHighlightMarker = cm.markText(
+        rangeHighlightMarker = group.getCm().markText(
             fromTo.getFrom(),
             fromTo.getTo(),
             Configuration.create()
@@ -150,6 +102,7 @@ abstract class CommentBox extends Composite {
   void clearRange() {
     if (rangeMarker != null) {
       rangeMarker.clear();
+      rangeMarker = null;
     }
   }
 
@@ -158,6 +111,6 @@ abstract class CommentBox extends Composite {
   }
 
   CodeMirror getCm() {
-    return cm;
+    return group.getCm();
   }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentBoxUi.css

@@ -13,18 +13,18 @@
  * limitations under the License.
  */
 
-.commentBox {
+.commentWidgets {
-  position: relative;
-  width: 679px;
-  min-height: 16px;
   font-family: sans-serif;
   background-color: #fcfa96;
   border: 1px solid black;
   -webkit-box-shadow: 3px 3px 3px #888888;
   -moz-box-shadow: 3px 3px 3px #888888;
   box-shadow: 3px 3px 3px #888888;
+
+  /* margin-bottom is fixed in CommentGroup.computeHeight() */
   margin-bottom: 5px;
   margin-right: 5px;
+
   -webkit-touch-callout: initial;
   -webkit-user-select: initial;
   -khtml-user-select: initial;
@@ -32,7 +32,15 @@
   -ms-user-select: initial;
 }
 
-.header { cursor: pointer; }
+.commentBox {
+  position: relative;
+  min-height: 16px;
+}
+
+.header {
+  max-width: 650px;
+  cursor: pointer;
+}
 
 .summary {
   color: #777;
@@ -58,13 +66,15 @@
   padding-top: 2px;
   position: relative;
 }
-.contents p,
+.message {
-.contents ul,
+  overflow-x: auto;
-.contents blockquote {
+}
+.message p,
+.message ul,
+.message blockquote {
   -webkit-margin-before: 0;
   -webkit-margin-after: 0.3em;
 }
-
 .commentBox button {
   margin-right: 3px;
   margin-bottom: 1px;

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java

@@ -0,0 +1,217 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.diff;
+
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.Style.Unit;
+import com.google.gwt.user.client.DOM;
+import com.google.gwt.user.client.Timer;
+import com.google.gwt.user.client.ui.Composite;
+import com.google.gwt.user.client.ui.FlowPanel;
+import com.google.gwt.user.client.ui.SimplePanel;
+
+import net.codemirror.lib.CodeMirror;
+import net.codemirror.lib.Configuration;
+import net.codemirror.lib.LineWidget;
+
+/**
+ * LineWidget attached to a CodeMirror container.
+ *
+ * When a comment is placed on a line a CommentWidget is created on both sides.
+ * The group tracks all comment boxes on that same line, and also includes an
+ * empty padding element to keep subsequent lines vertically aligned.
+ */
+class CommentGroup extends Composite {
+  static void pair(CommentGroup a, CommentGroup b) {
+    a.peer = b;
+    b.peer = a;
+  }
+
+  private final CommentManager manager;
+  private final CodeMirror cm;
+  private final int line;
+  private final FlowPanel comments;
+  private final Element padding;
+  private LineWidget lineWidget;
+  private Timer resizeTimer;
+  private CommentGroup peer;
+
+  CommentGroup(CommentManager manager, CodeMirror cm, int line) {
+    this.manager = manager;
+    this.cm = cm;
+    this.line = line;
+
+    comments = new FlowPanel();
+    comments.setStyleName(Resources.I.style().commentWidgets());
+    comments.setVisible(false);
+    initWidget(new SimplePanel(comments));
+
+    padding = DOM.createDiv();
+    padding.setClassName(DiffTable.style.padding());
+    getElement().appendChild(padding);
+  }
+
+  CommentManager getCommentManager() {
+    return manager;
+  }
+
+  CodeMirror getCm() {
+    return cm;
+  }
+
+  void add(PublishedBox box) {
+    comments.add(box);
+    comments.setVisible(true);
+  }
+
+  void add(DraftBox box) {
+    PublishedBox p = box.getReplyToBox();
+    if (p != null) {
+      for (int i = 0; i < getBoxCount(); i++) {
+        if (p == getCommentBox(i)) {
+          comments.insert(box, i + 1);
+          comments.setVisible(true);
+          resize();
+          return;
+        }
+      }
+    }
+    comments.add(box);
+    comments.setVisible(true);
+    resize();
+  }
+
+  CommentBox getCommentBox(int i) {
+    return (CommentBox) comments.getWidget(i);
+  }
+
+  int getBoxCount() {
+    return comments.getWidgetCount();
+  }
+
+  void openCloseLast() {
+    if (0 < getBoxCount()) {
+      CommentBox box = getCommentBox(getBoxCount() - 1);
+      box.setOpen(!box.isOpen());
+    }
+  }
+
+  void openCloseAll() {
+    boolean open = false;
+    for (int i = 0; i < getBoxCount(); i++) {
+      if (!getCommentBox(i).isOpen()) {
+        open = true;
+        break;
+      }
+    }
+    for (int i = 0; i < getBoxCount(); i++) {
+      getCommentBox(i).setOpen(open);
+    }
+  }
+
+  void remove(DraftBox box) {
+    comments.remove(box);
+    comments.setVisible(0 < getBoxCount());
+
+    if (0 < getBoxCount() || 0 < peer.getBoxCount()) {
+      resize();
+    } else {
+      detach();
+      peer.detach();
+    }
+  }
+
+  private void detach() {
+    if (lineWidget != null) {
+      lineWidget.clear();
+      lineWidget = null;
+    }
+    manager.clearLine(cm.side(), line);
+    removeFromParent();
+  }
+
+  void attach(DiffTable parent) {
+    parent.add(this);
+    lineWidget = cm.addLineWidget(Math.max(0, line - 1), getElement(),
+        Configuration.create()
+          .set("coverGutter", true)
+          .set("noHScroll", true)
+          .set("above", line <= 0)
+          .set("insertAt", 0));
+  }
+
+  void handleRedraw() {
+    lineWidget.onRedraw(new Runnable() {
+      @Override
+      public void run() {
+        if (canComputeHeight() && peer.canComputeHeight()) {
+          if (resizeTimer != null) {
+            resizeTimer.cancel();
+            resizeTimer = null;
+          }
+          adjustPadding(CommentGroup.this, peer);
+        } else if (resizeTimer == null) {
+          resizeTimer = new Timer() {
+            @Override
+            public void run() {
+              if (canComputeHeight() && peer.canComputeHeight()) {
+                cancel();
+                resizeTimer = null;
+                adjustPadding(CommentGroup.this, peer);
+              }
+            }
+          };
+          resizeTimer.scheduleRepeating(5);
+        }
+      }
+    });
+  }
+
+  @Override
+  protected void onUnload() {
+    super.onUnload();
+    if (resizeTimer != null) {
+      resizeTimer.cancel();
+    }
+  }
+
+  void resize() {
+    if (lineWidget != null) {
+      adjustPadding(this, peer);
+    }
+  }
+
+  private boolean canComputeHeight() {
+    return !comments.isVisible() || comments.getOffsetHeight() > 0;
+  }
+
+  private int computeHeight() {
+    if (comments.isVisible()) {
+      // Include margin-bottom: 5px from CSS class.
+      return comments.getOffsetHeight() + 5;
+    }
+    return 0;
+  }
+
+  private static void adjustPadding(CommentGroup a, CommentGroup b) {
+    int apx = a.computeHeight();
+    int bpx = b.computeHeight();
+    int h = Math.max(apx, bpx);
+    a.padding.getStyle().setHeight(Math.max(0, h - apx), Unit.PX);
+    b.padding.getStyle().setHeight(Math.max(0, h - bpx), Unit.PX);
+    a.lineWidget.changed();
+    b.lineWidget.changed();
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentManager.java

@@ -16,7 +16,6 @@ package com.google.gerrit.client.diff;
 
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.diff.PaddingManager.PaddingWidgetWrapper;
 import com.google.gerrit.client.patches.SkippedLine;
 import com.google.gerrit.client.rpc.CallbackGroup;
 import com.google.gerrit.client.rpc.Natives;
@@ -24,13 +23,8 @@ import com.google.gerrit.client.ui.CommentLinkProcessor;
 import com.google.gerrit.common.changes.Side;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gwt.core.client.JsArray;
-import com.google.gwt.core.client.Scheduler;
-import com.google.gwt.core.client.Scheduler.ScheduledCommand;
-import com.google.gwt.dom.client.Style.Unit;
 
 import net.codemirror.lib.CodeMirror;
-import net.codemirror.lib.Configuration;
-import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.CodeMirror.LineHandle;
 import net.codemirror.lib.TextMarker.FromTo;
 
@@ -40,6 +34,8 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 
 /** Tracks comment widgets for {@link SideBySide2}. */
 class CommentManager {
@@ -50,10 +46,10 @@ class CommentManager {
   private final CommentLinkProcessor commentLinkProcessor;
 
   private final Map<String, PublishedBox> published;
-  private final Map<LineHandle, CommentBox> lineActiveBox;
+  private final SortedMap<Integer, CommentGroup> sideA;
-  private final Map<LineHandle, List<PublishedBox>> linePublishedBoxes;
+  private final SortedMap<Integer, CommentGroup> sideB;
-  private final Map<LineHandle, PaddingManager> linePaddingManager;
   private final Set<DraftBox> unsavedDrafts;
+  private boolean attached;
 
   CommentManager(SideBySide2 host,
       PatchSet.Id base, PatchSet.Id revision,
@@ -66,9 +62,8 @@ class CommentManager {
     this.commentLinkProcessor = clp;
 
     published = new HashMap<String, PublishedBox>();
-    lineActiveBox = new HashMap<LineHandle, CommentBox>();
+    sideA = new TreeMap<Integer, CommentGroup>();
-    linePublishedBoxes = new HashMap<LineHandle, List<PublishedBox>>();
+    sideB = new TreeMap<Integer, CommentGroup>();
-    linePaddingManager = new HashMap<LineHandle, PaddingManager>();
     unsavedDrafts = new HashSet<DraftBox>();
   }
 
@@ -82,7 +77,7 @@ class CommentManager {
     }
   }
 
-  void render(CommentsCollections in) {
+  void render(CommentsCollections in, boolean expandAll) {
     if (in.publishedBase != null) {
       renderPublished(DisplaySide.A, in.publishedBase);
     }
@@ -95,64 +90,100 @@ class CommentManager {
     if (in.draftsRevision != null) {
       renderDrafts(DisplaySide.B, in.draftsRevision);
     }
+    if (expandAll) {
+      setExpandAllComments(true);
+    }
+    for (CommentGroup g : sideA.values()) {
+      g.attach(host.diffTable);
+    }
+    for (CommentGroup g : sideB.values()) {
+      g.attach(host.diffTable);
+      g.handleRedraw();
+    }
+    attached = true;
   }
 
   private void renderPublished(DisplaySide forSide, JsArray<CommentInfo> in) {
     for (CommentInfo info : Natives.asList(in)) {
       DisplaySide side = displaySide(info, forSide);
-      if (side == null) {
+      if (side != null) {
-        continue;
+        CommentGroup group = group(side, info.line());
+        PublishedBox box = new PublishedBox(
+            group,
+            commentLinkProcessor,
+            getPatchSetIdFromSide(side),
+            info);
+        group.add(box);
+        box.setGutterWrapper(host.diffTable.sidePanel.addGutter(
+            host.getCmFromSide(side),
+            Math.max(0, info.line() - 1),
+            SidePanel.GutterType.COMMENT));
+        published.put(info.id(), box);
       }
-
-      CodeMirror cm = host.getCmFromSide(side);
-      PublishedBox box = new PublishedBox(this, cm, commentLinkProcessor,
-          getPatchSetIdFromSide(side), info);
-      published.put(info.id(), box);
-      if (!info.has_line()) {
-        host.diffTable.addFileCommentBox(box);
-        continue;
-      }
-
-      int line = info.line() - 1;
-      LineHandle handle = cm.getLineHandle(line);
-      if (linePublishedBoxes.containsKey(handle)) {
-        linePublishedBoxes.get(handle).add(box);
-      } else {
-        List<PublishedBox> list = new ArrayList<PublishedBox>(4);
-        list.add(box);
-        linePublishedBoxes.put(handle, list);
-      }
-      lineActiveBox.put(handle, box);
-      addCommentBox(info, box);
     }
   }
 
   private void renderDrafts(DisplaySide forSide, JsArray<CommentInfo> in) {
     for (CommentInfo info : Natives.asList(in)) {
       DisplaySide side = displaySide(info, forSide);
-      if (side == null) {
+      if (side != null) {
-        continue;
+        addDraftBox(side, info);
       }
-
-      CodeMirror cm = host.getCmFromSide(side);
-      DraftBox box = new DraftBox(this, cm, commentLinkProcessor,
-          getPatchSetIdFromSide(side), info);
-      if (info.in_reply_to() != null) {
-        PublishedBox r = published.get(info.in_reply_to());
-        if (r != null) {
-          r.registerReplyBox(box);
-        }
-      }
-      if (!info.has_line()) {
-        host.diffTable.addFileCommentBox(box);
-        continue;
-      }
-
-      lineActiveBox.put(cm.getLineHandle(info.line() - 1), box);
-      addCommentBox(info, box);
     }
   }
 
+  /**
+   * Create a new {@link DraftBox} at the specified line and focus it.
+   *
+   * @param side which side the draft will appear on.
+   * @param line the line the draft will be at. Lines are 1-based. Line 0 is a
+   *        special case creating a file level comment.
+   */
+  void insertNewDraft(DisplaySide side, int line) {
+    if (line == 0) {
+      host.getSkipManager().ensureFirstLineIsVisible();
+    }
+
+    CommentGroup group = group(side, line);
+    if (0 < group.getBoxCount()) {
+      CommentBox last = group.getCommentBox(group.getBoxCount() - 1);
+      if (last instanceof DraftBox) {
+        ((DraftBox)last).setEdit(true);
+      } else {
+        ((PublishedBox)last).doReply();
+      }
+    } else {
+      addDraftBox(side, CommentInfo.create(
+          path,
+          getStoredSideFromDisplaySide(side),
+          line,
+          null)).setEdit(true);
+    }
+  }
+
+  DraftBox addDraftBox(DisplaySide side, CommentInfo info) {
+    CommentGroup group = group(side, info.line());
+    DraftBox box = new DraftBox(
+        group,
+        commentLinkProcessor,
+        getPatchSetIdFromSide(side),
+        info);
+
+    if (info.in_reply_to() != null) {
+      PublishedBox r = published.get(info.in_reply_to());
+      if (r != null) {
+        r.setReplyBox(box);
+      }
+    }
+
+    group.add(box);
+    box.setGutterWrapper(host.diffTable.sidePanel.addGutter(
+        host.getCmFromSide(side),
+        Math.max(0, info.line() - 1),
+        SidePanel.GutterType.DRAFT));
+    return box;
+  }
+
   private DisplaySide displaySide(CommentInfo info, DisplaySide forSide) {
     if (info.side() == Side.PARENT) {
       return base == null ? DisplaySide.A : null;
@@ -161,15 +192,21 @@ class CommentManager {
   }
 
   List<SkippedLine> splitSkips(int context, List<SkippedLine> skips) {
+    if (sideB.containsKey(0)) {
+      // Special case of file comment; cannot skip first line.
+      for (SkippedLine skip : skips) {
+        if (skip.getStartB() == 0) {
+          skip.incrementStart(1);
+        }
+      }
+    }
+
     // TODO: This is not optimal, but shouldn't be too costly in most cases.
     // Maybe rewrite after done keeping track of diff chunk positions.
-    for (CommentBox box : lineActiveBox.values()) {
+    for (int boxLine : sideB.tailMap(1).keySet()) {
-      int boxLine = box.getCommentInfo().line();
-      boolean sideA = box.getCm().side() == DisplaySide.A;
-
       List<SkippedLine> temp = new ArrayList<SkippedLine>(skips.size() + 2);
       for (SkippedLine skip : skips) {
-        int startLine = sideA ? skip.getStartA() : skip.getStartB();
+        int startLine = skip.getStartB();
         int deltaBefore = boxLine - startLine;
         int deltaAfter = startLine + skip.getSize() - boxLine;
         if (deltaBefore < -context || deltaAfter < -context) {
@@ -203,36 +240,33 @@ class CommentManager {
     }
   }
 
+  void clearLine(DisplaySide side, int line) {
+    map(side).remove(line);
+  }
+
   Runnable toggleOpenBox(final CodeMirror cm) {
     return new Runnable() {
       public void run() {
-        CommentBox box = lineActiveBox.get(cm.getActiveLine());
+        if (cm.hasActiveLine()) {
-        if (box != null) {
+          CommentGroup w = map(cm.side()).get(
-          box.setOpen(!box.isOpen());
+              cm.getLineNumber(cm.getActiveLine()) + 1);
+          if (w != null) {
+            w.openCloseLast();
+          }
         }
       }
     };
   }
 
-  Runnable openClosePublished(final CodeMirror cm) {
+  Runnable openCloseAll(final CodeMirror cm) {
     return new Runnable() {
       @Override
       public void run() {
         if (cm.hasActiveLine()) {
-          List<PublishedBox> list =
+          CommentGroup w = map(cm.side()).get(
-              linePublishedBoxes.get(cm.getActiveLine());
+              cm.getLineNumber(cm.getActiveLine()) + 1);
-          if (list == null) {
+          if (w != null) {
-            return;
+            w.openCloseAll();
-          }
-          boolean open = false;
-          for (PublishedBox box : list) {
-            if (!box.isOpen()) {
-              open = true;
-              break;
-            }
-          }
-          for (PublishedBox box : list) {
-            box.setOpen(open);
           }
         }
       }
@@ -244,156 +278,38 @@ class CommentManager {
       return new Runnable() {
         @Override
         public void run() {
-          Gerrit.doSignIn(host.getToken());
+          String token = host.getToken();
+          if (cm.hasActiveLine()) {
+            LineHandle handle = cm.getActiveLine();
+            int line = cm.getLineNumber(handle) + 1;
+            token += "@" + (cm.side() == DisplaySide.A ? "a" : "") + line;
+          }
+          Gerrit.doSignIn(token);
         }
      };
     }
 
     return new Runnable() {
       public void run() {
-        LineHandle handle = cm.getActiveLine();
+        if (cm.hasActiveLine()) {
-        int line = cm.getLineNumber(handle);
+          newDraft(cm);
-        CommentBox box = lineActiveBox.get(handle);
-        FromTo fromTo = cm.getSelectedRange();
-        if (cm.somethingSelected()) {
-          lineActiveBox.put(handle,
-              newRangeDraft(cm, line, fromTo.getTo().getLine() == line ? fromTo : null));
-          cm.setSelection(cm.getCursor());
-        } else if (box == null) {
-          lineActiveBox.put(handle, newRangeDraft(cm, line, null));
-        } else if (box instanceof DraftBox) {
-          ((DraftBox) box).setEdit(true);
-        } else {
-          ((PublishedBox) box).doReply();
         }
       }
     };
   }
 
-  private DraftBox newRangeDraft(CodeMirror cm, int line, FromTo fromTo) {
+  private void newDraft(CodeMirror cm) {
-    DisplaySide side = cm.side();
+    int line = cm.getLineNumber(cm.getActiveLine()) + 1;
-    return addDraftBox(CommentInfo.createRange(
+    if (cm.somethingSelected()) {
-        path,
+      FromTo fromTo = cm.getSelectedRange();
-        getStoredSideFromDisplaySide(side),
+      addDraftBox(cm.side(), CommentInfo.create(
-        line + 1,
+              path,
-        null,
+              getStoredSideFromDisplaySide(cm.side()),
-        null,
+              line,
-        CommentRange.create(fromTo)), side);
+              CommentRange.create(fromTo))).setEdit(true);
-  }
+      cm.setSelection(cm.getCursor());
-
-  DraftBox newFileDraft(DisplaySide side) {
-    return addDraftBox(CommentInfo.createFile(
-      path,
-      getStoredSideFromDisplaySide(side),
-      null, null), side);
-  }
-
-  CommentInfo createReply(CommentInfo replyTo) {
-    if (!replyTo.has_line() && replyTo.range() == null) {
-      return CommentInfo.createFile(path, replyTo.side(), replyTo.id(), null);
     } else {
-      return CommentInfo.createRange(path, replyTo.side(), replyTo.line(),
+      insertNewDraft(cm.side(), line);
-          replyTo.id(), null, replyTo.range());
-    }
-  }
-
-  DraftBox addDraftBox(CommentInfo info, DisplaySide side) {
-    CodeMirror cm = host.getCmFromSide(side);
-    final DraftBox box = new DraftBox(this, cm, commentLinkProcessor,
-        getPatchSetIdFromSide(side), info);
-    if (info.id() == null) {
-      Scheduler.get().scheduleDeferred(new ScheduledCommand() {
-        @Override
-        public void execute() {
-          box.setOpen(true);
-          box.setEdit(true);
-        }
-      });
-    }
-    if (!info.has_line()) {
-      return box;
-    }
-    addCommentBox(info, box);
-    box.setVisible(true);
-    LineHandle handle = cm.getLineHandle(info.line() - 1);
-    lineActiveBox.put(handle, box);
-    return box;
-  }
-
-  private CommentBox addCommentBox(CommentInfo info, final CommentBox box) {
-    host.diffTable.add(box);
-    CodeMirror cm = box.getCm();
-    CodeMirror other = host.otherCm(cm);
-    int line = info.line() - 1; // CommentInfo is 1-based, but CM is 0-based
-    LineHandle handle = cm.getLineHandle(line);
-    PaddingManager manager;
-    if (linePaddingManager.containsKey(handle)) {
-      manager = linePaddingManager.get(handle);
-    } else {
-      // Estimated height at 28px, fixed by deferring after display
-      manager = new PaddingManager(host.addPaddingWidget(cm, line, 0, Unit.PX, 0));
-      linePaddingManager.put(handle, manager);
-    }
-
-    int lineToPad = host.lineOnOther(cm.side(), line).getLine();
-    LineHandle otherHandle = other.getLineHandle(lineToPad);
-    ChunkManager chunkMgr = host.getChunkManager();
-    DiffChunkInfo myChunk = chunkMgr.getDiffChunk(cm.side(), line);
-    DiffChunkInfo otherChunk = chunkMgr.getDiffChunk(other.side(), lineToPad);
-    PaddingManager otherManager;
-    if (linePaddingManager.containsKey(otherHandle)) {
-      otherManager = linePaddingManager.get(otherHandle);
-    } else {
-      otherManager =
-          new PaddingManager(host.addPaddingWidget(other, lineToPad, 0, Unit.PX, 0));
-      linePaddingManager.put(otherHandle, otherManager);
-    }
-    if ((myChunk == null && otherChunk == null) || (myChunk != null && otherChunk != null)) {
-      PaddingManager.link(manager, otherManager);
-    }
-
-    int index = manager.getCurrentCount();
-    manager.insert(box, index);
-    Configuration config = Configuration.create()
-      .set("coverGutter", true)
-      .set("insertAt", index)
-      .set("noHScroll", true);
-    LineWidget boxWidget = host.addLineWidget(cm, line, box, config);
-    box.setPaddingManager(manager);
-    box.setSelfWidgetWrapper(new PaddingWidgetWrapper(boxWidget, box.getElement()));
-    if (otherChunk == null) {
-      box.setDiffChunkInfo(myChunk);
-    }
-    box.setGutterWrapper(host.diffTable.sidePanel.addGutter(cm, info.line() - 1,
-        box instanceof DraftBox
-          ? SidePanel.GutterType.DRAFT
-          : SidePanel.GutterType.COMMENT));
-    return box;
-  }
-
-  void removeDraft(DraftBox box) {
-    int line = box.getCommentInfo().line() - 1;
-    LineHandle handle = box.getCm().getLineHandle(line);
-    lineActiveBox.remove(handle);
-    if (linePublishedBoxes.containsKey(handle)) {
-      List<PublishedBox> list = linePublishedBoxes.get(handle);
-      lineActiveBox.put(handle, list.get(list.size() - 1));
-    }
-    unsavedDrafts.remove(box);
-  }
-
-  void addFileCommentBox(CommentBox box) {
-    host.diffTable.addFileCommentBox(box);
-  }
-
-  void removeFileCommentBox(DraftBox box) {
-    host.diffTable.onRemoveDraftBox(box);
-  }
-
-  void resizePadding(LineHandle handle) {
-    CommentBox box = lineActiveBox.get(handle);
-    if (box != null) {
-      box.resizePaddingWidget();
     }
   }
 
@@ -411,6 +327,47 @@ class CommentManager {
     }
   }
 
+  private CommentGroup group(DisplaySide side, int line) {
+    CommentGroup w = map(side).get(line);
+    if (w != null) {
+      return w;
+    }
+
+    int lineA, lineB;
+    if (line == 0) {
+      lineA = lineB = 0;
+    } else if (side == DisplaySide.A) {
+      lineA = line;
+      lineB = host.lineOnOther(side, line - 1).getLine() + 1;
+    } else {
+      lineA = host.lineOnOther(side, line - 1).getLine() + 1;
+      lineB = line;
+    }
+
+    CommentGroup a = newGroup(DisplaySide.A, lineA);
+    CommentGroup b = newGroup(DisplaySide.B, lineB);
+    CommentGroup.pair(a, b);
+
+    sideA.put(lineA, a);
+    sideB.put(lineB, b);
+
+    if (attached) {
+      a.attach(host.diffTable);
+      b.attach(host.diffTable);
+      b.handleRedraw();
+    }
+
+    return side == DisplaySide.A ? a : b;
+  }
+
+  private CommentGroup newGroup(DisplaySide side, int line) {
+    return new CommentGroup(this, host.getCmFromSide(side), line);
+  }
+
+  private SortedMap<Integer, CommentGroup> map(DisplaySide side) {
+    return side == DisplaySide.A ? sideA : sideB;
+  }
+
   private Side getStoredSideFromDisplaySide(DisplaySide side) {
     return side == DisplaySide.A && base == null ? Side.PARENT : Side.REVISION;
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentsCollections.java

@@ -89,8 +89,11 @@ class CommentsCollections {
     };
   }
 
-  private static JsArray<CommentInfo> sort(JsArray<CommentInfo> in) {
+  private JsArray<CommentInfo> sort(JsArray<CommentInfo> in) {
     if (in != null) {
+      for (CommentInfo c : Natives.asList(in)) {
+        c.path(path);
+      }
       Collections.sort(Natives.asList(in), new Comparator<CommentInfo>() {
         @Override
         public int compare(CommentInfo a, CommentInfo b) {

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.java

@@ -24,6 +24,7 @@ import com.google.gwt.resources.client.CssResource;
 import com.google.gwt.uibinder.client.UiBinder;
 import com.google.gwt.uibinder.client.UiField;
 import com.google.gwt.user.client.ui.Composite;
+import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.HTMLPanel;
 import com.google.gwt.user.client.ui.UIObject;
 import com.google.gwt.user.client.ui.Widget;
@@ -50,23 +51,14 @@ class DiffTable extends Composite {
     String padding();
   }
 
-  @UiField
+  @UiField Element cmA;
-  Element cmA;
+  @UiField Element cmB;
-
+  @UiField SidePanel sidePanel;
-  @UiField
+  @UiField Element patchSetNavRow;
-  Element cmB;
+  @UiField Element patchSetNavCellA;
-
+  @UiField Element patchSetNavCellB;
-  @UiField
+  @UiField FlowPanel widgets;
-  SidePanel sidePanel;
+  @UiField static DiffTableStyle style;
-
-  @UiField
-  Element patchSetNavRow;
-
-  @UiField
-  Element patchSetNavCellA;
-
-  @UiField
-  Element patchSetNavCellB;
 
   @UiField(provided = true)
   PatchSetSelectBox2 patchSetSelectBoxA;
@@ -74,36 +66,16 @@ class DiffTable extends Composite {
   @UiField(provided = true)
   PatchSetSelectBox2 patchSetSelectBoxB;
 
-  @UiField
-  Element fileCommentRow;
-
-  @UiField
-  Element fileCommentCellA;
-
-  @UiField
-  Element fileCommentCellB;
-
-  @UiField(provided = true)
-  FileCommentPanel fileCommentPanelA;
-
-  @UiField(provided = true)
-  FileCommentPanel fileCommentPanelB;
-
-  @UiField
-  static DiffTableStyle style;
-
   private SideBySide2 parent;
   private boolean headerVisible;
 
   DiffTable(SideBySide2 parent, PatchSet.Id base, PatchSet.Id revision, String path) {
     patchSetSelectBoxA = new PatchSetSelectBox2(
-        this, DisplaySide.A, revision.getParentKey(), base, path);
+        parent, DisplaySide.A, revision.getParentKey(), base, path);
     patchSetSelectBoxB = new PatchSetSelectBox2(
-        this, DisplaySide.B, revision.getParentKey(), revision, path);
+        parent, DisplaySide.B, revision.getParentKey(), revision, path);
     PatchSetSelectBox2.link(patchSetSelectBoxA, patchSetSelectBoxB);
 
-    fileCommentPanelA = new FileCommentPanel(parent, this, DisplaySide.A);
-    fileCommentPanelB = new FileCommentPanel(parent, this, DisplaySide.B);
     initWidget(uiBinder.createAndBindUi(this));
     this.parent = parent;
     this.headerVisible = true;
@@ -117,9 +89,6 @@ class DiffTable extends Composite {
     headerVisible = show;
     Gerrit.setHeaderVisible(show && !parent.getPrefs().hideTopMenu());
     UIObject.setVisible(patchSetNavRow, show);
-    UIObject.setVisible(fileCommentRow, show
-        && (fileCommentPanelA.getBoxCount() > 0
-            || fileCommentPanelB.getBoxCount() > 0));
     if (show) {
       parent.header.removeStyleName(style.fullscreen());
     } else {
@@ -128,25 +97,8 @@ class DiffTable extends Composite {
     parent.resizeCodeMirror();
   }
 
-  private FileCommentPanel getPanelFromSide(DisplaySide side) {
-    return side == DisplaySide.A ? fileCommentPanelA : fileCommentPanelB;
-  }
-
-  void createOrEditFileComment(DisplaySide side) {
-    getPanelFromSide(side).createOrEditFileComment();
-    setHeaderVisible(true);
-  }
-
-  void addFileCommentBox(CommentBox box) {
-    getPanelFromSide(box.getCm().side()).addFileComment(box);
-  }
-
-  void onRemoveDraftBox(DraftBox box) {
-    getPanelFromSide(box.getCm().side()).onRemoveDraftBox(box);
-  }
-
   int getHeaderHeight() {
-    return fileCommentRow.getOffsetHeight() + patchSetSelectBoxA.getOffsetHeight();
+    return patchSetSelectBoxA.getOffsetHeight();
   }
 
   void setUpPatchSetNav(JsArray<RevisionInfo> list, DiffInfo info) {
@@ -155,6 +107,6 @@ class DiffTable extends Composite {
   }
 
   void add(Widget widget) {
-    ((HTMLPanel) getWidget()).add(widget);
+    widgets.add(widget);
   }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DiffTable.ui.xml

@@ -44,14 +44,19 @@ limitations under the License.
     .difftable .CodeMirror-lines { padding: 0; }
     .difftable .CodeMirror pre {
       padding: 0;
-      padding-bottom: 0.11em;
       overflow: hidden;
       border-right: 0;
       width: auto;
     }
+
+    /* Preserve space for underscores. If this changes
+     * see ChunkManager.addPadding() and adjust there.
+     */
+    .difftable .CodeMirror pre,
     .difftable .CodeMirror pre span {
-      padding-bottom: 0.11em;
+      padding-bottom: 1px;
     }
+
     .contentCell {
       padding: 0;
     }
@@ -144,14 +149,6 @@ limitations under the License.
                 <d:PatchSetSelectBox2 ui:field='patchSetSelectBoxB' />
               </td>
             </tr>
-            <tr ui:field='fileCommentRow' class='{style.fileCommentRow}'>
-              <td ui:field='fileCommentCellA' class='{style.fileCommentCell}'>
-                <d:FileCommentPanel ui:field='fileCommentPanelA' />
-              </td>
-              <td ui:field='fileCommentCellB' class='{style.fileCommentCell}'>
-                <d:FileCommentPanel ui:field='fileCommentPanelB' />
-              </td>
-            </tr>
             <tr>
               <td ui:field='cmA' class='{style.a}'></td>
               <td ui:field='cmB' class='{style.b}'></td>
@@ -161,5 +158,6 @@ limitations under the License.
         <td class='{style.sidePanelCell}'><d:SidePanel ui:field='sidePanel'/></td>
       </tr>
     </table>
+    <g:FlowPanel ui:field='widgets' visible='false'/>
   </g:HTMLPanel>
 </ui:UiBinder>

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.java

@@ -17,7 +17,6 @@ package com.google.gerrit.client.diff;
 import com.google.gerrit.client.FormatUtil;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.changes.CommentInput;
 import com.google.gerrit.client.rpc.CallbackGroup;
 import com.google.gerrit.client.rpc.GerritCallback;
 import com.google.gerrit.client.ui.CommentLinkProcessor;
@@ -27,6 +26,7 @@ import com.google.gwt.core.client.JavaScriptObject;
 import com.google.gwt.core.client.Scheduler;
 import com.google.gwt.core.client.Scheduler.RepeatingCommand;
 import com.google.gwt.dom.client.Element;
+import com.google.gwt.event.dom.client.BlurEvent;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.dom.client.DoubleClickEvent;
@@ -35,6 +35,8 @@ import com.google.gwt.event.dom.client.KeyCodes;
 import com.google.gwt.event.dom.client.KeyDownEvent;
 import com.google.gwt.event.dom.client.MouseMoveEvent;
 import com.google.gwt.event.dom.client.MouseMoveHandler;
+import com.google.gwt.event.dom.client.MouseUpEvent;
+import com.google.gwt.event.dom.client.MouseUpHandler;
 import com.google.gwt.uibinder.client.UiBinder;
 import com.google.gwt.uibinder.client.UiField;
 import com.google.gwt.uibinder.client.UiHandler;
@@ -47,8 +49,6 @@ import com.google.gwt.user.client.ui.Widget;
 import com.google.gwtexpui.globalkey.client.NpTextArea;
 import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
 
-import net.codemirror.lib.CodeMirror;
-
 /** An HtmlPanel for displaying and editing a draft */
 class DraftBox extends CommentBox {
   interface Binder extends UiBinder<HTMLPanel, DraftBox> {}
@@ -62,6 +62,8 @@ class DraftBox extends CommentBox {
   private CommentInfo comment;
   private PublishedBox replyToBox;
   private Timer expandTimer;
+  private Timer resizeTimer;
+  private int editAreaHeight;
   private boolean autoClosed;
 
   @UiField Widget header;
@@ -80,12 +82,11 @@ class DraftBox extends CommentBox {
   @UiField Button discard2;
 
   DraftBox(
-      CommentManager manager,
+      CommentGroup group,
-      CodeMirror cm,
       CommentLinkProcessor clp,
       PatchSet.Id id,
       CommentInfo info) {
-    super(manager, cm, info);
+    super(group, info.range());
 
     linkProcessor = clp;
     psId = id;
@@ -112,6 +113,7 @@ class DraftBox extends CommentBox {
         }
       }
     }, ClickEvent.getType());
+
     addDomHandler(new DoubleClickHandler() {
       @Override
       public void onDoubleClick(DoubleClickEvent event) {
@@ -123,12 +125,8 @@ class DraftBox extends CommentBox {
         }
       }
     }, DoubleClickEvent.getType());
-    addDomHandler(new MouseMoveHandler() {
+
-      @Override
+    initResizeHandler();
-      public void onMouseMove(MouseMoveEvent event) {
-        resizePaddingWidget();
-      }
-    }, MouseMoveEvent.getType());
   }
 
   private void set(CommentInfo info) {
@@ -170,7 +168,8 @@ class DraftBox extends CommentBox {
     if (editArea.getVisibleLines() != rows) {
       editArea.setVisibleLines(rows);
     }
-    resizePaddingWidget();
+    editAreaHeight = editArea.getOffsetHeight();
+    getCommentGroup().resize();
   }
 
   boolean isEdit() {
@@ -191,6 +190,7 @@ class DraftBox extends CommentBox {
       editArea.setFocus(true);
       cancel.setVisible(!isNew());
       expandText();
+      editAreaHeight = editArea.getOffsetHeight();
       if (msg.length() > 0) {
         Scheduler.get().scheduleFixedDelay(new RepeatingCommand() {
           @Override
@@ -202,18 +202,24 @@ class DraftBox extends CommentBox {
       }
     } else {
       expandTimer.cancel();
+      resizeTimer.cancel();
     }
     getCommentManager().setUnsaved(this, edit);
-    resizePaddingWidget();
+    getCommentGroup().resize();
   }
 
-  void registerReplyToBox(PublishedBox box) {
+  PublishedBox getReplyToBox() {
+    return replyToBox;
+  }
+
+  void setReplyToBox(PublishedBox box) {
     replyToBox = box;
   }
 
   @Override
   protected void onUnload() {
     expandTimer.cancel();
+    resizeTimer.cancel();
     super.onUnload();
   }
 
@@ -221,20 +227,13 @@ class DraftBox extends CommentBox {
     if (replyToBox != null) {
       replyToBox.unregisterReplyBox();
     }
-    clearRange();
+
+    getCommentManager().setUnsaved(this, false);
     setRangeHighlight(false);
-    removeFromParent();
+    clearRange();
-    if (!getCommentInfo().has_line()) {
-      getCommentManager().removeFileCommentBox(this);
-      return;
-    }
-    PaddingManager manager = getPaddingManager();
-    manager.remove(this);
-    getCommentManager().removeDraft(this);
-    getCm().focus();
-    getSelfWidgetWrapper().getWidget().clear();
     getGutterWrapper().remove();
-    resizePaddingWidget();
+    getCommentGroup().remove(this);
+    getCm().focus();
   }
 
   @UiHandler("message")
@@ -265,9 +264,8 @@ class DraftBox extends CommentBox {
       return;
     }
 
-    CommentInfo original = comment;
+    CommentInfo input = CommentInfo.copy(comment);
-    CommentInput input = CommentInput.create(original);
+    input.message(message);
-    input.setMessage(message);
     enableEdit(false);
 
     GerritCallback<CommentInfo> cb = new GerritCallback<CommentInfo>() {
@@ -288,11 +286,11 @@ class DraftBox extends CommentBox {
         super.onFailure(e);
       }
     };
-    if (original.id() == null) {
+    if (input.id() == null) {
       CommentApi.createDraft(psId, input, group == null ? cb : group.add(cb));
     } else {
       CommentApi.updateDraft(
-          psId, original.id(), input, group == null ? cb : group.add(cb));
+          psId, input.id(), input, group == null ? cb : group.add(cb));
     }
     getCm().focus();
   }
@@ -337,6 +335,7 @@ class DraftBox extends CommentBox {
 
   @UiHandler("editArea")
   void onKeyDown(KeyDownEvent e) {
+    resizeTimer.cancel();
     if ((e.isControlKeyDown() || e.isMetaKeyDown())
         && !e.isAltKeyDown() && !e.isShiftKeyDown()) {
       switch (e.getNativeKeyCode()) {
@@ -362,6 +361,40 @@ class DraftBox extends CommentBox {
     expandTimer.schedule(250);
   }
 
+  @UiHandler("editArea")
+  void onBlur(BlurEvent e) {
+    resizeTimer.cancel();
+  }
+
+  private void initResizeHandler() {
+    resizeTimer = new Timer() {
+      @Override
+      public void run() {
+        getCommentGroup().resize();
+      }
+    };
+
+    addDomHandler(new MouseMoveHandler() {
+      @Override
+      public void onMouseMove(MouseMoveEvent event) {
+        int h = editArea.getOffsetHeight();
+        if (isEdit() && h != editAreaHeight) {
+          getCommentGroup().resize();
+          resizeTimer.scheduleRepeating(50);
+          editAreaHeight = h;
+        }
+      }
+    }, MouseMoveEvent.getType());
+
+    addDomHandler(new MouseUpHandler() {
+      @Override
+      public void onMouseUp(MouseUpEvent event) {
+        resizeTimer.cancel();
+        getCommentGroup().resize();
+      }
+    }, MouseUpEvent.getType());
+  }
+
   private boolean isNew() {
     return comment.id() == null;
   }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/DraftBox.ui.xml

@@ -49,7 +49,7 @@ limitations under the License.
         <div ui:field='date' class='{res.style.date}'/>
       </g:HTMLPanel>
       <div ui:field='p_view' aria-hidden='true' style='display: NONE'>
-        <g:HTML ui:field='message' styleName=''/>
+        <g:HTML ui:field='message' styleName='{res.style.message}'/>
         <div style='position: relative'>
           <g:Button ui:field='edit'
               title='Edit this draft comment'

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/FileCommentPanel.java

@@ -1,76 +0,0 @@
-//Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.client.diff;
-
-import com.google.gerrit.client.Gerrit;
-import com.google.gwt.user.client.ui.Composite;
-import com.google.gwt.user.client.ui.FlowPanel;
-
-import java.util.ArrayList;
-import java.util.List;
-
-/**
- * HTMLPanel to hold file comments.
- * TODO: Need to resize CodeMirror if this is resized since we don't have the
- * system scrollbar.
- */
-class FileCommentPanel extends Composite {
-  private final SideBySide2 parent;
-  private DiffTable table;
-  private DisplaySide side;
-  private List<CommentBox> boxes;
-  private FlowPanel body;
-
-  FileCommentPanel(SideBySide2 host, DiffTable table, DisplaySide side) {
-    this.parent = host;
-    this.table = table;
-    this.side = side;
-    boxes = new ArrayList<CommentBox>();
-    initWidget(body = new FlowPanel());
-  }
-
-  void createOrEditFileComment() {
-    if (!Gerrit.isSignedIn()) {
-      Gerrit.doSignIn(parent.getToken());
-      return;
-    }
-
-    if (boxes.isEmpty()) {
-      addFileComment(parent.getCommentManager().newFileDraft(side));
-    } else {
-      CommentBox box = boxes.get(boxes.size() - 1);
-      if (box instanceof DraftBox) {
-        ((DraftBox) box).setEdit(true);
-      } else {
-        addFileComment(((PublishedBox) box).addReplyBox());
-      }
-    }
-  }
-
-  int getBoxCount() {
-    return boxes.size();
-  }
-
-  void addFileComment(CommentBox box) {
-    boxes.add(box);
-    body.add(box);
-    table.setHeaderVisible(true);
-  }
-
-  void onRemoveDraftBox(DraftBox box) {
-    boxes.remove(box);
-    table.setHeaderVisible(true);
-  }
-}

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PaddingManager.java

@@ -1,170 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.client.diff;
-
-import com.google.gwt.dom.client.Element;
-import com.google.gwt.dom.client.Style.Unit;
-
-import net.codemirror.lib.LineWidget;
-
-import java.util.ArrayList;
-import java.util.List;
-
-/**
- * Manages paddings for CommentBoxes. Each line that may need to be padded owns
- * a PaddingManager instance, which maintains a padding widget whose height
- * changes as necessary. PaddingManager calculates padding by taking the
- * difference of the sum of CommentBox heights on the two sides.
- *
- * Note that in the case of an insertion or deletion gap, A PaddingManager
- * can map to a list of managers on the other side. The padding needed is then
- * calculated from the sum of all their heights.
- *
- * TODO: Let PaddingManager also take care of the paddings introduced by
- * insertions and deletions.
- */
-class PaddingManager {
-  private List<CommentBox> comments;
-  private PaddingWidgetWrapper wrapper;
-  private List<PaddingManager> others;
-
-  PaddingManager(PaddingWidgetWrapper padding) {
-    comments = new ArrayList<CommentBox>();
-    others = new ArrayList<PaddingManager>();
-    this.wrapper = padding;
-  }
-
-  static void link(PaddingManager a, PaddingManager b) {
-    if (!a.others.contains(b)) {
-      a.others.add(b);
-    }
-    if (!b.others.contains(a)) {
-      b.others.add(a);
-    }
-  }
-
-  private int getMyTotalHeight() {
-    int total = 0;
-    for (CommentBox box : comments) {
-      /**
-       * This gets the height of CM's line widget div, taking the margin and
-       * the horizontal scrollbar into account.
-       */
-      total += box.getSelfWidgetWrapper().getElement().getParentElement().getOffsetHeight();
-    }
-    return total;
-  }
-
-  /**
-   * If this instance is on the insertion side, its counterpart on the other
-   * side will map to a group of PaddingManagers on this side, so we calculate
-   * the group's total height instead of an individual one's.
-   */
-  private int getGroupTotalHeight() {
-    if (others.size() > 1) {
-      return getMyTotalHeight();
-    } else {
-      return others.get(0).getOthersTotalHeight();
-    }
-  }
-
-  private int getOthersTotalHeight() {
-    int total = 0;
-    for (PaddingManager manager : others) {
-      total += manager.getMyTotalHeight();
-    }
-    return total;
-  }
-
-  private void setPaddingHeight(int height) {
-    wrapper.element.getStyle().setHeight((double) height, Unit.PX);
-    wrapper.widget.changed();
-  }
-
-  void resizePaddingWidget() {
-    if (others.isEmpty()) {
-      return;
-    }
-    int myHeight = getGroupTotalHeight();
-    int othersHeight = getOthersTotalHeight();
-    int paddingNeeded = othersHeight - myHeight;
-    if (paddingNeeded < 0) {
-      for (PaddingManager manager : others.get(0).others) {
-        manager.setPaddingHeight(0);
-      }
-      others.get(others.size() - 1).setPaddingHeight(-paddingNeeded);
-    } else {
-      setPaddingHeight(paddingNeeded);
-      for (PaddingManager other : others) {
-        other.setPaddingHeight(0);
-      }
-    }
-  }
-
-  /** This is unused now because threading info is ignored. */
-  int getReplyIndex(CommentBox box) {
-    return comments.indexOf(box) + 1;
-  }
-
-  int getCurrentCount() {
-    return comments.size();
-  }
-
-  void insert(CommentBox box, int index) {
-    comments.add(index, box);
-  }
-
-  void remove(CommentBox box) {
-    comments.remove(box);
-  }
-
-  static class PaddingWidgetWrapper {
-    private LineWidget widget;
-    private Element element;
-
-    PaddingWidgetWrapper(LineWidget w, Element e) {
-      widget = w;
-      element = e;
-    }
-
-    LineWidget getWidget() {
-      return widget;
-    }
-
-    Element getElement() {
-      return element;
-    }
-  }
-
-  static class LinePaddingWidgetWrapper extends PaddingWidgetWrapper {
-    private int chunkLength;
-    private int otherLine;
-
-    LinePaddingWidgetWrapper(PaddingWidgetWrapper pair, int otherLine, int chunkLength) {
-      super(pair.widget, pair.element);
-
-      this.otherLine = otherLine;
-      this.chunkLength = chunkLength;
-    }
-
-    int getChunkLength() {
-      return chunkLength;
-    }
-
-    int getOtherLine() {
-      return otherLine;
-    }
-  }
-}

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PatchSetSelectBox2.java

@@ -49,7 +49,7 @@ class PatchSetSelectBox2 extends Composite {
   @UiField HTMLPanel linkPanel;
   @UiField BoxStyle style;
 
-  private DiffTable table;
+  private SideBySide2 parent;
   private DisplaySide side;
   private boolean sideA;
   private String path;
@@ -58,12 +58,16 @@ class PatchSetSelectBox2 extends Composite {
   private PatchSet.Id idActive;
   private PatchSetSelectBox2 other;
 
-  PatchSetSelectBox2(DiffTable table, final DisplaySide side,
+  PatchSetSelectBox2(SideBySide2 parent,
-      final Change.Id changeId, final PatchSet.Id revision, String path) {
+      DisplaySide side,
+      Change.Id changeId,
+      PatchSet.Id revision,
+      String path) {
     initWidget(uiBinder.createAndBindUi(this));
     icon.setTitle(PatchUtil.C.addFileCommentToolTip());
     icon.addStyleName(Gerrit.RESOURCES.css().link());
-    this.table = table;
+
+    this.parent = parent;
     this.side = side;
     this.sideA = side == DisplaySide.A;
     this.changeId = changeId;
@@ -127,6 +131,7 @@ class PatchSetSelectBox2 extends Composite {
 
   @UiHandler("icon")
   void onIconClick(ClickEvent e) {
-    table.createOrEditFileComment(side);
+    parent.getCmFromSide(side).scrollToY(0);
+    parent.getCommentManager().insertNewDraft(side, 0);
   }
 }

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PublishedBox.java

@@ -19,7 +19,6 @@ import com.google.gerrit.client.FormatUtil;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.changes.CommentInput;
 import com.google.gerrit.client.changes.Util;
 import com.google.gerrit.client.patches.PatchUtil;
 import com.google.gerrit.client.rpc.GerritCallback;
@@ -39,8 +38,6 @@ import com.google.gwt.user.client.ui.UIObject;
 import com.google.gwt.user.client.ui.Widget;
 import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
 
-import net.codemirror.lib.CodeMirror;
-
 /** An HtmlPanel for displaying a published comment */
 class PublishedBox extends CommentBox {
   interface Binder extends UiBinder<HTMLPanel, PublishedBox> {}
@@ -68,12 +65,11 @@ class PublishedBox extends CommentBox {
   AvatarImage avatar;
 
   PublishedBox(
-      CommentManager manager,
+      CommentGroup group,
-      CodeMirror cm,
       CommentLinkProcessor clp,
       PatchSet.Id psId,
       CommentInfo info) {
-    super(manager, cm, info);
+    super(group, info.range());
 
     this.psId = psId;
     this.comment = info;
@@ -125,9 +121,9 @@ class PublishedBox extends CommentBox {
     super.setOpen(open);
   }
 
-  void registerReplyBox(DraftBox box) {
+  void setReplyBox(DraftBox box) {
     replyBox = box;
-    box.registerReplyToBox(this);
+    box.setReplyToBox(this);
   }
 
   void unregisterReplyBox() {
@@ -139,21 +135,17 @@ class PublishedBox extends CommentBox {
     replyBox.setEdit(true);
   }
 
-  DraftBox addReplyBox() {
+  void addReplyBox() {
-    DraftBox box = getCommentManager().addDraftBox(
+    getCommentManager().addDraftBox(
-        getCommentManager().createReply(comment), getCm().side());
+      getCm().side(),
-    registerReplyBox(box);
+      CommentInfo.createReply(comment)).setEdit(true);
-    return box;
   }
 
   void doReply() {
     if (!Gerrit.isSignedIn()) {
       Gerrit.doSignIn(getCommentManager().getSideBySide2().getToken());
     } else if (replyBox == null) {
-      DraftBox box = addReplyBox();
+      addReplyBox();
-      if (!getCommentInfo().has_line()) {
-        getCommentManager().addFileCommentBox(box);
-      }
     } else {
       openReplyBox();
     }
@@ -172,19 +164,15 @@ class PublishedBox extends CommentBox {
       Gerrit.doSignIn(getCommentManager().getSideBySide2().getToken());
     } else if (replyBox == null) {
       done.setEnabled(false);
-      CommentInput input = CommentInput.create(getCommentManager().createReply(comment));
+      CommentInfo input = CommentInfo.createReply(comment);
-      input.setMessage(PatchUtil.C.cannedReplyDone());
+      input.message(PatchUtil.C.cannedReplyDone());
       CommentApi.createDraft(psId, input,
           new GerritCallback<CommentInfo>() {
             @Override
             public void onSuccess(CommentInfo result) {
               done.setEnabled(true);
               setOpen(false);
-              DraftBox box = getCommentManager().addDraftBox(result, getCm().side());
+              getCommentManager().addDraftBox(getCm().side(), result);
-              registerReplyBox(box);
-              if (!getCommentInfo().has_line()) {
-                getCommentManager().addFileCommentBox(box);
-              }
             }
           });
     } else {

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/PublishedBox.ui.xml

@@ -53,7 +53,8 @@ limitations under the License.
         <div ui:field='summary' class='{res.style.summary}'/>
         <div ui:field='date' class='{res.style.date}'/>
       </g:HTMLPanel>
-      <div ui:field='message' aria-hidden='true' style='display: NONE'/>
+      <div ui:field='message' class='{res.style.message}'
+           aria-hidden='true' style='display: NONE'/>
       <div ui:field='buttons' aria-hidden='true' style='display: NONE'>
         <g:Button ui:field='reply' styleName=''
             title='Reply to this comment'>

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/Resources.java

@@ -30,8 +30,10 @@ interface Resources extends ClientBundle {
   @Source("gear.png") ImageResource gear();
 
   interface Style extends CssResource {
+    String commentWidgets();
     String commentBox();
     String contents();
+    String message();
     String header();
     String summary();
     String date();

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySide2.java

@@ -22,7 +22,6 @@ import com.google.gerrit.client.changes.ChangeInfo;
 import com.google.gerrit.client.changes.ChangeInfo.RevisionInfo;
 import com.google.gerrit.client.changes.ChangeList;
 import com.google.gerrit.client.diff.LineMapper.LineOnOtherInfo;
-import com.google.gerrit.client.diff.PaddingManager.PaddingWidgetWrapper;
 import com.google.gerrit.client.patches.PatchUtil;
 import com.google.gerrit.client.projects.ConfigInfoCache;
 import com.google.gerrit.client.rpc.CallbackGroup;
@@ -56,8 +55,6 @@ import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.rpc.AsyncCallback;
 import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.Image;
-import com.google.gwt.user.client.ui.SimplePanel;
-import com.google.gwt.user.client.ui.Widget;
 import com.google.gwtexpui.globalkey.client.GlobalKey;
 import com.google.gwtexpui.globalkey.client.KeyCommand;
 import com.google.gwtexpui.globalkey.client.KeyCommandSet;
@@ -68,12 +65,10 @@ import net.codemirror.lib.CodeMirror.BeforeSelectionChangeHandler;
 import net.codemirror.lib.CodeMirror.GutterClickHandler;
 import net.codemirror.lib.CodeMirror.LineClassWhere;
 import net.codemirror.lib.CodeMirror.LineHandle;
-import net.codemirror.lib.CodeMirror.RenderLineHandler;
 import net.codemirror.lib.CodeMirror.Viewport;
 import net.codemirror.lib.Configuration;
 import net.codemirror.lib.KeyMap;
 import net.codemirror.lib.LineCharacter;
-import net.codemirror.lib.LineWidget;
 import net.codemirror.lib.ModeInjector;
 import net.codemirror.lib.Rect;
 
@@ -112,7 +107,6 @@ public class SideBySide2 extends Screen {
   private KeyCommandSet keysAction;
   private KeyCommandSet keysComment;
   private List<HandlerRegistration> handlers;
-  private List<Runnable> deferred;
   private PreferencesAction prefsAction;
   private int reloadVersionId;
 
@@ -294,7 +288,6 @@ public class SideBySide2 extends Screen {
     cm.on("beforeSelectionChange", onSelectionChange(cm));
     cm.on("cursorActivity", updateActiveLine(cm));
     cm.on("gutterClick", onGutterClick(cm));
-    cm.on("renderLine", resizeLinePadding(cm.side()));
     cm.on("viewportChange", adjustGutters(cm));
     cm.on("focus", new Runnable() {
       @Override
@@ -313,7 +306,7 @@ public class SideBySide2 extends Screen {
         .on("'c'", commentManager.insertNewDraft(cm))
         .on("N", maybeNextVimSearch(cm))
         .on("P", chunkManager.diffChunkNav(cm, Direction.PREV))
-        .on("Shift-O", commentManager.openClosePublished(cm))
+        .on("Shift-O", commentManager.openCloseAll(cm))
         .on("Shift-Left", moveCursorToSide(cm, DisplaySide.A))
         .on("Shift-Right", moveCursorToSide(cm, DisplaySide.B))
         .on("'i'", new Runnable() {
@@ -486,10 +479,7 @@ public class SideBySide2 extends Screen {
         cmB.setHeight(height);
 
         render(diff);
-        commentManager.render(comments);
+        commentManager.render(comments, prefs.expandAllComments());
-        if (prefs.expandAllComments()) {
-          commentManager.setExpandAllComments(true);
-        }
         skipManager.render(prefs.context(), diff);
       }
     });
@@ -620,42 +610,6 @@ public class SideBySide2 extends Screen {
     return chunkManager.getLineMapper().lineOnOther(side, line);
   }
 
-  PaddingWidgetWrapper addPaddingWidget(CodeMirror cm,
-      int line, double height, Unit unit, Integer index) {
-    SimplePanel padding = new SimplePanel();
-    padding.setStyleName(DiffTable.style.padding());
-    padding.getElement().getStyle().setHeight(height, unit);
-    Configuration config = Configuration.create()
-        .set("coverGutter", true)
-        .set("above", line == -1)
-        .set("noHScroll", true);
-    if (index != null) {
-      config = config.set("insertAt", index);
-    }
-    LineWidget widget = addLineWidget(cm, line == -1 ? 0 : line, padding, config);
-    return new PaddingWidgetWrapper(widget, padding.getElement());
-  }
-
-  /**
-   * A LineWidget needs to be added to diffTable in order to respond to browser
-   * events, but CodeMirror doesn't render the widget until the containing line
-   * is scrolled into viewportMargin, causing it to appear at the bottom of the
-   * DOM upon loading. Fix by hiding the widget until it is first scrolled into
-   * view (when CodeMirror fires a "redraw" event on the widget).
-   */
-  LineWidget addLineWidget(CodeMirror cm, int line,
-      final Widget widget, Configuration options) {
-    widget.setVisible(false);
-    LineWidget lineWidget = cm.addLineWidget(line, widget.getElement(), options);
-    lineWidget.onFirstRedraw(new Runnable() {
-      @Override
-      public void run() {
-        widget.setVisible(true);
-      }
-    });
-    return lineWidget;
-  }
-
   private void clearActiveLine(CodeMirror cm) {
     if (cm.hasActiveLine()) {
       LineHandle activeLine = cm.getActiveLine();
@@ -684,34 +638,37 @@ public class SideBySide2 extends Screen {
     final CodeMirror other = otherCm(cm);
     return new Runnable() {
       public void run() {
-        /**
+        // The rendering of active lines has to be deferred. Reflow
-         * The rendering of active lines has to be deferred. Reflow
+        // caused by adding and removing styles chokes Firefox when arrow
-         * caused by adding and removing styles chokes Firefox when arrow
+        // key (or j/k) is held down. Performance on Chrome is fine
-         * key (or j/k) is held down. Performance on Chrome is fine
+        // without the deferral.
-         * without the deferral.
+        //
-         */
+        Scheduler.get().scheduleDeferred(new ScheduledCommand() {
-        defer(new Runnable() {
           @Override
-          public void run() {
+          public void execute() {
-            LineHandle handle = cm.getLineHandleVisualStart(
+            operation(new Runnable() {
-                cm.getCursor("end").getLine());
+              public void run() {
-            if (cm.hasActiveLine() && cm.getActiveLine().equals(handle)) {
+                LineHandle handle = cm.getLineHandleVisualStart(
-              return;
+                    cm.getCursor("end").getLine());
-            }
+                if (cm.hasActiveLine() && cm.getActiveLine().equals(handle)) {
+                  return;
+                }
 
-            clearActiveLine(cm);
+                clearActiveLine(cm);
-            clearActiveLine(other);
+                clearActiveLine(other);
-            cm.setActiveLine(handle);
+                cm.setActiveLine(handle);
-            cm.addLineClass(
+                cm.addLineClass(
-                handle, LineClassWhere.WRAP, DiffTable.style.activeLine());
+                    handle, LineClassWhere.WRAP, DiffTable.style.activeLine());
-            LineOnOtherInfo info =
+                LineOnOtherInfo info =
-                lineOnOther(cm.side(), cm.getLineNumber(handle));
+                    lineOnOther(cm.side(), cm.getLineNumber(handle));
-            if (info.isAligned()) {
+                if (info.isAligned()) {
-              LineHandle oLineHandle = other.getLineHandle(info.getLine());
+                  LineHandle oLineHandle = other.getLineHandle(info.getLine());
-              other.setActiveLine(oLineHandle);
+                  other.setActiveLine(oLineHandle);
-              other.addLineClass(oLineHandle, LineClassWhere.WRAP,
+                  other.addLineClass(oLineHandle, LineClassWhere.WRAP,
-                  DiffTable.style.activeLine());
+                      DiffTable.style.activeLine());
-            }
+                }
+              }
+            });
           }
         });
       }
@@ -798,39 +755,6 @@ public class SideBySide2 extends Screen {
     };
   }
 
-
-  void defer(Runnable thunk) {
-    if (deferred == null) {
-      final ArrayList<Runnable> list = new ArrayList<Runnable>();
-      deferred = list;
-      Scheduler.get().scheduleDeferred(new ScheduledCommand() {
-        @Override
-        public void execute() {
-          deferred = null;
-          operation(new Runnable() {
-            public void run() {
-              for (Runnable thunk : list) {
-                thunk.run();
-              }
-            }
-          });
-        }
-      });
-    }
-    deferred.add(thunk);
-  }
-
-  // TODO: Maybe integrate this with PaddingManager.
-  private RenderLineHandler resizeLinePadding(final DisplaySide side) {
-    return new RenderLineHandler() {
-      @Override
-      public void handle(CodeMirror cm, LineHandle lh, Element e) {
-        commentManager.resizePadding(lh);
-        chunkManager.resizePadding(cm, lh, side);
-      }
-    };
-  }
-
   void resizeCodeMirror() {
     int height = getCodeMirrorHeight();
     cmA.setHeight(height);
@@ -873,6 +797,10 @@ public class SideBySide2 extends Screen {
     return commentManager;
   }
 
+  SkipManager getSkipManager() {
+    return skipManager;
+  }
+
   void operation(final Runnable apply) {
     cmA.operation(new Runnable() {
       @Override

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SkipBar.java

@@ -84,13 +84,11 @@ class SkipBar extends Composite {
         lineWidget = cm.addLineWidget(start - 1, getElement(), cfg);
       }
       if (isNew) {
-        setVisible(false);
         lineWidget.onFirstRedraw(new Runnable() {
           @Override
           public void run() {
             int w = cm.getGutterElement().getOffsetWidth();
             getElement().getStyle().setPaddingLeft(w, Unit.PX);
-            setVisible(true);
           }
         });
       }
@@ -124,16 +122,21 @@ class SkipBar extends Composite {
     lineWidget.clear();
   }
 
+  void expandBefore(int cnt) {
+    expandSideBefore(cnt);
+    otherBar.expandSideBefore(cnt);
+  }
+
   void expandAll() {
     clearMarkerAndWidget();
     removeFromParent();
     updateSelection();
   }
 
-  private void expandBefore() {
+  private void expandSideBefore(int cnt) {
     FromTo range = textMarker.find();
     int oldStart = range.getFrom().getLine();
-    int newStart = oldStart + NUM_ROWS_TO_EXPAND;
+    int newStart = oldStart + cnt;
     int end = range.getTo().getLine();
     clearMarkerAndWidget();
     collapse(newStart, end, true);
@@ -167,8 +170,7 @@ class SkipBar extends Composite {
 
   @UiHandler("upArrow")
   void onExpandBefore(ClickEvent e) {
-    otherBar.expandBefore();
+    expandBefore(NUM_ROWS_TO_EXPAND);
-    expandBefore();
     cm.focus();
   }
 

gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SkipManager.java

@@ -31,6 +31,7 @@ class SkipManager {
   private final SideBySide2 host;
   private final CommentManager commentManager;
   private Set<SkipBar> skipBars;
+  private SkipBar line0;
 
   SkipManager(SideBySide2 host, CommentManager commentManager) {
     this.host = host;
@@ -82,6 +83,7 @@ class SkipManager {
         if (skip.getStartA() == 0 || skip.getStartB() == 0) {
           barA.upArrow.setVisible(false);
           barB.upArrow.setVisible(false);
+          line0 = barB;
         } else if (skip.getStartA() + skip.getSize() == lineA
             || skip.getStartB() + skip.getSize() == lineB) {
           barA.downArrow.setVisible(false);
@@ -91,18 +93,29 @@ class SkipManager {
     }
   }
 
+  void ensureFirstLineIsVisible() {
+    if (line0 != null) {
+      line0.expandBefore(1);
+      line0 = null;
+    }
+  }
+
   void removeAll() {
     if (skipBars != null) {
       for (SkipBar bar : skipBars) {
         bar.expandAll();
       }
       skipBars = null;
+      line0 = null;
     }
   }
 
   void remove(SkipBar a, SkipBar b) {
     skipBars.remove(a);
     skipBars.remove(b);
+    if (line0 == a || line0 == b) {
+      line0 = null;
+    }
     if (skipBars.isEmpty()) {
       skipBars = null;
     }

gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/AbstractPatchContentTable.java

@@ -20,7 +20,6 @@ import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.account.AccountInfo;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.changes.CommentInput;
 import com.google.gerrit.client.changes.PatchTable;
 import com.google.gerrit.client.changes.Util;
 import com.google.gerrit.client.rpc.GerritCallback;
@@ -941,7 +940,7 @@ public abstract class AbstractPatchContentTable extends NavigationTable<Object>
       if (p == null) {
         enableButtons(false);
         final PatchSet.Id psId = newComment.getKey().getParentKey().getParentKey();
-        CommentInput in = CommentEditorPanel.toInput(newComment);
+        CommentInfo in = CommentEditorPanel.toInput(newComment);
         CommentApi.createDraft(psId, in,
             new GerritCallback<CommentInfo>() {
               @Override

gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/CommentEditorPanel.java

@@ -17,7 +17,6 @@ package com.google.gerrit.client.patches;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.changes.CommentApi;
 import com.google.gerrit.client.changes.CommentInfo;
-import com.google.gerrit.client.changes.CommentInput;
 import com.google.gerrit.client.rpc.GerritCallback;
 import com.google.gerrit.client.ui.CommentLinkProcessor;
 import com.google.gerrit.client.ui.CommentPanel;
@@ -268,7 +267,7 @@ public class CommentEditorPanel extends CommentPanel implements ClickHandler,
         onSave.onFailure(caught);
       }
     };
-    CommentInput input = toInput(comment);
+    CommentInfo input = toInput(comment);
     if (wasNew) {
       CommentApi.createDraft(psId, input, cb);
     } else {
@@ -336,16 +335,16 @@ public class CommentEditorPanel extends CommentPanel implements ClickHandler,
     return null;
   }
 
-  public static CommentInput toInput(PatchLineComment c) {
+  public static CommentInfo toInput(PatchLineComment c) {
-    CommentInput i = CommentInput.createObject().cast();
+    CommentInfo i = CommentInfo.createObject().cast();
-    i.setId(c.getKey().get());
+    i.id(c.getKey().get());
-    i.setPath(c.getKey().getParentKey().get());
+    i.path(c.getKey().getParentKey().get());
-    i.setSide(c.getSide() == 0 ? Side.PARENT : Side.REVISION);
+    i.side(c.getSide() == 0 ? Side.PARENT : Side.REVISION);
     if (c.getLine() > 0) {
-      i.setLine(c.getLine());
+      i.line(c.getLine());
     }
-    i.setInReplyTo(c.getParentUuid());
+    i.in_reply_to(c.getParentUuid());
-    i.setMessage(c.getMessage());
+    i.message(c.getMessage());
     return i;
   }
 

gerrit-gwtui/src/main/java/net/codemirror/lib/CodeMirror.java

@@ -327,6 +327,9 @@ public class CodeMirror extends JavaScriptObject {
   public static class Viewport extends JavaScriptObject {
     public final native int getFrom() /*-{ return this.from; }-*/;
     public final native int getTo() /*-{ return this.to; }-*/;
+    public final boolean contains(int line) {
+      return getFrom() <= line && line < getTo();
+    }
 
     protected Viewport() {
     }