Add POST /changes/{id}/revisions/{sha1}/review

Clients can post a code review scoring and comments using a JSON
REST-style API. The API accepts change specifications using either the
triplet "project~branch~change_id" or the legacy _number format, and
also accepts commit SHA-1 or legacy patch set id to identify which
patch set the comments apply too. Clients should prefer to use commit
SHA-1 to write comments, especially from automated builders where the
commit was just checked out.

Comments may include a top level cover message, updated scores for
labels (previously approval categories), and inline file comments:

  POST /changes/gerrit~master~I9589cc46b.../revisions/fe7ffc.../review
  Content-Type: application/json;charset=utf-8

  {
    "message": "Thank you for starting this project, it is useful.",

    "labels": {
      "Verified": 1,
      "Code-Review": -1
    },

    "comments": {
      "gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java": [
        { "line": 1, "message": "Is the copyright notice date correct?" },
        { "line": 32, "message": "Great idea, but try ..."}
      ]
    }
  }

Labels are merged with the already existing labels for the calling
user. This allows updating independent categories from unrelated
processes, such as two different build systems testing on Linux and
Mac OS X. Callers can erase a label by setting it to 0.

Label names are free-form in the JSON map, but the server still needs
to restrict labels to those declared in the approval_categories table
as it needs a category_id value to link the score to the patch set in
the database. This will change in the future as we fix more of the UI
and server code to not have this requirement.

By default the POST is rejected with a 403 Forbidden error if a label
is used in a way that is not permitted by the caller, e.g. trying to
assign Code-Review+2 will fail unless the caller has that permission.
Clients can request these permission errors to be ignored by setting
the top level property "strict_labels": false in the request:

    "strict_labels": false,
	"labels": { "Code-Review": 2 },

If a label is used without permission the value will be squashed to
the highest (or lowest) value permitted to be used by the caller. If
this value is 0, the label won't be set.

Inline file comments can also be supplied using the comments map. Keys
are file names, with lists of comment objects holding line number and
message text. All comments must be supplied during the POST. If the
user has any drafts these will be erased when the comments are stored
and published. In a future commit the web UI will be rewritten to use
this RPC and will include all draft comments anytime it publishes.
This will simplify last-minute edits of inline comments, as they can
be updated without needing to wait for drafts to save.

Drafts may optionally be published or left in draft status by setting
the drafts field to another value:

  "drafts": DELETE,   // Delete non-updated drafts (default behavior).
  "drafts": PUBLISH,  // Publish non-updated drafts now.
  "drafts": KEEP,     // Leave non-updated draft in draft status.

This commit copies a lot of code from PublishComments and refactors it
to a cleaner implementation. As soon as the web UI switches to the new
/review REST API call, PublishComments will be deleted, so no attempt
is made at sharing a common implementation.

Change-Id: Iee7b4dcaa28cfc83f585ff99e3ed705973a2788d

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java

@@ -174,6 +174,10 @@ public final class PatchLineComment {
     writtenOn = new Timestamp(System.currentTimeMillis());
   }
 
+  public void setWrittenOn(Timestamp ts) {
+    writtenOn = ts;
+  }
+
   public String getParentUuid() {
     return parentUuid;
   }

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java

@@ -146,6 +146,10 @@ public final class PatchSetApproval {
     granted = new Timestamp(System.currentTimeMillis());
   }
 
