Fix DocumentEngine line behaviors in Diffusion

Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:

  - Adding `$1-3` to the URI didn't work correctly with query parameters.
  - Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.

Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.

Reviewers: mydeveloperday

Reviewed By: mydeveloperday

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19305

Author: epriestley

resources/celerity/map.php

@@ -473,7 +473,7 @@
     'rsrc/js/core/behavior-keyboard-pager.js' => 'a8da01f0',
     'rsrc/js/core/behavior-keyboard-shortcuts.js' => '01fca1f0',
     'rsrc/js/core/behavior-lightbox-attachments.js' => '6b31879a',
-    'rsrc/js/core/behavior-line-linker.js' => '13e39479',
+    'rsrc/js/core/behavior-line-linker.js' => 'febf4ae7',
     'rsrc/js/core/behavior-more.js' => 'a80d0378',
     'rsrc/js/core/behavior-object-selector.js' => '77c1f0b0',
     'rsrc/js/core/behavior-oncopy.js' => '2926fff2',
@@ -638,7 +638,7 @@
     'javelin-behavior-phabricator-gesture-example' => '558829c2',
     'javelin-behavior-phabricator-keyboard-pager' => 'a8da01f0',
     'javelin-behavior-phabricator-keyboard-shortcuts' => '01fca1f0',
-    'javelin-behavior-phabricator-line-linker' => '13e39479',
+    'javelin-behavior-phabricator-line-linker' => 'febf4ae7',
     'javelin-behavior-phabricator-nav' => '836f966d',
     'javelin-behavior-phabricator-notification-example' => '8ce821c5',
     'javelin-behavior-phabricator-object-selector' => '77c1f0b0',
@@ -964,12 +964,6 @@
       'javelin-install',
       'javelin-util',
     ),
-    '13e39479' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-history',
-    ),
     '15d5ff71' => array(
       'aphront-typeahead-control-css',
       'phui-tag-view-css',
@@ -2174,6 +2168,12 @@
       'javelin-view-visitor',
       'javelin-util',
     ),
+    'febf4ae7' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-history',
+    ),
   ),
   'packages' => array(
     'conpherence.pkg.css' => array(

src/aphront/AphrontRequest.php

@@ -61,6 +61,10 @@ public function getURIData($key, $default = null) {
    */
   public function getURILineRange($key, $limit) {
     $range = $this->getURIData($key);
+    return self::parseURILineRange($range, $limit);
+  }
+
+  public static function parseURILineRange($range, $limit) {
     if (!strlen($range)) {
       return null;
     }

src/applications/diffusion/document/DiffusionDocumentRenderingEngine.php

@@ -14,10 +14,6 @@ public function getDiffusionRequest() {
     return $this->diffusionRequest;
   }
 
-  protected function getSelectedDocumentEngineKey() {
-    return $this->getRequest()->getStr('as');
-  }
-
   protected function newRefViewURI(
     PhabricatorDocumentRef $ref,
     PhabricatorDocumentEngine $engine) {
@@ -58,6 +54,15 @@ protected function newRefRenderURI(
       ));
   }
 
+  protected function getSelectedDocumentEngineKey() {
+    return $this->getRequest()->getStr('as');
+  }
+
+  protected function getSelectedLineRange() {
+    $range = $this->getDiffusionRequest()->getLine();
+    return AphrontRequest::parseURILineRange($range, 1000);
+  }
+
   protected function addApplicationCrumbs(
     PHUICrumbsView $crumbs,
     PhabricatorDocumentRef $ref = null) {

src/applications/files/document/render/PhabricatorDocumentRenderingEngine.php

@@ -54,7 +54,7 @@ final public function newDocumentView(PhabricatorDocumentRef $ref) {
     }
     $engine = $engines[$engine_key];
 
-    $lines = $request->getURILineRange('lines', 1000);
+    $lines = $this->getSelectedLineRange();
     if ($lines) {
       $engine->setHighlightedLines(range($lines[0], $lines[1]));
     }
@@ -157,18 +157,6 @@ final public function newDocumentView(PhabricatorDocumentRef $ref) {
       ->appendChild($viewport);
   }
 
-  abstract protected function newRefViewURI(
-    PhabricatorDocumentRef $ref,
-    PhabricatorDocumentEngine $engine);
-
-  abstract protected function newRefRenderURI(
-    PhabricatorDocumentRef $ref,
-    PhabricatorDocumentEngine $engine);
-
-  protected function getSelectedDocumentEngineKey() {
-    return $this->getRequest()->getURIData('engineKey');
-  }
-
   final public function newRenderResponse(PhabricatorDocumentRef $ref) {
     $request = $this->getRequest();
     $viewer = $request->getViewer();
@@ -280,6 +268,22 @@ protected function newCrumbs() {
     return $crumbs;
   }
 
+  abstract protected function newRefViewURI(
+    PhabricatorDocumentRef $ref,
+    PhabricatorDocumentEngine $engine);
+
+  abstract protected function newRefRenderURI(
+    PhabricatorDocumentRef $ref,
+    PhabricatorDocumentEngine $engine);
+
+  protected function getSelectedDocumentEngineKey() {
+    return $this->getRequest()->getURIData('engineKey');
+  }
+
+  protected function getSelectedLineRange() {
+    return $this->getRequest()->getURILineRange('lines', 1000);
+  }
+
   protected function addApplicationCrumbs(
     PHUICrumbsView $crumbs,
     PhabricatorDocumentRef $ref = null) {

webroot/rsrc/js/core/behavior-line-linker.js

@@ -144,17 +144,26 @@ JX.behavior('phabricator-line-linker', function() {
       var o = getRowNumber(origin);
       var t = getRowNumber(target);
       var uri = JX.Stratcom.getData(root).uri;
+      var path;
 
       if (!uri) {
-        uri = ('' + window.location).split('$')[0];
+        uri = JX.$U(window.location);
+        path = uri.getPath();
+        path = path.replace(/\$[\d-]+$/, '');
+        uri.setPath(path);
+        uri = uri.toString();
       }
 
       origin = null;
       target = null;
       root = null;
 
       var lines = (o == t ? o : Math.min(o, t) + '-' + Math.max(o, t));
-      uri = uri + '$' + lines;
+
+      uri = JX.$U(uri);
+      path = uri.getPath();
+      path = path + '$' + lines;
+      uri = uri.setPath(path).toString();
 
       JX.History.replace(uri);