Navigation to relative fragments does not work correctly for OOP pdf.

chrome/browser/resources/pdf/pdf.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * @return {number} Width of a scrollbar in pixels
  */
 function getScrollbarWidth() {
@@ skipping 118 matching lines @@
                                zoomChangeInfo.newZoomFactor);
       // We should not change zoom level when we are responsible for initiating
       // the zoom. onZoomChange() is called before setZoomComplete() callback
       // when we initiate the zoom.
       if ((zoomDelta > MIN_ZOOM_DELTA) && !this.setZoomInProgress_)
         this.viewport_.setZoom(zoomChangeInfo.newZoomFactor);
     }.bind(this));
   }
 
   // Parse open pdf parameters.
-  var paramsParser = new OpenPDFParamsParser(this.streamDetails_.originalUrl);
-  this.urlParams_ = paramsParser.urlParams;
+  this.paramsParser = new OpenPDFParamsParser();

[raymes, 2015/01/15 04:51:11]

Let's keep this named paramsParser_. An underscore at the end means it's private
to the class

[Deepak, 2015/01/15 06:30:05]

Done.

 }
 
 PDFViewer.prototype = {
   /**
    * @private
    * Handle key events. These may come from the user directly or via the
    * scripting API.
    * @param {KeyboardEvent} e the event to handle.
    */
   handleKeyEvent_: function(e) {
@@ skipping 126 matching lines @@
     });
   },
 
   /**
    * @private
    * Handle open pdf parameters. This function updates the viewport as per
    * the parameters mentioned in the url while opening pdf. The order is
    * important as later actions can override the effects of previous actions.
    */
   handleURLParams_: function() {
-    if (this.urlParams_.page)
-      this.viewport_.goToPage(this.urlParams_.page);
-    if (this.urlParams_.position) {
+    var urlParams =
+        this.paramsParser.getURLPDFParams(this.streamDetails_.originalUrl);
+    if (urlParams.page)
+      this.viewport_.goToPage(urlParams.page);
+    if (urlParams.nameddest)
+      this.viewport_.goToPage(urlParams.nameddest);

[raymes, 2015/01/15 04:51:11]

If we merge nameddest and page we can remove this if-statement

[Deepak, 2015/01/15 06:30:05]

Done.

+    if (urlParams.position) {
       // Make sure we don't cancel effect of page parameter.
       this.viewport_.position = {
-        x: this.viewport_.position.x + this.urlParams_.position.x,
-        y: this.viewport_.position.y + this.urlParams_.position.y
+        x: this.viewport_.position.x + urlParams.position.x,
+        y: this.viewport_.position.y + urlParams.position.y
       };
     }
-    if (this.urlParams_.zoom)
-      this.viewport_.setZoom(this.urlParams_.zoom);
+    if (urlParams.zoom)
+      this.viewport_.setZoom(urlParams.zoom);
   },
 
   /**
    * @private
    * Update the loading progress of the document in response to a progress
    * message being received from the plugin.
    * @param {number} progress the progress as a percentage.
    */
   updateProgress_: function(progress) {
     this.progressBar_.progress = progress;
@@ skipping 26 matching lines @@
    */
   onPasswordSubmitted_: function(event) {
     this.plugin_.postMessage({
       type: 'getPasswordComplete',
       password: event.detail.password
     });
   },
 
   /**
    * @private
+   * Helper function to navigate to given URL. This might involve navigating
+   * within the PDF page or opening a new url (in the same tab or a new tab)
+   * @param {string} message data that have the url for navigation.
+   * @param {boolean} message data that have info regarding navigation happen
+   * within the PDF page or in the newTab.
+   */
+  navigate_: function(urlFragment, isNewTab) {

[raymes, 2015/01/15 04:51:11]

I'd like to start reducing this size of this class so can we move this function
into a separate class/file? We can call it URLNavigator or something and it
would just have a single function which is navigate.

[Deepak, 2015/01/15 06:30:05]

I understand, I will do refactoring in a separate CL, Let this CL rolled in.

+    var namedFragment;
+    if (urlFragment.length != 0) {
+      // If |urlFragment| starts with '#', then it's for the same URL with a
+      // different URL fragment.
+      if (urlFragment.charAt(0) == '#') {
+        // if '#' is already present in |originalUrl| then remove old fragment
+        // and add new url fragment.
+        namedFragment = urlFragment.substring(1);
+        var originalUrl = this.streamDetails_.originalUrl;
+        var hashIndex = originalUrl.search('#');
+        if (hashIndex != -1)
+          urlFragment = originalUrl.substring(0, hashIndex) + urlFragment;
+        else
+          urlFragment = originalUrl + urlFragment;
+      }
+    }
+    var inputURL = urlFragment;
+    // If there's no scheme, add http.
+    if (inputURL.indexOf('://') == -1 && inputURL.indexOf('mailto:') == -1) {
+        inputURL = 'http://' + inputURL;
+    }
+    // Make sure inputURL starts with a valid scheme.
+    if (inputURL.indexOf('http://') != 0 &&
+        inputURL.indexOf('https://') != 0 &&
+        inputURL.indexOf('ftp://') != 0 &&
+        inputURL.indexOf('file://') != 0 &&
+        inputURL.indexOf('mailto:') != 0) {
+      return;
+    }
+    // Make sure inputURL is not only a scheme.
+    if (inputURL == 'http://' ||
+        inputURL == 'https://' ||
+        inputURL == 'ftp://' ||
+        inputURL == 'file://' ||
+        inputURL == 'mailto:') {
+      return;
+    }
+
+    if (isNewTab) {
+      chrome.tabs.create({ url: inputURL });

[raymes, 2015/01/15 04:51:11]

Cool, thanks for looking into that! Let's leave it as you have it for now then
:)

[Deepak, 2015/01/15 06:30:05]

Acknowledged.

+    } else {
+      var pageNumber = -1;
+      if (namedFragment)
+        pageNumber = this.paramsParser.namedDestinations[namedFragment];
+      if (pageNumber >= 0)
+        this.viewport_.goToPage(pageNumber);
+      else
+        window.location.href = inputURL;
+    }
+  },

[raymes, 2015/01/15 04:51:11]

Here is what I was thinking (there are a few changes from what you have). I've
stripped out all the comments but please add them back in! :)

