PrintPreview: [LINUX] Update the margin values after flipping the paper orientation.

PrintPreview: [LINUX] Update the margin values after flipping the paper orientation.

BUG=101419
TEST=Please refer to bug description.


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

[vandebo (ex-Chrome), 2011-11-02 01:35:37]

Instead of doing this at this let, lets save which kind of margins are used
(set/forced) in the page setup so that FlipOrientation can do the right
thing.

On Tue, Nov 1, 2011 at 6:26 PM, <kmadhusu@chromium.org> wrote:

> Reviewers: vandebo,
>
> Description:
> PrintPreview: [LINUX] Always restore the margin values after flipping the
> paper
> orientation.
>
> BUG=101419
> TEST=Please refer to bug description.
>
>
> Please review this at
http://codereview.chromium.**org/8351063/<http://codereview.chromium.org/8351...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M printing/print_settings.cc
>
>
> Index: printing/print_settings.cc
> diff --git a/printing/print_settings.cc b/printing/print_settings.cc
> index 70ace6dcf9da36ad74f3b6a60a1ede**083fecf2b3..**
> 9acff9eb494a5c9279b9a49d069b5a**fa56f38761 100644
> --- a/printing/print_settings.cc
> +++ b/printing/print_settings.cc
> @@ -241,7 +241,13 @@ int PrintSettings::NewCookie() {
>  void PrintSettings::SetOrientation(**bool landscape) {
>   if (landscape_ != landscape) {
>     landscape_ = landscape;
> +    // Store the margins and restore it after FlipOrientation().
> +    PageMargins margins = page_setup_device_units_.**effective_margins();
>     page_setup_device_units_.**FlipOrientation();
> +    if (margin_type == DEFAULT_MARGINS || margin_type ==
> PRINTABLE_AREA_MARGINS)
> +      page_setup_device_units_.**SetRequestedMargins(margins);
> +    else
> +      page_setup_device_units_.**ForceRequestedMargins(margins)**;
>   }
>  }
>
>
>
>

[vandebo (ex-Chrome), 2011-11-02 16:13:06]

And we should add a test too.

[vandebo (ex-Chrome), 2011-11-02 20:26:03]

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.cc#newco...
printing/page_setup.cc:123: if (has_forced_margins_value)
With the addition of has_forced_margins_value, I think we should abstract this,
i.e. add private UpdateValues() that would be called from Init,
Set/ForceRequestedMargins and would call CalculateSizesWithRect.  This would
make FlipOrientation not need to be aware of forced/non-forced margins.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode20
printing/page_setup.h:20: PageMargins(const PageMargins& page_margins, bool
rotate_margins);
How about a rotate method instead of a modified copy constructor?

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode94
printing/page_setup.h:94: // Store whether the |requested_margins_| were set
forcefully.
nit: were set forcefully -> respect printable_area_

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode95
printing/page_setup.h:95: bool has_forced_margins_value;
nit: forced_margins_;

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup_unittest.cc
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup_unittest...
printing/page_setup_unittest.cc:219: setup.FlipOrientation();
Let's check all the values (physical_size_, printable_area_, overlay_area_,
content_area_, requested_margins_) before/after the flip since there isn't
already a test of FlipOrientation.

[vandebo (ex-Chrome), 2011-11-02 20:29:17]

Update the issue description?  Is this really Linux specific?

[kmadhusu, 2011-11-02 23:15:03]

As we discussed this is LINUX specific. I am leaving the description as it is.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.cc#newco...
printing/page_setup.cc:123: if (has_forced_margins_value)
On 2011/11/02 20:26:03, vandebo wrote:
> With the addition of has_forced_margins_value, I think we should abstract
this,
> i.e. add private UpdateValues() that would be called from Init,
> Set/ForceRequestedMargins and would call CalculateSizesWithRect.  This would
> make FlipOrientation not need to be aware of forced/non-forced margins.

Done.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode20
printing/page_setup.h:20: PageMargins(const PageMargins& page_margins, bool
rotate_margins);
On 2011/11/02 20:26:03, vandebo wrote:
> How about a rotate method instead of a modified copy constructor?

Done.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode94
printing/page_setup.h:94: // Store whether the |requested_margins_| were set
forcefully.
On 2011/11/02 20:26:03, vandebo wrote:
> nit: were set forcefully -> respect printable_area_

Done.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup.h#newcode95
printing/page_setup.h:95: bool has_forced_margins_value;
On 2011/11/02 20:26:03, vandebo wrote:
> nit: forced_margins_;

Done.

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup_unittest.cc
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/3005/printing/page_setup_unittest...
printing/page_setup_unittest.cc:219: setup.FlipOrientation();
On 2011/11/02 20:26:03, vandebo wrote:
> Let's check all the values (physical_size_, printable_area_, overlay_area_,
> content_area_, requested_margins_) before/after the flip since there isn't
> already a test of FlipOrientation.

Done.

