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(this.streamDetails_.originalUrl);
 }
 
 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) {
+    if (this.paramsParser.namedDest) {
+      this.viewport_.goToPage(
+        this.paramsParser.namedDestinations[this.paramsParser.namedDest]);
+    }
+    if (this.paramsParser.urlParams.page)
+      this.viewport_.goToPage(this.paramsParser.urlParams.page);
+    if (this.paramsParser.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 + this.paramsParser.urlParams.position.x,
+        y: this.viewport_.position.y + this.paramsParser.urlParams.position.y
       };
     }
-    if (this.urlParams_.zoom)
-      this.viewport_.setZoom(this.urlParams_.zoom);
+    if (this.paramsParser.urlParams.zoom)
+      this.viewport_.setZoom(this.paramsParser.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 process url from message and to navigate with in the

[raymes, 2015/01/13 01:59:31]

nit: "with in" -> "within"

[Deepak, 2015/01/13 08:29:07]

Done.

+   * PDF page or opening new url in the same tab.
+   * @param {Object} message data that have the url and newTab info.
+   */
+  navigate_: function(navigateData) {
+    var inputURL = navigateData.url;
+    // If there's no scheme, add http.
+    if (inputURL.indexOf('://') == -1 && inputURL.indexOf('mailto:') == -1) {
+        inputURL = 'http://' + inputURL;
+    }
+    // Make sure |url_copy| 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 |url_copy| is not only a scheme.
+    if (inputURL == 'http://' ||
+        inputURL == 'https://' ||
+        inputURL == 'ftp://' ||
+        inputURL == 'file://' ||
+        inputURL == 'mailto:') {
+      return;
+    }
+
+    if (navigateData.newTab) {
+      chrome.tabs.create({ url: inputURL });
+    } else {
+      var hashIndex = inputURL.search('#');
+      if (hashIndex != -1) {
+        var urlStr = inputURL.substring(hashIndex + 1);
+        if (this.paramsParser.namedDestinations[urlStr] != undefined)

[raymes, 2015/01/13 01:59:31]

Rather than just getting the named destination out of the params parser, we want
to re-use the logic in the params parser to get the page that should be
navigated to. The reason is that the URL might contain something like #page=N
which we should also support properly here. 

We can modify the params parser to support this by changing the constructor of
it to no longer take a url, so you would just construct a params parser:
new OpenPDFParamsParser();

Then you would call a function to get the URL params for a given URL:
this.paramsParser.getURLParams(url)

Then we can use the page number that it returns as the page to navigate to (this
corresponds to the GetInitialPage function inside instance.cc).

This way we could reuse the paramsParser here and inside handleUrlParams

[Deepak, 2015/01/13 08:29:07]

Done.

+          this.viewport_.goToPage(this.paramsParser.namedDestinations[urlStr]);
+      } else {
+          window.location.href = inputURL;
+      }
+    }

[raymes, 2015/01/13 01:59:31]

I think the logic in this function needs some work. Let's try to follow the
logic in instance.cc closely unless there is a good reason not to. This will
avoid introducing bugs.

[Deepak, 2015/01/13 08:29:07]

I have added a new function on lines of getInitalPage(), that gives us
pageNumber for the '#page=X' case and '#namedest=x' or '#namedest' case.
so now we don't need nameDest variable. and code is also simplified.

+  },
+
+  /**
+   * @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 22 matching lines @@
         else
           this.passwordScreen_.deny();
         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);

[raymes, 2015/01/13 01:59:31]

can we pull out the url and newTab properties and pass them separately to the
function?

[Deepak, 2015/01/13 08:29:07]

Done.

+        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 201 matching lines @@
               this.streamDetails_.tabId != -1);
   },
 
   /**
    * @type {Viewport} the viewport of the PDF viewer.
    */
   get viewport() {
     return this.viewport_;
   }
 };