Fix for PDFs with lots of named destinations take a long time to load.

Fix for PDFs with lots of named destinations take a long time to load.

When PDF have lots of named destinations then as we are fetching
all the named destinations at load time, so this will increase
loading time.
This is a workaround so that we will get named destination whenever
it is required by passing message from page to plugin.

BUG=457457
Committed: https://crrev.com/9cdcfdfdb5fd39249669a5743f39d739fe05b47a
Cr-Commit-Position: refs/heads/master@{#316685}

[Deepak, 2015-02-12 07:32:52]

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

[Deepak, 2015-02-12 07:32:53]

PTAL and share your thoughts.
I will update test cases for this, Once this approach logic get accepted.
Thanks

[raymes, 2015-02-12 22:19:05]

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
File chrome/browser/resources/pdf/navigator.js (left):

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/navigator.js:83: }
None of this should be removed. Instead the code should look like this:

  this.paramsParser_.getViewportFromUrlParams(url, onViewportReceived);
}

onViewportReceived: function(initialViewport) {
  var pageNumber = initialViewport.pageNumber;
  if (pageNumber != undefined)
    this.viewport_.goToPage(pageNumber);
  else
    this.navigateInCurrentTabCallback_(url);
});

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

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:63:
getViewportFromUrlParams: function(url, namedDestCallback, isNavigate) {
-None of the isNavigate stuff should be here, see the comment in the previous
file.
-This function should not return anything. The callback should always be called
with the viewport.
-we can just call namedDestCallback->callback

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:67: return
viewportPosition;
callback(viewportPosition)
return;

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:74:
viewportPosition['page'] = this.namedDestinations[paramTokens[0]];
This is where we need to call into the plugin. We should save off the state of
viewportPosition into a queue and call getNamedDestination:

this.outstandingRequests_.push({
  callback: callback,
  viewportPosition: viewportPosition
});
getNamedDestination(paramTokens[0]);
return;

And then in namedDestinationReceived:
namedDestinationReceived: function(namedDestinationPage) {
  var oustandingRequest = callback.shift();
  if (!outstandingRequest.viewportPosition.page)
    oustandingRequest.viewportPosition.page = namedDestinationPage;
  oustandingRequest.callback(oustandingRequest.viewportPosition);
}

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:83: return
viewportPosition;
callback(viewportPosition)
return;

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:103: } else {
The same thing here. We should shift this code this if-block right to the end of
the function and call:

this.outstandingRequests_.push({
  callback: callback,
  viewportPosition: viewportPosition
});
getNamedDestination(paramTokens[0]);
return;

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/open_pdf_params_parser.js:118: return
viewportPosition;
callback(viewportPosition)
return;

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

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:184:
this.onNavigateInCurrentTab_.bind(this), onNavigateInNewTab);
This shouldn't change

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:345: save_: function() {
this.plugin_.postMessage({type: 'save'}); },
nit: don't change these

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:351: namedDestCallback_:
function(namedDestination, url) {
This should be called getNamedDestination(name)

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:372: },
This shouldn't be here

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:384: false);
This should look like

this.parmsParser_.getViewportFromUrlParams(this.streamDetails_.originalUrl,
function(initialViewport) {
  if (initialViewport.page && initialViewport.page != -1)
    this.viewport_.goToPage(initialViewport.page);
  if (initialViewport.position) {
    // Make sure we don't cancel effect of page parameter.
.... (the rest of the code in this function)

https://codereview.chromium.org/918953002/diff/1/chrome/browser/resources/pdf...
chrome/browser/resources/pdf/pdf.js:529: case 'getNamedDestinationReply':
this should just call: this.paramsParser_.onNamedDestinationReceived(...)

[Deepak, 2015-02-13 14:59:29]

Thanks for sharing your view.
I understood your approach & tried it.
But facing 3 issues:

1) navigation is not working on first click, when
I visit  http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf
and 
scrolled to last page and then select "Go back to the first page." it does not
go to first page.
2) nameddestinations like #page=4 and #UY are not working at all.
3) And when I an doing clicks 2 different hrefs fast that moves viewport to
different 
pages then it will be jurky as we get page number asynchronous way.

I am raising modified changes temporarily that are just to validate approach as:
https://codereview.chromium.org/923063002/

