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

chrome/browser/resources/pdf/pdf.js

Index: chrome/browser/resources/pdf/pdf.js
diff --git a/chrome/browser/resources/pdf/pdf.js b/chrome/browser/resources/pdf/pdf.js
index a8ec3081fc0f46a7015d7ede91659ce26a14140e..b773521c58e9e401460387aace225c482ad3792b 100644
--- a/chrome/browser/resources/pdf/pdf.js
+++ b/chrome/browser/resources/pdf/pdf.js
@@ -136,8 +136,7 @@ function PDFViewer(streamDetails) {
   }
 
   // Parse open pdf parameters.
-  var paramsParser = new OpenPDFParamsParser(this.streamDetails_.originalUrl);
-  this.urlParams_ = paramsParser.urlParams;
+  this.paramsParser = new OpenPDFParamsParser(this.streamDetails_.originalUrl);
 }
 
 PDFViewer.prototype = {
@@ -284,17 +283,21 @@ PDFViewer.prototype = {
    * 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);
   },
 
   /**
@@ -341,6 +344,49 @@ PDFViewer.prototype = {
 
   /**
    * @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.
    */
@@ -383,10 +429,10 @@ PDFViewer.prototype = {
         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;