Merge branch 'master' into redesign-2015

src/__phutil_library_map__.php

@@ -159,6 +159,7 @@ phutil_register_library_map(array(
     'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php',
     'AphrontResponse' => 'aphront/response/AphrontResponse.php',
     'AphrontSideNavFilterView' => 'view/layout/AphrontSideNavFilterView.php',
+    'AphrontSite' => 'aphront/site/AphrontSite.php',
     'AphrontStackTraceView' => 'view/widget/AphrontStackTraceView.php',
     'AphrontStandaloneHTMLResponse' => 'aphront/response/AphrontStandaloneHTMLResponse.php',
     'AphrontTableView' => 'view/control/AphrontTableView.php',
@@ -1660,6 +1661,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigSchemaQuery' => 'applications/config/schema/PhabricatorConfigSchemaQuery.php',
     'PhabricatorConfigSchemaSpec' => 'applications/config/schema/PhabricatorConfigSchemaSpec.php',
     'PhabricatorConfigServerSchema' => 'applications/config/schema/PhabricatorConfigServerSchema.php',
+    'PhabricatorConfigSiteModuleController' => 'applications/config/controller/PhabricatorConfigSiteModuleController.php',
     'PhabricatorConfigSiteSource' => 'infrastructure/env/PhabricatorConfigSiteSource.php',
     'PhabricatorConfigSource' => 'infrastructure/env/PhabricatorConfigSource.php',
     'PhabricatorConfigStackSource' => 'infrastructure/env/PhabricatorConfigStackSource.php',
@@ -2328,6 +2330,7 @@ phutil_register_library_map(array(
     'PhabricatorPhrequentConfigOptions' => 'applications/phrequent/config/PhabricatorPhrequentConfigOptions.php',
     'PhabricatorPhrictionApplication' => 'applications/phriction/application/PhabricatorPhrictionApplication.php',
     'PhabricatorPhrictionConfigOptions' => 'applications/phriction/config/PhabricatorPhrictionConfigOptions.php',
+    'PhabricatorPlatformSite' => 'aphront/site/PhabricatorPlatformSite.php',
     'PhabricatorPolicies' => 'applications/policy/constants/PhabricatorPolicies.php',
     'PhabricatorPolicy' => 'applications/policy/storage/PhabricatorPolicy.php',
     'PhabricatorPolicyApplication' => 'applications/policy/application/PhabricatorPolicyApplication.php',
@@ -2525,6 +2528,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php',
     'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php',
     'PhabricatorRepositoryVersion' => 'applications/repository/constants/PhabricatorRepositoryVersion.php',
+    'PhabricatorResourceSite' => 'aphront/site/PhabricatorResourceSite.php',
     'PhabricatorRobotsController' => 'applications/system/controller/PhabricatorRobotsController.php',
     'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php',
     'PhabricatorSMS' => 'infrastructure/sms/storage/PhabricatorSMS.php',
@@ -2614,6 +2618,7 @@ phutil_register_library_map(array(
     'PhabricatorSetupIssue' => 'applications/config/issue/PhabricatorSetupIssue.php',
     'PhabricatorSetupIssueUIExample' => 'applications/uiexample/examples/PhabricatorSetupIssueUIExample.php',
     'PhabricatorSetupIssueView' => 'applications/config/view/PhabricatorSetupIssueView.php',
+    'PhabricatorSite' => 'aphront/site/PhabricatorSite.php',
     'PhabricatorSlowvoteApplication' => 'applications/slowvote/application/PhabricatorSlowvoteApplication.php',
     'PhabricatorSlowvoteChoice' => 'applications/slowvote/storage/PhabricatorSlowvoteChoice.php',
     'PhabricatorSlowvoteCloseController' => 'applications/slowvote/controller/PhabricatorSlowvoteCloseController.php',
@@ -2892,6 +2897,8 @@ phutil_register_library_map(array(
     'PhameBlogListController' => 'applications/phame/controller/blog/PhameBlogListController.php',
     'PhameBlogLiveController' => 'applications/phame/controller/blog/PhameBlogLiveController.php',
     'PhameBlogQuery' => 'applications/phame/query/PhameBlogQuery.php',
+    'PhameBlogResourceSite' => 'applications/phame/site/PhameBlogResourceSite.php',
+    'PhameBlogSite' => 'applications/phame/site/PhameBlogSite.php',
     'PhameBlogSkin' => 'applications/phame/skins/PhameBlogSkin.php',
     'PhameBlogTransaction' => 'applications/phame/storage/PhameBlogTransaction.php',
     'PhameBlogViewController' => 'applications/phame/controller/blog/PhameBlogViewController.php',
@@ -2920,6 +2927,7 @@ phutil_register_library_map(array(
     'PhameQueryPostsConduitAPIMethod' => 'applications/phame/conduit/PhameQueryPostsConduitAPIMethod.php',
     'PhameResourceController' => 'applications/phame/controller/PhameResourceController.php',
     'PhameSchemaSpec' => 'applications/phame/storage/PhameSchemaSpec.php',
+    'PhameSite' => 'applications/phame/site/PhameSite.php',
     'PhameSkinSpecification' => 'applications/phame/skins/PhameSkinSpecification.php',
     'PhluxController' => 'applications/phlux/controller/PhluxController.php',
     'PhluxDAO' => 'applications/phlux/storage/PhluxDAO.php',
@@ -3525,6 +3533,7 @@ phutil_register_library_map(array(
     'AphrontRequestTestCase' => 'PhabricatorTestCase',
     'AphrontResponse' => 'Phobject',
     'AphrontSideNavFilterView' => 'AphrontView',
+    'AphrontSite' => 'Phobject',
     'AphrontStackTraceView' => 'AphrontView',
     'AphrontStandaloneHTMLResponse' => 'AphrontHTMLResponse',
     'AphrontTableView' => 'AphrontView',
@@ -5270,6 +5279,7 @@ phutil_register_library_map(array(
     'PhabricatorConfigSchemaQuery' => 'Phobject',
     'PhabricatorConfigSchemaSpec' => 'Phobject',
     'PhabricatorConfigServerSchema' => 'PhabricatorConfigStorageSchema',
+    'PhabricatorConfigSiteModuleController' => 'PhabricatorConfigController',
     'PhabricatorConfigSiteSource' => 'PhabricatorConfigProxySource',
     'PhabricatorConfigSource' => 'Phobject',
     'PhabricatorConfigStackSource' => 'PhabricatorConfigSource',
@@ -6028,6 +6038,7 @@ phutil_register_library_map(array(
     'PhabricatorPhrequentConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorPhrictionApplication' => 'PhabricatorApplication',
     'PhabricatorPhrictionConfigOptions' => 'PhabricatorApplicationConfigOptions',
+    'PhabricatorPlatformSite' => 'PhabricatorSite',
     'PhabricatorPolicies' => 'PhabricatorPolicyConstants',
     'PhabricatorPolicy' => array(
       'PhabricatorPolicyDAO',
@@ -6296,6 +6307,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase',
     'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO',
     'PhabricatorRepositoryVersion' => 'Phobject',
+    'PhabricatorResourceSite' => 'PhabricatorSite',
     'PhabricatorRobotsController' => 'PhabricatorController',
     'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine',
     'PhabricatorSMS' => 'PhabricatorSMSDAO',
@@ -6387,6 +6399,7 @@ phutil_register_library_map(array(
     'PhabricatorSetupIssue' => 'Phobject',
     'PhabricatorSetupIssueUIExample' => 'PhabricatorUIExample',
     'PhabricatorSetupIssueView' => 'AphrontView',
+    'PhabricatorSite' => 'AphrontSite',
     'PhabricatorSlowvoteApplication' => 'PhabricatorApplication',
     'PhabricatorSlowvoteChoice' => 'PhabricatorSlowvoteDAO',
     'PhabricatorSlowvoteCloseController' => 'PhabricatorSlowvoteController',
@@ -6711,6 +6724,8 @@ phutil_register_library_map(array(
     'PhameBlogListController' => 'PhameController',
     'PhameBlogLiveController' => 'PhameController',
     'PhameBlogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
+    'PhameBlogResourceSite' => 'PhameSite',
+    'PhameBlogSite' => 'PhameSite',
     'PhameBlogSkin' => 'PhabricatorController',
     'PhameBlogTransaction' => 'PhabricatorApplicationTransaction',
     'PhameBlogViewController' => 'PhameController',
@@ -6745,6 +6760,7 @@ phutil_register_library_map(array(
     'PhameQueryPostsConduitAPIMethod' => 'PhameConduitAPIMethod',
     'PhameResourceController' => 'CelerityResourceController',
     'PhameSchemaSpec' => 'PhabricatorConfigSchemaSpec',
+    'PhameSite' => 'PhabricatorSite',
     'PhameSkinSpecification' => 'Phobject',
     'PhluxController' => 'PhabricatorController',
     'PhluxDAO' => 'PhabricatorLiskDAO',

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -350,7 +350,9 @@ abstract class AphrontApplicationConfiguration extends Phobject {
       }
     }
 
-    if (PhabricatorEnv::getEnvConfig('security.require-https')) {
+    $site = $this->buildSiteForRequest($request);
+
+    if ($site->shouldRequireHTTPS()) {
       if (!$request->isHTTPS()) {
         $https_uri = $request->getRequestURI();
         $https_uri->setDomain($request->getHost());
@@ -362,82 +364,9 @@ abstract class AphrontApplicationConfiguration extends Phobject {
       }
     }
 
-    $path         = $request->getPath();
-    $host         = $request->getHost();
-    $base_uri     = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
-    $prod_uri     = PhabricatorEnv::getEnvConfig('phabricator.production-uri');
-    $file_uri     = PhabricatorEnv::getEnvConfig(
-      'security.alternate-file-domain');
-    $allowed_uris = PhabricatorEnv::getEnvConfig('phabricator.allowed-uris');
-
-    $uris = array_merge(
-      array(
-        $base_uri,
-        $prod_uri,
-      ),
-      $allowed_uris);
-
-    $cdn_routes = array(
-      '/res/',
-      '/file/data/',
-      '/file/xform/',
-      '/phame/r/',
-      );
-
-    $host_match = false;
-    foreach ($uris as $uri) {
-      if ($host === id(new PhutilURI($uri))->getDomain()) {
-        $host_match = true;
-        break;
-      }
-    }
-
-    if (!$host_match) {
-      if ($host === id(new PhutilURI($file_uri))->getDomain()) {
-        foreach ($cdn_routes as $route) {
-          if (strncmp($path, $route, strlen($route)) == 0) {
-            $host_match = true;
-            break;
-          }
-        }
-      }
-    }
-
-    // NOTE: If the base URI isn't defined yet, don't activate alternate
-    // domains.
-    if ($base_uri && !$host_match) {
-
-      try {
-        $blog = id(new PhameBlogQuery())
-          ->setViewer(new PhabricatorUser())
-          ->withDomain($host)
-          ->executeOne();
-      } catch (PhabricatorPolicyException $ex) {
-        throw new Exception(
-          pht(
-            'This blog is not visible to logged out users, so it can not be '.
-            'visited from a custom domain.'));
-      }
-
-      if (!$blog) {
-        if ($prod_uri && $prod_uri != $base_uri) {
-          $prod_str = pht('%s or %s', $base_uri, $prod_uri);
-        } else {
-          $prod_str = $base_uri;
-        }
-        throw new Exception(
-          pht(
-            'Specified domain %s is not configured for Phabricator '.
-            'requests. Please use %s to visit this instance.',
-            $host,
-            $prod_str));
-      }
-
-      // TODO: Make this more flexible and modular so any application can
-      // do crazy stuff here if it wants.
-
-      $path = '/phame/live/'.$blog->getID().'/'.$path;
-    }
+    // TODO: Really, the Site should get more control here and be able to
+    // do its own routing logic if it wants, but we don't need that for now.
+    $path = $site->getPathForRouting($request);
 
     list($controller, $uri_data) = $this->buildControllerForPath($path);
     if (!$controller) {
@@ -509,4 +438,29 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     return array($controller, $uri_data);
   }
 
+  private function buildSiteForRequest(AphrontRequest $request) {
+    $sites = PhabricatorSite::getAllSites();
+
+    $site = null;
+    foreach ($sites as $candidate) {
+      $site = $candidate->newSiteForRequest($request);
+      if ($site) {
+        break;
+      }
+    }
+
+    if (!$site) {
+      $path = $request->getPath();
+      $host = $request->getHost();
+      throw new Exception(
+        pht(
+          'This request asked for "%s" on host "%s", but no site is '.
+          'configured which can serve this request.',
+          $path,
+          $host));
+    }
+
+    return $site;
+  }
+
 }

src/aphront/site/AphrontSite.php

@@ -0,0 +1,52 @@
+<?php
+
+abstract class AphrontSite extends Phobject {
+
+  abstract public function getPriority();
+  abstract public function getDescription();
+
+  abstract public function shouldRequireHTTPS();
+  abstract public function newSiteForRequest(AphrontRequest $request);
+
+  /**
+   * NOTE: This is temporary glue; eventually, sites will return an entire
+   * route map.
+   */
+  public function getPathForRouting(AphrontRequest $request) {
+    return $request->getPath();
+  }
+
+  protected function isHostMatch($host, array $uris) {
+    foreach ($uris as $uri) {
+      if (!strlen($uri)) {
+        continue;
+      }
+
+      $domain = id(new PhutilURI($uri))->getDomain();
+
+      if ($domain === $host) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  protected function isPathPrefixMatch($path, array $paths) {
+    foreach ($paths as $candidate) {
+      if (strncmp($path, $candidate, strlen($candidate)) === 0) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  final public static function getAllSites() {
+    return id(new PhutilClassMapQuery())
+      ->setAncestorClass(__CLASS__)
+      ->setSortMethod('getPriority')
+      ->execute();
+  }
+
+}

src/aphront/site/PhabricatorPlatformSite.php

@@ -0,0 +1,33 @@
+<?php
+
+final class PhabricatorPlatformSite extends PhabricatorSite {
+
+  public function getDescription() {
+    return pht('Serves the core platform and applications.');
+  }
+
+  public function getPriority() {
+    return 1000;
+  }
+
+  public function newSiteForRequest(AphrontRequest $request) {
+    $uris = array();
+    $uris[] = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
+    $uris[] = PhabricatorEnv::getEnvConfig('phabricator.production-uri');
+
+    $allowed = PhabricatorEnv::getEnvConfig('phabricator.allowed-uris');
+    if ($allowed) {
+      foreach ($allowed as $uri) {
+        $uris[] = $uri;
+      }
+    }
+
+    $host = $request->getHost();
+    if ($this->isHostMatch($host, $uris)) {
+      return new PhabricatorPlatformSite();
+    }
+
+    return null;
+  }
+
+}

src/aphront/site/PhabricatorResourceSite.php

@@ -0,0 +1,41 @@
+<?php
+
+final class PhabricatorResourceSite extends PhabricatorSite {
+
+  public function getDescription() {
+    return pht('Serves static resources like images, CSS and JS.');
+  }
+
+  public function getPriority() {
+    return 2000;
+  }
+
+  public function newSiteForRequest(AphrontRequest $request) {
+    $host = $request->getHost();
+
+    $uri = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
+    if (!strlen($uri)) {
+      return null;
+    }
+
+    if ($this->isHostMatch($host, array($uri))) {
+      return new PhabricatorResourceSite();
+    }
+
+    // These are CDN routes, so we let them through even if the "Host" header
+    // doesn't match anything we recognize. The
+    $whitelist = array(
+      '/res/',
+      '/file/data/',
+      '/file/xform/',
+    );
+
+    $path = $request->getPath();
+    if ($this->isPathPrefixMatch($path, $whitelist)) {
+      return new PhabricatorResourceSite();
+    }
+
+    return null;
+  }
+
+}

src/aphront/site/PhabricatorSite.php

@@ -0,0 +1,9 @@
+<?php
+
+abstract class PhabricatorSite extends AphrontSite {
+
+  public function shouldRequireHTTPS() {
+    return PhabricatorEnv::getEnvConfig('security.require-https');
+  }
+
+}

src/applications/config/application/PhabricatorConfigApplication.php

@@ -56,6 +56,9 @@ final class PhabricatorConfigApplication extends PhabricatorApplication {
           '(?P<key>[^/]+)/' => 'PhabricatorConfigIssueViewController',
         ),
         'cache/' => 'PhabricatorConfigCacheController',
+        'module/' => array(
+          'sites/' => 'PhabricatorConfigSiteModuleController',
+        ),
       ),
     );
   }

src/applications/config/controller/PhabricatorConfigController.php

@@ -24,6 +24,8 @@ abstract class PhabricatorConfigController extends PhabricatorController {
     $nav->addFilter('cache/', pht('Cache Status'));
     $nav->addLabel(pht('Welcome'));
     $nav->addFilter('welcome/', pht('Welcome Screen'));
+    $nav->addLabel(pht('Modules'));
+    $nav->addFilter('module/sites/', pht('Sites'));
 
     return $nav;
   }

src/applications/config/controller/PhabricatorConfigSiteModuleController.php

@@ -0,0 +1,56 @@
+<?php
+
+final class PhabricatorConfigSiteModuleController
+  extends PhabricatorConfigController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $sites = AphrontSite::getAllSites();
+
+    $rows = array();
+    foreach ($sites as $key => $site) {
+      $rows[] = array(
+        $site->getPriority(),
+        $key,
+        $site->getDescription(),
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setHeaders(
+        array(
+          pht('Priority'),
+          pht('Class'),
+          pht('Description'),
+        ))
+      ->setColumnClasses(
+        array(
+          null,
+          'pri',
+          'wide',
+        ));
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Site Modules'))
+      ->appendChild($table);
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb(pht('Site Modules'));
+
+    $nav = $this->buildSideNavView();
+    $nav->selectFilter('module/sites/');
+    $nav->appendChild(
+      array(
+        $crumbs,
+        $box,
+      ));
+
+    return $this->buildApplicationPage(
+      $nav,
+      array(
+        'title' => array(pht('Site Modules')),
+      ));
+  }
+
+}

src/applications/phame/site/PhameBlogResourceSite.php

@@ -0,0 +1,30 @@
+<?php
+
+final class PhameBlogResourceSite extends PhameSite {
+
+  public function getDescription() {
+    return pht('Serves static resources for blogs.');
+  }
+
+  public function getPriority() {
+    return 3000;
+  }
+
+  public function newSiteForRequest(AphrontRequest $request) {
+    if (!$this->isPhameActive()) {
+      return null;
+    }
+
+    $whitelist = array(
+      '/phame/r/',
+    );
+
+    $path = $request->getPath();
+    if (!$this->isPathPrefixMatch($path, $whitelist)) {
+      return null;
+    }
+
+    return new PhameBlogResourceSite();
+  }
+
+}

src/applications/phame/site/PhameBlogSite.php

@@ -0,0 +1,63 @@
+<?php
+
+final class PhameBlogSite extends PhameSite {
+
+  private $blog;
+
+  public function setBlog(PhameBlog $blog) {
+    $this->blog = $blog;
+    return $this;
+  }
+
+  public function getBlog() {
+    return $this->blog;
+  }
+
+  public function getDescription() {
+    return pht('Serves blogs with custom domains.');
+  }
+
+  public function shouldRequireHTTPS() {
+    // TODO: We should probably provide options here eventually, but for now
+    // just never require HTTPS for external-domain blogs.
+    return false;
+  }
+
+  public function getPriority() {
+    return 4000;
+  }
+
+  public function newSiteForRequest(AphrontRequest $request) {
+    if (!$this->isPhameActive()) {
+      return null;
+    }
+
+    $host = $request->getHost();
+
+    try {
+      $blog = id(new PhameBlogQuery())
+        ->setViewer(new PhabricatorUser())
+        ->withDomain($host)
+        ->executeOne();
+    } catch (PhabricatorPolicyException $ex) {
+      throw new Exception(
+        pht(
+          'This blog is not visible to logged out users, so it can not be '.
+          'visited from a custom domain.'));
+    }
+
+    if (!$blog) {
+      return null;
+    }
+
+    return id(new PhameBlogSite())->setBlog($blog);
+  }
+
+  public function getPathForRouting(AphrontRequest $request) {
+    $path = $request->getPath();
+    $id = $this->getBlog()->getID();
+
+    return "/phame/live/{$id}/{$path}";
+  }
+
+}

src/applications/phame/site/PhameSite.php

@@ -0,0 +1,22 @@
+<?php
+
+abstract class PhameSite extends PhabricatorSite {
+
+  protected function isPhameActive() {
+    $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
+    if (!strlen($base_uri)) {
+      // Don't activate Phame if we don't have a base URI configured.
+      return false;
+    }
+
+    $phame_installed = PhabricatorApplication::isClassInstalled(
+      'PhabricatorPhameApplication');
+    if (!$phame_installed) {
+      // Don't activate Phame if the the application is uninstalled.
+      return false;
+    }
+
+    return true;
+  }
+
+}

src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php

@@ -3,21 +3,13 @@
 final class PhabricatorApplicationTransactionCommentEditController
   extends PhabricatorApplicationTransactionController {
 
-  private $phid;
-
-  public function willProcessRequest(array $data) {
-    $this->phid = $data['phid'];
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
 
     $xaction = id(new PhabricatorObjectQuery())
-      ->withPHIDs(array($this->phid))
-      ->setViewer($user)
+      ->setViewer($viewer)
+      ->withPHIDs(array($request->getURIData('phid')))
       ->executeOne();
-
     if (!$xaction) {
       return new Aphront404Response();
     }
@@ -33,11 +25,9 @@ final class PhabricatorApplicationTransactionCommentEditController
       return new Aphront400Response();
     }
 
-    $obj_phid = $xaction->getObjectPHID();
-    $obj_handle = id(new PhabricatorHandleQuery())
-      ->setViewer($user)
-      ->withPHIDs(array($obj_phid))
-      ->executeOne();
+    $phid = $xaction->getObjectPHID();
+    $handles = $viewer->loadHandles(array($phid));
+    $obj_handle = $handles[$phid];
 
     if ($request->isDialogFormPost()) {
       $text = $request->getStr('text');
@@ -49,7 +39,7 @@ final class PhabricatorApplicationTransactionCommentEditController
       }
 
       $editor = id(new PhabricatorApplicationTransactionCommentEditor())
-        ->setActor($user)
+        ->setActor($viewer)
         ->setContentSource(PhabricatorContentSource::newFromRequest($request))
         ->applyEdit($xaction, $comment);
 
@@ -60,28 +50,20 @@ final class PhabricatorApplicationTransactionCommentEditController
       }
     }
 
-    $dialog = id(new AphrontDialogView())
-      ->setUser($user)
-      ->setSubmitURI(
-        $this->getApplicationURI('/transactions/edit/'.$xaction->getPHID().'/'))
-      ->setTitle(pht('Edit Comment'));
+    $form = id(new AphrontFormView())
+      ->setUser($viewer)
+      ->setFullWidth(true)
+      ->appendControl(
+        id(new PhabricatorRemarkupControl())
+        ->setName('text')
+        ->setValue($xaction->getComment()->getContent()));
 
-    $dialog
+    return $this->newDialog()
+      ->setTitle(pht('Edit Comment'))
       ->addHiddenInput('anchor', $request->getStr('anchor'))
-      ->appendChild(
-        id(new PHUIFormLayoutView())
-        ->setFullWidth(true)
-        ->appendChild(
-          id(new PhabricatorRemarkupControl())
-          ->setUser($user)
-          ->setName('text')
-          ->setValue($xaction->getComment()->getContent())));
-
-    $dialog
+      ->appendForm($form)
       ->addSubmitButton(pht('Save Changes'))
       ->addCancelButton($obj_handle->getURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }

src/view/form/control/AphrontFormPolicyControl.php

@@ -39,6 +39,22 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
     return $this;
   }
 
+  public function getSerializedValue() {
+    return json_encode(array(
+      $this->getValue(),
+      $this->getSpacePHID(),
+    ));
+  }
+
+  public function readSerializedValue($value) {
+    $decoded = phutil_json_decode($value);
+    $policy_value = $decoded[0];
+    $space_phid = $decoded[1];
+    $this->setValue($policy_value);
+    $this->setSpacePHID($space_phid);
+    return $this;
+  }
+
   public function readValueFromDictionary(array $dictionary) {
     // TODO: This is a little hacky but will only get us into trouble if we
     // have multiple view policy controls in multiple paged form views on the