navigate_: function(url, isNewTab) {
  if (url.length == 0)
    return;

  var originalUrl = this.streamDetails_.originalUrl;

  if (url[0] == '#') {
    var hashIndex = originalUrl.search('#')
    if (hashIndex != -1)
      url = originalUrl.substring(0, hashIndex) + url
    else
      url = originalUrl + url
  }

  if (inputURL.indexOf('://') == -1 && inputURL.indexOf('mailto:') == -1) {
    inputURL = 'http://' + inputURL;
  }

  if (inputURL.indexOf('http://') != 0 &&
      inputURL.indexOf('https://') != 0 &&
      inputURL.indexOf('ftp://') != 0 &&
      inputURL.indexOf('file://') != 0 &&
      inputURL.indexOf('mailto:') != 0) {
    return;
  }

  if (inputURL == 'http://' ||
      inputURL == 'https://' ||
      inputURL == 'ftp://' ||
      inputURL == 'file://' ||
      inputURL == 'mailto:') {
    return;
  }

  if (isNewTab) {
    window.open(url);
  } else {
    var page = this.paramsParser.getURLPDFParams(url).page;
    if (page)
      this.viewport_.goToPage(page);
    else
      window.location.href = url;    
  }
}

[Deepak, 2015/01/15 06:30:05]

Done.

+
+  /**
+   * @private
    * An event handler for handling message events received from the plugin.
    * @param {MessageObject} message a message event.
    */
   handlePluginMessage_: function(message) {
     switch (message.data.type.toString()) {
       case 'documentDimensions':
         this.documentDimensions_ = message.data;
         this.viewport_.setDocumentDimensions(this.documentDimensions_);
         // If we received the document dimensions, the password was good so we
         // can dismiss the password screen.
@@ skipping 25 matching lines @@
       case 'getSelectedTextReply':
         this.sendScriptingMessage_(message.data);
         break;
       case 'goToPage':
         this.viewport_.goToPage(message.data.page);
         break;
       case 'loadProgress':
         this.updateProgress_(message.data.progress);
         break;
       case 'navigate':
-        if (message.data.newTab)
-          window.open(message.data.url);
-        else
-          window.location.href = message.data.url;
+        this.navigate_(message.data.url, message.data.newTab);
+        break;
+      case 'setNamedDestinations':
+        this.paramsParser.namedDestinations = message.data.namedDestinations;
         break;
       case 'setScrollPosition':
         var position = this.viewport_.position;
         if (message.data.x != undefined)
           position.x = message.data.x;
         if (message.data.y != undefined)
           position.y = message.data.y;
         this.viewport_.position = position;
         break;
       case 'setTranslatedStrings':
@@ skipping 204 matching lines @@
               this.streamDetails_.tabId != -1);
   },
 
   /**
    * @type {Viewport} the viewport of the PDF viewer.
    */
   get viewport() {
     return this.viewport_;
   }
 };