Refactoring of code by moving navigate_() to separate file.

Refactoring of code by moving navigate_() to separate file.

This is followup for issue 447888 for putting navigate_() to separate
file to avoid cluttering in pdf.js file.

BUG=449909
Committed: https://crrev.com/150b0c13edf565cc617fb226f00798762ccbcf96
Cr-Commit-Position: refs/heads/master@{#312779}

[Deepak, 2015-01-19 09:21:59]

deepak.m1@samsung.com changed reviewers:
+ dgozman@chromium.org, raymes@chromium.org

[Deepak, 2015-01-19 09:21:59]

This is follow up CL for issue 447888 , for moving navigate_() to separate file.
PTAL at my changes.
Thanks

[dgozman, 2015-01-19 09:25:08]

component_extension_resources.grd lgtm

[raymes, 2015-01-20 02:58:29]

Thanks!

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/index-material.html (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/index-material.html:58: <script
src="chrome://resources/js/util.js"></script>
Could you please sort these in the same order as index.html? Thanks for catching
this :)

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/navigation_pdf.js (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:10: * tab based on newTab value.
nit: "Creates a new Navigator for navigating to links inside or outside the PDF"

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:12: function NavigationPDF() {
can we call this "Navigator" and and put it inside "navigator.js"

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:18: * Function to navigate to
given URL. This might involve navigating
nit "navigate to the given URL"

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:23: * @param {string} originalUrl
The opiginal page URL.
nit: opiginal->original

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:25: navigate: function(url,
newTab, originalUrl) {
nit: I just landed https://codereview.chromium.org/838153004/. Please merge the
changes from there into here.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:66:
viewer.paramsParser_.getViewportFromUrlParams(inputURL).page;
Can we pass in the paramsParser as an argument?

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:68:
viewer.viewport_.goToPage(pageNumber);
Can we pass in the viewport as an argument?

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:144: this.navigatePDF_ = new
NavigationPDF();
this.navigator_ = new Navigator(this.viewport_, this.paramsParser_);

[Deepak, 2015-01-20 07:08:37]

Thanks @Raymes for review and sharing your thoughts.
I have done changes as per your suggestion
PTAL.
Thanks

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/index-material.html (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/index-material.html:58: <script
src="chrome://resources/js/util.js"></script>
On 2015/01/20 02:58:28, raymes wrote:
> Could you please sort these in the same order as index.html? Thanks for
catching
> this :)

Done, In both file arranged js files in alphabetical order.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/navigation_pdf.js (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:10: * tab based on newTab value.
On 2015/01/20 02:58:29, raymes wrote:
> nit: "Creates a new Navigator for navigating to links inside or outside the
PDF"

Done.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:12: function NavigationPDF() {
On 2015/01/20 02:58:29, raymes wrote:
> can we call this "Navigator" and and put it inside "navigator.js"

Done.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:18: * Function to navigate to
given URL. This might involve navigating
On 2015/01/20 02:58:29, raymes wrote:
> nit "navigate to the given URL"

Done.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:23: * @param {string} originalUrl
The opiginal page URL.
On 2015/01/20 02:58:29, raymes wrote:
> nit: opiginal->original

Done.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:25: navigate: function(url,
newTab, originalUrl) {
On 2015/01/20 02:58:29, raymes wrote:
> nit: I just landed https://codereview.chromium.org/838153004/. Please merge
the
> changes from there into here.

Done.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:66:
viewer.paramsParser_.getViewportFromUrlParams(inputURL).page;
On 2015/01/20 02:58:29, raymes wrote:
> Can we pass in the paramsParser as an argument?

yes, it is better we pass these as argument.
As it is required only for navigation with the page or opening link in the same
tab.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigation_pdf.js:68:
viewer.viewport_.goToPage(pageNumber);
On 2015/01/20 02:58:29, raymes wrote:
> Can we pass in the viewport as an argument?

Same as above.

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:144: this.navigatePDF_ = new
NavigationPDF();
On 2015/01/20 02:58:29, raymes wrote:
> this.navigator_ = new Navigator(this.viewport_, this.paramsParser_);

we should not pass these here as information in these parameters is not ready. 
As these parameters are required for with in the page navigation or opening link
in the same tab.So passing this info in arguments.

[raymes, 2015-01-21 07:03:45]

Thanks!

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/855323002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:640: return this.bookmarks_;
Please keep this. It is used in tests

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/index.html (left):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/index.html:57: <script src="main.js"></script>
We should keep this order. They are ordered by dependency

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:20: * in the current tab.
nit: 4 space indent

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:22: * @param {Object} viewPort The
viewport info of the page.
nit: viewPort->viewport (and below)

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:399: this.streamDetails_.originalUrl,
this.viewport_, this.paramsParser_);
Can you please pass in the original URL, viewport and paramsParser on
construction. They never change so we might as well pass them in on construction

[Deepak, 2015-01-21 08:33:41]

Dear Raymes,
Thanks for reviewing and sharing your thoughts.
I have done changes.
PTAL.
Thanks

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/index.html (left):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/index.html:57: <script src="main.js"></script>
On 2015/01/21 07:03:44, raymes wrote:
> We should keep this order. They are ordered by dependency

Done.

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:20: * in the current tab.
On 2015/01/21 07:03:44, raymes wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:22: * @param {Object} viewPort The
viewport info of the page.
On 2015/01/21 07:03:44, raymes wrote:
> nit: viewPort->viewport (and below)

Done.

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/855323002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:399: this.streamDetails_.originalUrl,
this.viewport_, this.paramsParser_);
On 2015/01/21 07:03:44, raymes wrote:
> Can you please pass in the original URL, viewport and paramsParser on
> construction. They never change so we might as well pass them in on
construction

Done.

[Deepak, 2015-01-22 05:29:58]

@Raymes
These changes are ready to rolled in.
PTAL

[raymes, 2015-01-22 22:52:15]

lgtm!

https://codereview.chromium.org/855323002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/855323002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2015

[Deepak, 2015-01-23 04:02:07]

Dear Raymes,
nit addressed.
Thanks for review.

https://codereview.chromium.org/855323002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/855323002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/22 22:52:15, raymes wrote:
> nit: 2015

Done.

[Deepak, 2015-01-23 04:02:12]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-01-23 04:03:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/855323002/60001

[commit-bot: I haz the power, 2015-01-23 05:40:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-23 05:41:44]

Patchset 4 (id:??) landed as
https://crrev.com/150b0c13edf565cc617fb226f00798762ccbcf96
Cr-Commit-Position: refs/heads/master@{#312779}