+  public void setGranted(Timestamp ts) {
+    granted = ts;
+  }
+
   public void cache(final Change c) {
     changeOpen = c.open;
     changeSortKey = c.sortKey;

gerrit-server/src/main/java/com/google/gerrit/server/change/EmailReviewComments.java

@@ -0,0 +1,141 @@
+// Copyright (C) 2012 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.server.change;
+
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.mail.CommentSender;
+import com.google.gerrit.server.patch.PatchSetInfoFactory;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
+import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.SchemaFactory;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.ProvisionException;
+import com.google.inject.assistedinject.Assisted;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.List;
+
+class EmailReviewComments implements Runnable, RequestContext {
+  private static final Logger log = LoggerFactory.getLogger(EmailReviewComments.class);
+
+  interface Factory {
+    EmailReviewComments create(
+        Change change,
+        PatchSet patchSet,
+        Account.Id authorId,
+        ChangeMessage message,
+        List<PatchLineComment> comments);
+  }
+
+  private final WorkQueue workQueue;
+  private final PatchSetInfoFactory patchSetInfoFactory;
+  private final CommentSender.Factory commentSenderFactory;
+  private final SchemaFactory<ReviewDb> schemaFactory;
+  private final ThreadLocalRequestContext requestContext;
+
+  private final Change change;
+  private final PatchSet patchSet;
+  private final Account.Id authorId;
+  private final ChangeMessage message;
+  private final List<PatchLineComment> comments;
+  private ReviewDb db;
+
+  @Inject
+  EmailReviewComments (
+      WorkQueue workQueue,
+      PatchSetInfoFactory patchSetInfoFactory,
+      CommentSender.Factory commentSenderFactory,
+      SchemaFactory<ReviewDb> schemaFactory,
+      ThreadLocalRequestContext requestContext,
+      @Assisted Change change,
+      @Assisted PatchSet patchSet,
+      @Assisted Account.Id authorId,
+      @Assisted ChangeMessage message,
+      @Assisted List<PatchLineComment> comments) {
+    this.workQueue = workQueue;
+    this.patchSetInfoFactory = patchSetInfoFactory;
+    this.commentSenderFactory = commentSenderFactory;
+    this.schemaFactory = schemaFactory;
+    this.requestContext = requestContext;
+    this.change = change;
+    this.patchSet = patchSet;
+    this.authorId = authorId;
+    this.message = message;
+    this.comments = comments;
+  }
+
+  void sendAsync() {
+    workQueue.getDefaultQueue().submit(this);
+  }
+
+  @Override
+  public void run() {
+    try {
+      requestContext.setContext(this);
+      CommentSender cm = commentSenderFactory.create(change);
+      cm.setFrom(authorId);
+      cm.setPatchSet(patchSet, patchSetInfoFactory.get(change, patchSet));
+      cm.setChangeMessage(message);
+      cm.setPatchLineComments(comments);
+      cm.send();
+    } catch (Exception e) {
+      log.error("Cannot email comments for " + patchSet.getId(), e);
+    } finally {
+      requestContext.setContext(null);
+      if (db != null) {
+        db.close();
+        db = null;
+      }
+    }
+  }
+
+  @Override
+  public String toString() {
+    return "send-email comments";
+  }
+
+  @Override
+  public CurrentUser getCurrentUser() {
+    return null;
+  }
+
+  @Override
+  public Provider<ReviewDb> getReviewDbProvider() {
+    return new Provider<ReviewDb>() {
+      @Override
+      public ReviewDb get() {
+        if (db == null) {
+          try {
+            db = schemaFactory.open();
+          } catch (OrmException e) {
+            throw new ProvisionException("Cannot open ReviewDb", e);
+          }
+        }
+        return db;
+      }
+    };
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -16,20 +16,33 @@ package com.google.gerrit.server.change;
 
 import static com.google.gerrit.server.change.ChangeResource.CHANGE_KIND;
 import static com.google.gerrit.server.change.ReviewerResource.REVIEWER_KIND;
+import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND;
 
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.RestApiModule;
+import com.google.gerrit.server.config.FactoryModule;
 
 public class Module extends RestApiModule {
   @Override
   protected void configure() {
     DynamicMap.mapOf(binder(), CHANGE_KIND);
     DynamicMap.mapOf(binder(), REVIEWER_KIND);
+    DynamicMap.mapOf(binder(), REVISION_KIND);
 
     get(CHANGE_KIND).to(GetChange.class);
     post(CHANGE_KIND, "abandon").to(Abandon.class);
     child(CHANGE_KIND, "reviewers").to(Reviewers.class);
 
     get(REVIEWER_KIND).to(GetReviewer.class);
+
+    child(CHANGE_KIND, "revisions").to(Revisions.class);
+    post(REVISION_KIND, "review").to(PostReview.class);
+
+    install(new FactoryModule() {
+      @Override
+      protected void configure() {
+        factory(EmailReviewComments.Factory.class);
+      }
+    });
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -0,0 +1,437 @@
+// Copyright (C) 2012 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.server.change;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Strings;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.gerrit.common.ChangeHooks;
+import com.google.gerrit.common.data.ApprovalType;
+import com.google.gerrit.common.data.ApprovalTypes;
+import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.common.data.PermissionRange;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.DefaultInput;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.ApprovalCategory;
+import com.google.gerrit.reviewdb.client.ApprovalCategoryValue;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.ChangeMessage;
+import com.google.gerrit.reviewdb.client.Patch;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.change.PostReview.Input;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.sql.Timestamp;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+
+class PostReview implements RestModifyView<RevisionResource, Input> {
+  private static final Logger log = LoggerFactory.getLogger(PostReview.class);
+
+  static class Input {
+    @DefaultInput
+    String message;
+
+    Map<String, Short> labels;
+    Map<String, List<Comment>> comments;
+
+    /**
+     * If true require all labels to be within the user's permitted ranges based
+     * on access controls, attempting to use a label not granted to the user
+     * will fail the entire modify operation early. If false the operation will
+     * execute anyway, but the proposed labels given by the user will be
+     * modified to be the "best" value allowed by the access controls.
+     */
+    boolean strictLabels = true;
+
+    /**
+     * How to process draft comments already in the database that were not also
+     * described in this input request.
+     */
+    DraftHandling drafts = DraftHandling.DELETE;
+  }
+
+  static enum DraftHandling {
+    DELETE, PUBLISH, KEEP;
+  }
+
+  static enum Side {
+    PARENT, REVISION;
+  }
+
+  static class Comment {
+    String id;
+    Side side;
+    int line;
+    String message;
+  }
+
+  private final ReviewDb db;
+  private final ApprovalTypes approvalTypes;
+  private final EmailReviewComments.Factory email;
+  @Deprecated private final ChangeHooks hooks;
+
+  private Change change;
+  private ChangeMessage message;
+  private Timestamp timestamp;
+  private List<PatchLineComment> comments = Lists.newArrayList();
+  private List<String> labelDelta = Lists.newArrayList();
+  @Deprecated private Map<ApprovalCategory.Id, ApprovalCategoryValue.Id> categories
+    = Maps.newHashMap();
+
+  @Inject
+  PostReview(ReviewDb db,
+      ApprovalTypes approvalTypes,
+      EmailReviewComments.Factory email,
+      ChangeHooks hooks) {
+    this.db = db;
+    this.approvalTypes = approvalTypes;
+    this.email = email;
+    this.hooks = hooks;
+  }
+
+  @Override
+  public Class<Input> inputType() {
+    return Input.class;
+  }
+
+  @Override
+  public Object apply(RevisionResource revision, Input input)
+      throws AuthException, BadRequestException, ResourceConflictException,
+      Exception {
+    if (input.labels != null) {
+      checkLabels(revision, input.strictLabels, input.labels);
+    }
+    if (input.comments != null) {
+      checkComments(input.comments);
+    }
+
+    db.changes().beginTransaction(revision.getChange().getId());
+    try {
+      change = db.changes().get(revision.getChange().getId());
+      ChangeUtil.updated(change);
+      timestamp = change.getLastUpdatedOn();
+
+      if (input.comments != null) {
+        insertComments(revision, input.comments, input.drafts);
+      }
+      if (change.getStatus().isOpen() && input.labels != null) {
+        // TODO Allow updating some labels even when closed.
+        updateLabels(revision, input.labels);
+      }
+
+      insertMessage(revision, input.message);
+      db.changes().update(Collections.singleton(change));
+      db.commit();
+    } finally {
+      db.rollback();
+    }
+
+    email.create(
+        change,
+        revision.getPatchSet(),
+        revision.getAuthorId(),
+        message,
+        comments).sendAsync();
+    fireCommentAddedHook(revision);
+    return input;
+  }
+
+  private void checkLabels(RevisionResource revision, boolean strict,
+      Map<String, Short> labels) throws BadRequestException, AuthException {
+    ChangeControl ctl = revision.getControl();
+    Iterator<Map.Entry<String, Short>> itr = labels.entrySet().iterator();
+    while (itr.hasNext()) {
+      Map.Entry<String, Short> ent = itr.next();
+
+      // TODO Support more generic label assignments.
+      ApprovalType at = approvalTypes.byLabel(ent.getKey());
+      if (at == null) {
+        if (strict) {
+          throw new BadRequestException(String.format(
+              "label \"%s\" is not a configured ApprovalCategory",
+              ent.getKey()));
+        } else {
+          itr.remove();
+          continue;
+        }
+      }
+
+      if (ent.getValue() == null || ent.getValue() == 0) {
+        // Always permit 0, even if it is not within range.
+        // Later null/0 will be deleted and revoke the label.
+        continue;
+      }
+
+      if (!at.getValuesAsList().contains(ent.getValue())) {
+        if (strict) {
+          throw new BadRequestException(String.format(
+              "label \"%s\": %d is not a valid value",
+              ent.getKey(), ent.getValue()));
+        } else {
+          itr.remove();
+          continue;
+        }
+      }
+
+      String name = at.getCategory().getLabelName();
+      PermissionRange range = ctl.getRange(Permission.forLabel(name));
+      if (range == null || !range.contains(ent.getValue())) {
+        if (strict) {
+          throw new AuthException(String.format(
+              "Applying label \"%s\": %d is restricted",
+              ent.getKey(), ent.getValue()));
+        } else if (range == null || range.isEmpty()) {
+          ent.setValue((short) 0);
+        } else {
+          ent.setValue((short) range.squash(ent.getValue()));
+        }
+      }
+    }
+  }
+
+  private void checkComments(Map<String, List<Comment>> in)
+      throws BadRequestException {
+    Iterator<Map.Entry<String, List<Comment>>> mapItr =
+        in.entrySet().iterator();
+    while (mapItr.hasNext()) {
+      Map.Entry<String, List<Comment>> ent = mapItr.next();
+      String path = ent.getKey();
+      List<Comment> list = ent.getValue();
+      if (list == null) {
+        mapItr.remove();
+        continue;
+      }
+
+      Iterator<Comment> listItr = list.iterator();
+      while (listItr.hasNext()) {
+        Comment c = listItr.next();
+        if (c.line < 0) {
+          throw new BadRequestException(String.format(
+              "negative line number %d not allowed on %s",
+              c.line, path));
+        }
+        c.message = Strings.emptyToNull(c.message).trim();
+        if (c.message.isEmpty()) {
+          listItr.remove();
+        }
+      }
+      if (list.isEmpty()) {
+        mapItr.remove();
+      }
+    }
+  }
+
+  private void insertComments(RevisionResource rsrc,
+      Map<String, List<Comment>> in, DraftHandling draftsHandling)
+      throws OrmException {
+    Map<String, PatchLineComment> drafts = scanDraftComments(rsrc);
+    List<PatchLineComment> del = Lists.newArrayList();
+    List<PatchLineComment> ins = Lists.newArrayList();
+    List<PatchLineComment> upd = Lists.newArrayList();
+
+    for (Map.Entry<String, List<Comment>> ent : in.entrySet()) {
+      String path = ent.getKey();
+      for (Comment c : ent.getValue()) {
+        PatchLineComment e = drafts.remove(c.id);
+        boolean create = e == null;
+        if (create) {
+          e = new PatchLineComment(
+              new PatchLineComment.Key(
+                  new Patch.Key(rsrc.getPatchSet().getId(), path),
+                  ChangeUtil.messageUUID(db)),
+              c.line,
+              rsrc.getAuthorId(),
+              null);
+        }
+        e.setStatus(PatchLineComment.Status.PUBLISHED);
+        e.setWrittenOn(timestamp);
+        e.setSide(c.side == Side.PARENT ? (short) 0 : (short) 1);
+        e.setMessage(c.message);
+        (create ? ins : upd).add(e);
+      }
+    }
+
+    switch (Objects.firstNonNull(draftsHandling, DraftHandling.DELETE)) {
+      case KEEP:
+      default:
+        break;
+      case DELETE:
+        del.addAll(drafts.values());
+        break;
+      case PUBLISH:
+        for (PatchLineComment e : drafts.values()) {
+          e.setStatus(PatchLineComment.Status.PUBLISHED);
+          e.setWrittenOn(timestamp);
+          upd.add(e);
+        }
+        break;
+    }
+    db.patchComments().delete(del);
+    db.patchComments().insert(ins);
+    db.patchComments().update(upd);
+    comments.addAll(ins);
+    comments.addAll(upd);
+  }
+
+  private Map<String, PatchLineComment> scanDraftComments(
+      RevisionResource rsrc) throws OrmException {
+    Map<String, PatchLineComment> drafts = Maps.newHashMap();
+    for (PatchLineComment c : db.patchComments().draftByPatchSetAuthor(
+          rsrc.getPatchSet().getId(),
+          rsrc.getAuthorId())) {
+      drafts.put(c.getKey().get(), c);
+    }
+    return drafts;
+  }
+
+  private void updateLabels(RevisionResource rsrc, Map<String, Short> labels)
+      throws OrmException {
+    List<PatchSetApproval> del = Lists.newArrayList();
+    List<PatchSetApproval> ins = Lists.newArrayList();
+    List<PatchSetApproval> upd = Lists.newArrayList();
+    Map<String, PatchSetApproval> current = scanLabels(rsrc, del);
+    for (Map.Entry<String, Short> ent : labels.entrySet()) {
+      // TODO Support arbitrary label names.
+      ApprovalType at = approvalTypes.byLabel(ent.getKey());
+      String name = at.getCategory().getLabelName();
+      PatchSetApproval c = current.get(name);
+
+      if (ent.getValue() == null || ent.getValue() == 0) {
+        // User requested delete of this label.
+        if (c != null) {
+          del.add(c);
+          labelDelta.add("-" + name);
+        }
+        continue;
+      }
+
+      if (c != null && c.getValue() != ent.getValue()) {
+        c.setValue(ent.getValue());
+        c.setGranted(timestamp);
+        c.cache(change);
+        upd.add(c);
+        labelDelta.add(format(name, c.getValue()));
+        categories.put(
+            at.getCategory().getId(),
+            at.getValue(c.getValue()).getId());
+      } else if (c == null) {
+        c = new PatchSetApproval(new PatchSetApproval.Key(
+                rsrc.getPatchSet().getId(),
+                rsrc.getAuthorId(),
+                at.getCategory().getId()),
+            ent.getValue());
+        c.setGranted(timestamp);
+        c.cache(change);
+        ins.add(c);
+        labelDelta.add(format(name, c.getValue()));
+        categories.put(
+            at.getCategory().getId(),
+            at.getValue(c.getValue()).getId());
+      }
+    }
+
+    db.patchSetApprovals().delete(del);
+    db.patchSetApprovals().insert(ins);
+    db.patchSetApprovals().update(upd);
+  }
+
+  private Map<String, PatchSetApproval> scanLabels(RevisionResource rsrc,
+      List<PatchSetApproval> del) throws OrmException {
+    Map<String, PatchSetApproval> current = Maps.newHashMap();
+    for (PatchSetApproval a : db.patchSetApprovals().byPatchSetUser(
+          rsrc.getPatchSet().getId(), rsrc.getAuthorId())) {
+      if (ApprovalCategory.SUBMIT.equals(a.getCategoryId())) {
+        continue;
+      }
+
+      ApprovalType at = approvalTypes.byId(a.getCategoryId());
+      if (at != null) {
+        current.put(at.getCategory().getLabelName(), a);
+      } else {
+        del.add(a);
+      }
+    }
+    return current;
+  }
+
+  private static String format(String name, short value) {
+    StringBuilder sb = new StringBuilder(name.length() + 2);
+    sb.append(name);
+    if (value >= 0) {
+      sb.append('+');
+    }
+    sb.append(value);
+    return sb.toString();
+  }
+
+  private void insertMessage(RevisionResource rsrc, String msg)
+      throws OrmException {
+    msg = Strings.nullToEmpty(msg).trim();
+
+    StringBuilder buf = new StringBuilder();
+    buf.append(String.format(
+        "Patch Set %d:",
+        rsrc.getPatchSet().getPatchSetId()));
+    for (String d : labelDelta) {
+      buf.append(" ").append(d);
+    }
+    if (comments.size() == 1) {
+      buf.append("\n\n(1 inline comment)");
+    } else if (comments.size() > 1) {
+      buf.append(String.format("\n\n(%d inline comments)", comments.size()));
+    }
+    if (!msg.isEmpty()) {
+      buf.append("\n\n").append(msg);
+    }
+
+    message = new ChangeMessage(
+        new ChangeMessage.Key(change.getId(), ChangeUtil.messageUUID(db)),
+        rsrc.getAuthorId(),
+        timestamp,
+        rsrc.getPatchSet().getId());
+    message.setMessage(buf.toString());
+    db.changeMessages().insert(Collections.singleton(message));
+  }
+
+  @Deprecated
+  private void fireCommentAddedHook(RevisionResource rsrc) throws OrmException {
+    IdentifiedUser user = (IdentifiedUser) rsrc.getControl().getCurrentUser();
+    try {
+      hooks.doCommentAddedHook(change,
+          user.getAccount(),
+          rsrc.getPatchSet(),
+          message.getMessage(),
+          categories, db);
+    } catch (OrmException e) {
+      log.warn("ChangeHook.doCommentAddedHook delivery failed", e);
+    }
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/RevisionResource.java

@@ -0,0 +1,53 @@
+// Copyright (C) 2012 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.server.change;
+
+import com.google.gerrit.extensions.restapi.RestResource;
+import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.inject.TypeLiteral;
+
+public class RevisionResource implements RestResource {
+  public static final TypeLiteral<RestView<RevisionResource>> REVISION_KIND =
+      new TypeLiteral<RestView<RevisionResource>>() {};
+
+  private final ChangeResource change;
+  private final PatchSet ps;
+
+  public RevisionResource(ChangeResource change, PatchSet ps) {
+    this.change = change;
+    this.ps = ps;
+  }
+
+  public ChangeControl getControl() {
+    return change.getControl();
+  }
+
+  public Change getChange() {
+    return getControl().getChange();
+  }
+
+  public PatchSet getPatchSet() {
+    return ps;
+  }
+
+  Account.Id getAuthorId() {
+    return ((IdentifiedUser) getControl().getCurrentUser()).getAccountId();
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java

@@ -0,0 +1,87 @@
+// Copyright (C) 2012 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.server.change;
+
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.restapi.ChildCollection;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+import java.util.List;
+
+class Revisions implements ChildCollection<ChangeResource, RevisionResource> {
+  private final DynamicMap<RestView<RevisionResource>> views;
+  private final Provider<ReviewDb> dbProvider;
+
+  @Inject
+  Revisions(DynamicMap<RestView<RevisionResource>> views,
+      Provider<ReviewDb> dbProvider) {
+    this.views = views;
+    this.dbProvider = dbProvider;
+  }
+
+  @Override
+  public DynamicMap<RestView<RevisionResource>> views() {
+    return views;
+  }
+
+  @Override
+  public RestView<ChangeResource> list() throws ResourceNotFoundException {
+    throw new ResourceNotFoundException();
+  }
+
+  @Override
+  public RevisionResource parse(ChangeResource change, String id)
+      throws ResourceNotFoundException, Exception {
+    if (id.matches("^[1-9][0-9]{0,4}$")) {
+      PatchSet ps = dbProvider.get().patchSets().get(new PatchSet.Id(
+          change.getChange().getId(),
+          Integer.parseInt(id)));
+      if (ps != null
+          && change.getControl().isPatchVisible(ps, dbProvider.get())) {
+        return new RevisionResource(change, ps);
+      }
+      throw new ResourceNotFoundException(id);
+    }
+
+    for (PatchSet ps : find(id)) {
+      Change.Id changeId = ps.getId().getParentKey();
+      if (changeId.equals(change.getChange().getId())) {
+        if (change.getControl().isPatchVisible(ps, dbProvider.get())) {
+          return new RevisionResource(change, ps);
+        }
+        break;
+      }
+    }
+    throw new ResourceNotFoundException(id);
+  }
+
+  private List<PatchSet> find(String id) throws OrmException {
+    ReviewDb db = dbProvider.get();
+    RevId revid = new RevId(id);
+    if (revid.isComplete()) {
+      return db.patchSets().byRevision(revid).toList();
+    } else {
+      return db.patchSets().byRevisionRange(revid, revid.max()).toList();
+    }
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java

@@ -93,7 +93,6 @@ public class GerritRequestModule extends FactoryModule {
     factory(RemoveReviewer.Factory.class);
     factory(RestoredSender.Factory.class);
     factory(RevertedSender.Factory.class);
-    factory(CommentSender.Factory.class);
     factory(MergedSender.Factory.class);
     factory(MergeFailSender.Factory.class);
     factory(PerformCreateGroup.Factory.class);

gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailModule.java

@@ -20,5 +20,6 @@ public class EmailModule extends FactoryModule {
   @Override
   protected void configure() {
     factory(AbandonedSender.Factory.class);
+    factory(CommentSender.Factory.class);
   }
 }