[vandebo (ex-Chrome), 2011-11-03 17:26:35]

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:62: text_height_ = 0;
clear should probably reset forced_margins_

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:89: CalculateSizesWithinRect(printable_area_,
text_height_);
This should call SetRequestedMarginsANdUpdateValues.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:122: if (physical_size_.width() &&
physical_size_.height())
Pull this 'if' up one scope so you don't have to repeat it.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h#newcode20
printing/page_setup.h:20: void RotatePageMarginsAndUpdateCurrentValue(const
PageMargins& page_margins);
I think a simple "Rotate(void)" method, that modifies the current class will
work, no?

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h#newcode97
printing/page_setup.h:97: // True when the |requested_margins_| respects
|printable_area_| else false.
True when |effective_margins_|...

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest.cc
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:207: gfx::Rect printable_area(3, 3, 92, 50);
Try to use all unique values so that things don't just happen to work.  i.e.
you've used 3 twice in printable area as well as text height.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:214: // Calculate the effective margins.
I'd rather just hardcode the expected values than calculate them.  It makes the
test easier to read.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:246: page_size.ToString() << " " <<
printable_area.ToString() <<
won't EXPECT_EQ print the observed and expected values if they differ, or does
it not work when it's a class?

[kmadhusu, 2011-11-03 20:46:54]

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:62: text_height_ = 0;
On 2011/11/03 17:26:35, vandebo wrote:
> clear should probably reset forced_margins_

oops. Fixed.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:89: CalculateSizesWithinRect(printable_area_,
text_height_);
On 2011/11/03 17:26:35, vandebo wrote:
> This should call SetRequestedMarginsANdUpdateValues.

Done.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.cc#newco...
printing/page_setup.cc:122: if (physical_size_.width() &&
physical_size_.height())
On 2011/11/03 17:26:35, vandebo wrote:
> Pull this 'if' up one scope so you don't have to repeat it.

if (physical_size_....) {
   if (forced_margins_)
     Calculate...
   else
     Calculate..
}

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h#newcode20
printing/page_setup.h:20: void RotatePageMarginsAndUpdateCurrentValue(const
PageMargins& page_margins);
On 2011/11/03 17:26:35, vandebo wrote:
> I think a simple "Rotate(void)" method, that modifies the current class will
> work, no?

Yes. It will work perfectly. Fixed.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup.h#newcode97
printing/page_setup.h:97: // True when the |requested_margins_| respects
|printable_area_| else false.
On 2011/11/03 17:26:35, vandebo wrote:
> True when |effective_margins_|...

Done.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest.cc
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:207: gfx::Rect printable_area(3, 3, 92, 50);
On 2011/11/03 17:26:35, vandebo wrote:
> Try to use all unique values so that things don't just happen to work.  i.e.
> you've used 3 twice in printable area as well as text height.

Done.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:214: // Calculate the effective margins.
On 2011/11/03 17:26:35, vandebo wrote:
> I'd rather just hardcode the expected values than calculate them.  It makes
the
> test easier to read.

Done.

http://codereview.chromium.org/8351063/diff/6002/printing/page_setup_unittest...
printing/page_setup_unittest.cc:246: page_size.ToString() << " " <<
printable_area.ToString() <<
On 2011/11/03 17:26:35, vandebo wrote:
> won't EXPECT_EQ print the observed and expected values if they differ, or does
> it not work when it's a class?

I removed these additional info. It works fine.

[vandebo (ex-Chrome), 2011-11-03 21:12:57]

LGTM

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.cc#newc...
printing/page_setup.cc:48: PageSetup::PageSetup() : forced_margins_(false) {
nit: not needed because it's in Clear

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.h#newco...
printing/page_setup.h:71: void SetRequestedMarginsAndUpdateValues(const
PageMargins& requested_margins);
nit: SetRequestedMarginsAndCalculateSizes

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup_unittes...
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup_unittes...
printing/page_setup_unittest.cc:207: gfx::Rect printable_area(8, 9, 92, 50);
nit: 100-92 = 8

[kmadhusu, 2011-11-03 22:37:39]

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.cc
File printing/page_setup.cc (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.cc#newc...
printing/page_setup.cc:48: PageSetup::PageSetup() : forced_margins_(false) {
On 2011/11/03 21:12:57, vandebo wrote:
> nit: not needed because it's in Clear

Done.

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.h
File printing/page_setup.h (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup.h#newco...
printing/page_setup.h:71: void SetRequestedMarginsAndUpdateValues(const
PageMargins& requested_margins);
On 2011/11/03 21:12:57, vandebo wrote:
> nit: SetRequestedMarginsAndCalculateSizes

Done.

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup_unittes...
File printing/page_setup_unittest.cc (right):

http://codereview.chromium.org/8351063/diff/10003/printing/page_setup_unittes...
printing/page_setup_unittest.cc:207: gfx::Rect printable_area(8, 9, 92, 50);
On 2011/11/03 21:12:57, vandebo wrote:
> nit: 100-92 = 8

Ignoring this..

[commit-bot: I haz the power, 2011-11-03 22:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/8351063/9007

[commit-bot: I haz the power, 2011-11-04 00:29:23]

Change committed as 108598