OOP PDF - Add OpenPDFParamsParser class

OOP PDF - Add OpenPDFParamsParser class

This patch introduces OpenPDFParamsParser class which is responsible
to parse open pdf parameters and set them in a dictionary. This
dictionary will be used to set initial viewport settings when opening
pdf document.

BUG=303491
Committed: https://crrev.com/53b6e7a7c94dfdd0b2b5b8632776d5978bf46b91
Cr-Commit-Position: refs/heads/master@{#292360}

[Nikhil, 2014-08-26 11:23:22]

n.bansal@samsung.com changed reviewers:
+ raymes@chromium.org

[Nikhil, 2014-08-26 11:25:06]

PTAL at initial implementation of OpenPDFParamsParser class.

If you've any review comments, then please let me know. I'll incorporate them.

Thanks!

[raymes, 2014-08-27 01:02:24]

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:8: <include src="pdf_open_params_parser.js">
nit: can we call this file open_pdf_params_parser.js?

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf_open_params_parser.js:52:
this.parseOpenPDFParams_();
Why not just do this on construction? Then you wouldn't need a getter here, you
could just expose the viewportSettings member directly.

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/viewport.js:185: },
I think we don't need this setter, we can just call the member
"this.initialViewport" and set it directly.

Also, I think it would be better to hook this up before landing this CL.

[Nikhil, 2014-08-27 10:16:10]

Thanks raymes@ for the quick review! I've incorporated the review comments.
PTAL.

[*] Since we don't need to update setDocumentDimensions() now, I think this
patch can be submitted once it looks good enough.

[*] Basically, this patch introduces new class to parse open pdf parameters (and
to remove code from pdf.js). "page" feature which is currently supported in OOP
will continue to work with this patch.

Please let me know if you've any review comments.

Thanks!

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:8: <include src="pdf_open_params_parser.js">
On 2014/08/27 01:02:24, raymes wrote:
> nit: can we call this file open_pdf_params_parser.js?

Done.

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf_open_params_parser.js:52:
this.parseOpenPDFParams_();
On 2014/08/27 01:02:24, raymes wrote:
> Why not just do this on construction? Then you wouldn't need a getter here,
you
> could just expose the viewportSettings member directly.

Done.

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

https://codereview.chromium.org/477933003/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/viewport.js:185: },
On 2014/08/27 01:02:24, raymes wrote:
> I think we don't need this setter, we can just call the member
> "this.initialViewport" and set it directly.
> 
> Also, I think it would be better to hook this up before landing this CL.

Done.

[raymes, 2014-08-28 01:50:33]

lgtm, thanks!

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

https://codereview.chromium.org/477933003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:14: this.viewportSettings
= {};
ditto here, maybe we can just call this "this.urlParams"

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

https://codereview.chromium.org/477933003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:157: this.openParams_ =
paramsParser.viewportSettings;
maybe we can call this this.urlParams_ (a bit more descriptive than openParams)

[Nikhil, 2014-08-28 05:07:37]

Thanks for the quick review, Raymes :)

I've updated the variable name like you suggested. PTAL.

Please correct me if I'm wrong, I'm assuming we don't need to mention newly
added Js file anywhere else in codebase (sorry, I'm new to this).

Thanks!

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

https://codereview.chromium.org/477933003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/open_pdf_params_parser.js:14: this.viewportSettings
= {};
On 2014/08/28 01:50:32, raymes wrote:
> ditto here, maybe we can just call this "this.urlParams"

Done.

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

https://codereview.chromium.org/477933003/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:157: this.openParams_ =
paramsParser.viewportSettings;
On 2014/08/28 01:50:32, raymes wrote:
> maybe we can call this this.urlParams_ (a bit more descriptive than
openParams)

Done.

[Nikhil, 2014-08-28 05:11:30]

n.bansal@samsung.com changed reviewers:
+ thestig@chromium.org

[Nikhil, 2014-08-28 05:13:55]

thestig@ - Could you please take a look? I need OWNERS review to land this.
Thanks.

[Lei Zhang, 2014-08-28 05:45:55]

lgtm

[Nikhil, 2014-08-28 06:56:32]

Thanks raymes@ and thestig@ for reviewing this! I'm going to submit this now :)

[Nikhil, 2014-08-28 06:56:41]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-08-28 06:57:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/477933003/60001

[commit-bot: I haz the power, 2014-08-28 11:00:00]

Committed patchset #4 (id:60001) as 539d26b7003ebf66ac3ee936cfb0e1f9b667c09f

[commit-bot: I haz the power, 2014-09-10 02:58:51]

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