Will do changes in this review only later.
Please pointed out if you found anything that I am missing.

currently due to some issue console.log() is not working for me..
so not able to get actual issue at js side.
I will update with more finding ASAP.

[Deepak, 2015-02-14 10:23:43]

@Raymes
Changes done as suggested.
It is working as expected.
PTAL.
Thanks

[raymes, 2015-02-16 02:02:31]

Thanks!

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:78: url, function(initialViewport) {
nit: initialViewport->viewportPosition 
nit: could you pull this function out into a separate function in the class
called onViewportReceived?

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:17:
this.getNamedDestination_ = getNamedDestination;
nit: getNamedDestinationFunction_

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:59: * @param {Object}
callback function to be called with viewport info.
nit: {Function}

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:74:
viewportPosition['page'] = this.namedDestinations[paramTokens[0]];
Let's not bother caching the namedDestinations here, I don't think it will be
particularly useful. So we can remove this line and the 4 below and always just
go to the plugin.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:112: }
Same here - let's remove the above few lines and remove this.namedDestinations
altogether

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:126: * namedDestination
and call callback with viewport info.
This is called when a named destination is received and the page number
corresponding to the request for a named destination is passed in.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:127: * @param {number}
pageNumber for viewport.
nit: pageNumber The page corresponding to the named destination requested.

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:194: new
OpenPDFParamsParser(this.getNamedDestination_.bind(this));
There is some refactoring that would be really nice to do here, but let's leave
it for another CL if you are interested. The idea is to create a PluginProxy
class for communicating with the plugin. Rather than passing in
this.getNamedDestination_ here, we would pass in the PluginProxy class which has
a function getNamedDestination hanging off it.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:371: * Callback to fetch namedDestination
from plugin.
nit: Fetches the page number corresponding to the given named destination from
the plugin.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:389: this.streamDetails_.originalUrl,
function(initialViewport) {
nit: viewportPosition

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:429: this.handleURLParams_();
nit: Let's call this instead:

this.paramsParser_.getViewportFromUrlParams(this.streamDetails_.originalUrl,
this.handleURLParams_.bind(this);

Then we can simplify handleURLParams_

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
File chrome/test/data/pdf/navigator_test.js (left):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
chrome/test/data/pdf/navigator_test.js:89: chrome.test.assertEq(300,
viewport.position.y);
How come this is removed?

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
File chrome/test/data/pdf/params_parser_test.js (right):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
chrome/test/data/pdf/params_parser_test.js:19:
paramsParser.getViewportFromUrlParams(url + "#RU", function(urlParams) {
nit: here and below urlParams->viewportPosition

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (left):

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:1099: pp::VarDictionary named_destinations =
engine_->GetNamedDestinations();
nit: This function is now unused, let's remove it entirely

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:143: // Get the named destinations (Page ->
Plugin)
nit: Get the named destination with the given name (Page -> Plugin)

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:146: // Reply with page number of named
destination (Plugin -> Page)
nit: Reply with the page number of the named destination (Plugin -> Page)

[Deepak, 2015-02-16 06:45:38]

Thanks for review.
PTAL.

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:78: url, function(initialViewport) {
On 2015/02/16 02:02:30, raymes wrote:
> nit: initialViewport->viewportPosition 
> nit: could you pull this function out into a separate function in the class
> called onViewportReceived?

Done.

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:17:
this.getNamedDestination_ = getNamedDestination;
On 2015/02/16 02:02:30, raymes wrote:
> nit: getNamedDestinationFunction_

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:59: * @param {Object}
callback function to be called with viewport info.
On 2015/02/16 02:02:30, raymes wrote:
> nit: {Function}

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:74:
viewportPosition['page'] = this.namedDestinations[paramTokens[0]];
On 2015/02/16 02:02:30, raymes wrote:
> Let's not bother caching the namedDestinations here, I don't think it will be
> particularly useful. So we can remove this line and the 4 below and always
just
> go to the plugin.

This is done for testcases. As we don't want to load real page.
And we are putting values in namedDestinations in the test case.
so we are not fetching page number for named destination from plugin for test
cases.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:112: }
On 2015/02/16 02:02:30, raymes wrote:
> Same here - let's remove the above few lines and remove this.namedDestinations
> altogether

Same reason as above.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:126: * namedDestination
and call callback with viewport info.
On 2015/02/16 02:02:31, raymes wrote:
> This is called when a named destination is received and the page number
> corresponding to the request for a named destination is passed in. 

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:127: * @param {number}
pageNumber for viewport.
On 2015/02/16 02:02:31, raymes wrote:
> nit: pageNumber The page corresponding to the named destination requested.

Done.

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:194: new
OpenPDFParamsParser(this.getNamedDestination_.bind(this));
On 2015/02/16 02:02:31, raymes wrote:
> There is some refactoring that would be really nice to do here, but let's
leave
> it for another CL if you are interested. The idea is to create a PluginProxy
> class for communicating with the plugin. Rather than passing in
> this.getNamedDestination_ here, we would pass in the PluginProxy class which
has
> a function getNamedDestination hanging off it.

I agree with you, will do it in-separate CL.
Thanks

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:371: * Callback to fetch namedDestination
from plugin.
On 2015/02/16 02:02:31, raymes wrote:
> nit: Fetches the page number corresponding to the given named destination from
> the plugin. 

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:389: this.streamDetails_.originalUrl,
function(initialViewport) {
On 2015/02/16 02:02:31, raymes wrote:
> nit: viewportPosition

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:429: this.handleURLParams_();
On 2015/02/16 02:02:31, raymes wrote:
> nit: Let's call this instead:
> 
> this.paramsParser_.getViewportFromUrlParams(this.streamDetails_.originalUrl,
> this.handleURLParams_.bind(this);
> 
> Then we can simplify handleURLParams_

Done.

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
File chrome/test/data/pdf/navigator_test.js (left):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
chrome/test/data/pdf/navigator_test.js:89: chrome.test.assertEq(300,
viewport.position.y);
On 2015/02/16 02:02:31, raymes wrote:
> How come this is removed?

This is removed as:
when #ABC is not in paramsParser.namedDestinations.
so then it will fetch that #ABC from the plugin, via
getNamedDestinationFunction_() that function is provided by pdfviewer while
making OpenPDFParamsParser object.

Even if we provide this function while creating OpenPDFParamsParser() then this
callback will get called, and navigator callback will not receive call as that
will happen after receiving message from plugin.
So this case is not relevant here.

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
File chrome/test/data/pdf/params_parser_test.js (right):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
chrome/test/data/pdf/params_parser_test.js:19:
paramsParser.getViewportFromUrlParams(url + "#RU", function(urlParams) {
On 2015/02/16 02:02:31, raymes wrote:
> nit: here and below urlParams->viewportPosition

Done.

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (left):

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:1099: pp::VarDictionary named_destinations =
engine_->GetNamedDestinations();
On 2015/02/16 02:02:31, raymes wrote:
> nit: This function is now unused, let's remove it entirely

Done.

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
File pdf/out_of_process_instance.cc (right):

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:143: // Get the named destinations (Page ->
Plugin)
On 2015/02/16 02:02:31, raymes wrote:
> nit: Get the named destination with the given name (Page -> Plugin)

Done.

https://codereview.chromium.org/918953002/diff/20001/pdf/out_of_process_insta...
pdf/out_of_process_instance.cc:146: // Reply with page number of named
destination (Plugin -> Page)
On 2015/02/16 02:02:31, raymes wrote:
> nit: Reply with the page number of the named destination (Plugin -> Page)

Done.

[raymes, 2015-02-16 22:44:04]

Thanks!

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

https://codereview.chromium.org/918953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:74:
viewportPosition['page'] = this.namedDestinations[paramTokens[0]];
I still don't think I agree - I think we should change it. For test cases we can
use a fake getNamedDestinations function and call onNamedDestinationReceived
directly.

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
File chrome/test/data/pdf/navigator_test.js (left):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
chrome/test/data/pdf/navigator_test.js:39: paramsParser.namedDestinations['UY']
= 2;
instead, we should pass in the function to get the named destination which would
simply call onNamedDestinationReceived with the correct value.

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/nav...
chrome/test/data/pdf/navigator_test.js:89: chrome.test.assertEq(300,
viewport.position.y);
If we do the suggestion above we should be able to keep this test.

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
File chrome/test/data/pdf/params_parser_test.js (right):

https://codereview.chromium.org/918953002/diff/20001/chrome/test/data/pdf/par...
chrome/test/data/pdf/params_parser_test.js:14:
paramsParser.namedDestinations['UY'] = 22;
Same thing here.

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

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:77: this.onViewportReceived_(url);
This isn't what I meant. Please keep this the same as what you had before, but
instead of having an inline function ( function(initialViewport)... please pull
make it out into a separate function

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/open_pdf_params_parser.js (right):

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:13: function
OpenPDFParamsParser(getNamedDestination) {
nit: getNamedDestinationsFunction

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:17:
this.getNamedDestinationFunction_ = getNamedDestination;
nit: this.getNamedDestinationsFunction_ = getNamedDestinationsFunction;

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

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:428: this.streamDetails_.originalUrl,
this.handleURLParams_.bind(this));
nit: 4 space indent

[raymes, 2015-02-17 02:19:23]

Hi Deepak - in the interest of time I have uploaded the CL with changes at
https://codereview.chromium.org/915853004/. Please take this patch and apply it
and upload it here and sammc@ will also be able to review it.

[Deepak, 2015-02-17 07:12:11]

deepak.m1@samsung.com changed reviewers:
+ sammc@chromium.org

[Deepak, 2015-02-17 07:12:12]

Thanks for review.
Changes done as suggested.
PTAL

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

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/navigator.js:77: this.onViewportReceived_(url);
On 2015/02/16 22:44:04, raymes wrote:
> This isn't what I meant. Please keep this the same as what you had before, but
> instead of having an inline function ( function(initialViewport)... please
pull
> make it out into a separate function

Done.

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/pdf/open_pdf_params_parser.js (right):

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:13: function
OpenPDFParamsParser(getNamedDestination) {
On 2015/02/16 22:44:04, raymes wrote:
> nit: getNamedDestinationsFunction

Done.

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:17:
this.getNamedDestinationFunction_ = getNamedDestination;
On 2015/02/16 22:44:04, raymes wrote:
> nit: this.getNamedDestinationsFunction_ = getNamedDestinationsFunction;

Done.

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

https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:428: this.streamDetails_.originalUrl,
this.handleURLParams_.bind(this));
On 2015/02/16 22:44:04, raymes wrote:
> nit: 4 space indent

Done.

[raymes, 2015-02-17 22:42:17]

On 2015/02/17 07:12:12, Deepak wrote:
> Thanks for review.
> Changes done as suggested.
> PTAL
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/pdf/navigator.js (right):
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/pdf/navigator.js:77: this.onViewportReceived_(url);
> On 2015/02/16 22:44:04, raymes wrote:
> > This isn't what I meant. Please keep this the same as what you had before,
but
> > instead of having an inline function ( function(initialViewport)... please
> pull
> > make it out into a separate function
> 
> Done.
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/pdf/open_pdf_params_parser.js (right):
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/pdf/open_pdf_params_parser.js:13: function
> OpenPDFParamsParser(getNamedDestination) {
> On 2015/02/16 22:44:04, raymes wrote:
> > nit: getNamedDestinationsFunction
> 
> Done.
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/pdf/open_pdf_params_parser.js:17:
> this.getNamedDestinationFunction_ = getNamedDestination;
> On 2015/02/16 22:44:04, raymes wrote:
> > nit: this.getNamedDestinationsFunction_ = getNamedDestinationsFunction;
> 
> Done.
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> File chrome/browser/resources/pdf/pdf.js (right):
> 
>
https://codereview.chromium.org/918953002/diff/80001/chrome/browser/resources...
> chrome/browser/resources/pdf/pdf.js:428: this.streamDetails_.originalUrl,
> this.handleURLParams_.bind(this));
> On 2015/02/16 22:44:04, raymes wrote:
> > nit: 4 space indent
> 
> Done.

lgtm

(for the record I'm referencing the additional changes by me and review by
sammc@ here: https://codereview.chromium.org/915853004/)

[raymes, 2015-02-17 22:42:36]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-02-17 22:43:20]

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

[commit-bot: I haz the power, 2015-02-17 22:47:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-17 22:48:20]

Patchset 7 (id:??) landed as
https://crrev.com/9cdcfdfdb5fd39249669a5743f39d739fe05b47a
Cr-Commit-Position: refs/heads/master@{#316685}