Print Preview: Fixing javascript error occuring in the margins code.

Print Preview: Fixing javascript error occuring in the margins code.

BUG=104992
TEST=See bug description.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111177

[kmadhusu, 2011-11-21 19:20:22]

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:542:
this.updatePageData_();
Can you explain why you need to update the page data here?

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:111: if
(!this.pdfLoaded_)
Reuse the getter function.

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:126: if (this.pdfPlugin_)
{
same here

[dpapad, 2011-11-22 02:05:10]

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:542:
this.updatePageData_();
On 2011/11/21 19:20:22, kmadhusu wrote:
> Can you explain why you need to update the page data here?

Yes. cutomMargins_ might not be initialized when this code executes (because of
the checks added in line 586 which are necessary). Therefore they need to be
initialized here before drawing the lines.

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:111: if
(!this.pdfLoaded_)
On 2011/11/21 19:20:22, kmadhusu wrote:
> Reuse the getter function.

Why use the getter within this class when the variable can be accessed directly?
Getters are for clients of that class.

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:126: if (this.pdfPlugin_)
{
On 2011/11/21 19:20:22, kmadhusu wrote:
> same here

See previous comment.

[kmadhusu, 2011-11-22 02:32:26]

lgtm

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/preview_area.js (right):

http://codereview.chromium.org/8592003/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/preview_area.js:111: if
(!this.pdfLoaded_)
On 2011/11/22 02:05:10, dpapad wrote:
> On 2011/11/21 19:20:22, kmadhusu wrote:
> > Reuse the getter function.
> 
> Why use the getter within this class when the variable can be accessed
directly?
> Getters are for clients of that class.

Ofcourse it is not strict rule, it is just my opinion. It is a nit.

[commit-bot: I haz the power, 2011-11-22 16:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8592003/2001

[commit-bot: I haz the power, 2011-11-22 18:26:28]

Change committed as 111177