Print Preview: Making margin selection sticky (part 2/2)

Print Preview: Making margin selection sticky (part 2/2)

This CL makes "Custom" margins sticky across different preview sessions but also within the same preview session.

Here is a summary of all changes.
- Within same preview session: So far selecting "Custom" was
  picking up from whatever the previous selected option was.
  Now, "Custom" remembers the specified margins. To test this
  select "Custom", do some dragging, then change to another
  option and back to "Custom".
- Across preview sessions: Select "Custom", do some dragging,
  then print. Open print preview again, the last used custom
  margins are remembered.
- There was a bunch of messages sent from JS to get various
  info/settings (initiator tab title, measurement system and
  number format, last used margin settings, default printer).
  I created a single message "getInitialSettings" to get all
  at once (they are needed before the 1st preview is is
  requested).

BUG=102446
TEST=See bug description.


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

[dpapad, 2011-11-02 18:13:20]

I am not doing any validation in the backend as of now. After looking at
page_setup.cc it seems that PageSetup::CalculateSizesWithinRect is the place
where a minimum content_area_ can be enforced (instead of 0 as of now). vandebo
is that right?

[kmadhusu, 2011-11-02 20:40:05]

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

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = null;
Since this is a number, can you default to -1? From my understanding, null is
neither 0 or false or undefined.

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = null;
optionToRestore_ => MarginValueToRestore_?

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:205: set
customMargins(margins) {
This function is called only from line 231. Can you inline this function
definition there?

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:226:
setLastUsedMargins: function(lastUsedMarginsType, lastUsedMargins) {
"lastUsedMargins" param is optional. (Document this).

Can you make this function to take a single object as its parameter.

setLastUsedMargins: function (lastUsedMarginSettings) {}

where, lastUsedMarginSettings is an object with 2 attributes.
  lastUsedMarginSettings.type = integer value.
  lastUsedMarginSettings.values = margin_value_object (which has top, left,
right, bottom values)

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:411:
last_used_page_size_margins_ = new printing::PageSizeMargins();
where do you release this object?

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:658: if
(last_used_page_size_margins_ != NULL) {
nit: if (last_used_page_size_margins_) {
}

http://codereview.chromium.org/8351048/diff/4012/printing/printing_context.cc
File printing/printing_context.cc (right):

http://codereview.chromium.org/8351048/diff/4012/printing/printing_context.cc...
printing/printing_context.cc:66: printing::PageSizeMargins page_size_margins;
PageSizeMargins object is not required here. You are actually fetching the page
margins. Can you use PageMargins directly?

extractMarginValues(job_settings, &margins_in_points);

[dpapad, 2011-11-02 21:33:57]

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

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = null;
On 2011/11/02 20:40:05, kmadhusu wrote:
> optionToRestore_ => MarginValueToRestore_?

Margin value is confusing (sounds like it is a value in points which is not), I
would either leave it as is or choose marginType.

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = null;
On 2011/11/02 20:40:05, kmadhusu wrote:
> Since this is a number, can you default to -1? From my understanding, null is
> neither 0 or false or undefined.

Done. In general though !!null returns false and if (null) works as if (false).

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:205: set
customMargins(margins) {
On 2011/11/02 20:40:05, kmadhusu wrote:
> This function is called only from line 231. Can you inline this function
> definition there?

I could but what is the benefit? Having one less function? This way it can be
documented better than when it is inlined.

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:226:
setLastUsedMargins: function(lastUsedMarginsType, lastUsedMargins) {
On 2011/11/02 20:40:05, kmadhusu wrote:
> "lastUsedMargins" param is optional. (Document this).
> 
> Can you make this function to take a single object as its parameter.
> 
> setLastUsedMargins: function (lastUsedMarginSettings) {}
> 
> where, lastUsedMarginSettings is an object with 2 attributes.
>   lastUsedMarginSettings.type = integer value.
>   lastUsedMarginSettings.values = margin_value_object (which has top, left,
> right, bottom values)

Any parameter not specified is initialized to undefined. In that sense all
parameters are optional in javascript. Turning this in an object adds complexity
in print_preview_handler,cc having to create two nested DictionaryValue objects
and I don't see an obvious benefit.

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:411:
last_used_page_size_margins_ = new printing::PageSizeMargins();
On 2011/11/02 20:40:05, kmadhusu wrote:
> where do you release this object?

This object is never realeased (it is an intentional leak, in other words it is
released when the program exits). This is what we do with other sticky settings
(like last_used_printer_name_). Take a look at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:658: if
(last_used_page_size_margins_ != NULL) {
On 2011/11/02 20:40:05, kmadhusu wrote:
> nit: if (last_used_page_size_margins_) {
> }

Done.

http://codereview.chromium.org/8351048/diff/4012/printing/printing_context.cc
File printing/printing_context.cc (right):

http://codereview.chromium.org/8351048/diff/4012/printing/printing_context.cc...
printing/printing_context.cc:66: printing::PageSizeMargins page_size_margins;
On 2011/11/02 20:40:05, kmadhusu wrote:
> PageSizeMargins object is not required here. You are actually fetching the
page
> margins. Can you use PageMargins directly?
> 
> extractMarginValues(job_settings, &margins_in_points);

I am fetching it this way so that I can reuse extractPageSizeMargins. Reusing it
was the reason for adding it in the first place.

[vandebo (ex-Chrome), 2011-11-02 22:36:54]

http://codereview.chromium.org/8351048/diff/4012/printing/page_size_margins.h
File printing/page_size_margins.h (right):

http://codereview.chromium.org/8351048/diff/4012/printing/page_size_margins.h...
printing/page_size_margins.h:25: void extractPageSizeMargins(const
base::DictionaryValue& settings,
getCustomMarginsFromJobSettings?

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = -1;
As we discussed, a better name would be something like forceMarginUIOnPDFLoad

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:179:
print_preview.MarginSettings.setNumberFormatAndMeasurementSystem(
As discussed, this is brittle, save the old value to a variable and invoke it
from the variable.

[kmadhusu, 2011-11-02 23:46:16]

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:411:
last_used_page_size_margins_ = new printing::PageSizeMargins();
On 2011/11/02 21:33:57, dpapad wrote:
> On 2011/11/02 20:40:05, kmadhusu wrote:
> > where do you release this object?
> 
> This object is never realeased (it is an intentional leak, in other words it
is
> released when the program exits). This is what we do with other sticky
settings
> (like last_used_printer_name_). Take a look at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

(repeating our conversation): Instantiate once and reuse it multiple times.

[dpapad, 2011-11-03 00:15:27]

Addressed comments.

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/4012/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:411:
last_used_page_size_margins_ = new printing::PageSizeMargins();
On 2011/11/02 23:46:16, kmadhusu wrote:
> On 2011/11/02 21:33:57, dpapad wrote:
> > On 2011/11/02 20:40:05, kmadhusu wrote:
> > > where do you release this object?
> > 
> > This object is never realeased (it is an intentional leak, in other words it
> is
> > released when the program exits). This is what we do with other sticky
> settings
> > (like last_used_printer_name_). Take a look at
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...
> 
> (repeating our conversation): Instantiate once and reuse it multiple times.

Done. This was meant to use "new" only once. Good catch.

http://codereview.chromium.org/8351048/diff/4012/printing/page_size_margins.h
File printing/page_size_margins.h (right):

http://codereview.chromium.org/8351048/diff/4012/printing/page_size_margins.h...
printing/page_size_margins.h:25: void extractPageSizeMargins(const
base::DictionaryValue& settings,
On 2011/11/02 22:36:54, vandebo wrote:
> getCustomMarginsFromJobSettings?

Done.

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
chrome/browser/resources/print_preview/margin_settings.js:134:
this.optionToRestore_ = -1;
On 2011/11/02 22:36:54, vandebo wrote:
> As we discussed, a better name would be something like forceMarginUIOnPDFLoad

Done.

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/32/chrome/browser/resources/print...
chrome/browser/resources/print_preview/print_preview.js:179:
print_preview.MarginSettings.setNumberFormatAndMeasurementSystem(
On 2011/11/02 22:36:54, vandebo wrote:
> As discussed, this is brittle, save the old value to a variable and invoke it
> from the variable.

Done.

[kmadhusu, 2011-11-03 17:35:38]

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

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:132: // @type
{boolean} True if the margins UI should be diplayed when the next
As per the comments, Margins options will not be shown until the pdf load event
is fired. I think you meant to set the custom margins on PDF Load event. 

forceMarginsUIOnPDFLoad_ => forceCustomMarginsOnPDFLoad_? or something similar.
"forceMarginsUI" seems vague to me.

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:176: function
serializeRequests() {
If the source is pdf, we will not display margin settings. You don't need to
send getNumberFormatAndMeasurementSystem and getLastUsedMarginSettings messages.

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:659: if
(last_used_page_size_margins_) {
You need to check the margins_type_ along with last_used_page_size_margins_.

Consider the scenario:

UserA initiates a print job with custom margins (3,4,5,6). When we come to
HandlePrint() we store these information in last_used_page_size_margins_ object.

After the print job is complete, UserA previews another webpage, since the last
used margin type is custom, we send the custom margin values to the UI. Lets say
the UserA reset the margins to Default and initiate the print job. 

In line 410, you don't override the last_used_page_size_margins_ object because
the current margin type is Default (and not custom).

UserA requests to preview another tab. Now, the user gets the custom margin
setting values (which is not the correct last used margin type and value).

Either delete the last_used_page_size_margins_ object whenever you see a
non-custom margin type or add a condition here to check whether the
last_used_margins_type_ is custom or not.

[vandebo (ex-Chrome), 2011-11-03 18:07:27]

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

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:230: * @param {Object}
lastUsedMargins The last used margins. It is undefined if
nit: It is only used if |lastUsedMarginsType| ==
MarginSettings.MARGINS_VALUE_CUSTOM.

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:176: function
serializeRequests() {
serializeRequests is a pretty generic name...  Maybe setupInitialRequests?

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:409: last_used_margins_type_ =
static_cast<printing::MarginType>(margin_type);
I think we don't want to do this if the source was a PDF...

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:659: if
(last_used_page_size_margins_) {
On 2011/11/03 17:35:38, kmadhusu wrote:
> You need to check the margins_type_ along with last_used_page_size_margins_.
> 
> Consider the scenario:
> 
> UserA initiates a print job with custom margins (3,4,5,6). When we come to
> HandlePrint() we store these information in last_used_page_size_margins_
object.
> 
> After the print job is complete, UserA previews another webpage, since the
last
> used margin type is custom, we send the custom margin values to the UI. Lets
say
> the UserA reset the margins to Default and initiate the print job. 
> 
> In line 410, you don't override the last_used_page_size_margins_ object
because
> the current margin type is Default (and not custom).
> 
> UserA requests to preview another tab. Now, the user gets the custom margin
> setting values (which is not the correct last used margin type and value).

But the margin type is set correctly, if they select custom margins again, we
should give them the custom margins they used last, so we always have to send
it.

[kmadhusu, 2011-11-03 18:28:38]

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:659: if
(last_used_page_size_margins_) {
On 2011/11/03 18:07:27, vandebo wrote:
> On 2011/11/03 17:35:38, kmadhusu wrote:
> > You need to check the margins_type_ along with last_used_page_size_margins_.
> > 
> > Consider the scenario:
> > 
> > UserA initiates a print job with custom margins (3,4,5,6). When we come to
> > HandlePrint() we store these information in last_used_page_size_margins_
> object.
> > 
> > After the print job is complete, UserA previews another webpage, since the
> last
> > used margin type is custom, we send the custom margin values to the UI. Lets
> say
> > the UserA reset the margins to Default and initiate the print job. 
> > 
> > In line 410, you don't override the last_used_page_size_margins_ object
> because
> > the current margin type is Default (and not custom).
> > 
> > UserA requests to preview another tab. Now, the user gets the custom margin
> > setting values (which is not the correct last used margin type and value).
> 
> But the margin type is set correctly, if they select custom margins again, we
> should give them the custom margins they used last, so we always have to send
> it.

Then we should change "last_used_page_size_margins_" =>
"last_used_custom_margin_values_".

[dpapad, 2011-11-03 18:48:27]

Posting some comments, I am still working on the source PDF case. It seems that
the info needed in print_preview_handler.cc to avoid storing margins if not
modifiable is not available at that time (it only comes from the renderer in
OnDidGetPreviewPageCount).

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

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/margin_settings.js:230: * @param {Object}
lastUsedMargins The last used margins. It is undefined if
On 2011/11/03 18:07:27, vandebo wrote:
> nit: It is only used if |lastUsedMarginsType| ==
> MarginSettings.MARGINS_VALUE_CUSTOM.

Done.

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/3015/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:659: if
(last_used_page_size_margins_) {
On 2011/11/03 17:35:38, kmadhusu wrote:
> You need to check the margins_type_ along with last_used_page_size_margins_.
> 
> Consider the scenario:
> 
> UserA initiates a print job with custom margins (3,4,5,6). When we come to
> HandlePrint() we store these information in last_used_page_size_margins_
object.
> 
> After the print job is complete, UserA previews another webpage, since the
last
> used margin type is custom, we send the custom margin values to the UI. Lets
say
> the UserA reset the margins to Default and initiate the print job. 
> 
> In line 410, you don't override the last_used_page_size_margins_ object
because
> the current margin type is Default (and not custom).
> 
> UserA requests to preview another tab. Now, the user gets the custom margin
> setting values (which is not the correct last used margin type and value).
> 
> Either delete the last_used_page_size_margins_ object whenever you see a
> non-custom margin type or add a condition here to check whether the
> last_used_margins_type_ is custom or not. 

Thanks for catching this. One correction to the scenario though, the user would
get the correct margin type value (Default) but would also get 4 extra unneeded
values.

Added a check here and also added a DCHECK for last_used_page_size_margins_ (it
has to be non-null if Custom).

http://codereview.chromium.org/8351048/diff/8002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/8002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:660:
DCHECK(last_used_page_size_margins_);
Let me clarify a bit how the 4 margin values are used. 

- If the last print job was done with custom margins they are sent to the UI and
displayed. 
- If the last print job was NOT done with custom margins these values are not
sent to the UI. 

In any case changing the selection manually to custom in the UI picks up from
whatever is currently displayed (not from previously used settings).

[kmadhusu, 2011-11-09 00:57:54]

Initial set of comments.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:205: * @param
{{marginLeft: number, marginTop: number, marginRight: bottom,
"marginRight: bottom" => "marginBottom: number"

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:229: *     custom
margins have not been used before
nit: 
@param {Object} lastUsedCustomMargins The last used custom margins.
|lastUsedCustomMargins| is undefined if |lastUsedMarginType| is a not a custom
type.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:239:
this.customMargins = lastUsedCustomMargins;
"this.customMargins" => "this.customMargins_"? Missing underscore..

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:320:
this.previousCustomMargins_.copy(this.customMargins_);
Do you need to the same in line 239?

this.customMargins_.copy(lastUsedCustomMargins);

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:587: if
(customMarginsNotSpecified) {
customMarginsNotSpecified variable is not required here.

Just do: if (!this.customMargins_)

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:589:
this.previousCustomMargins_.copy(this.customMargins_);
this.customMargins_ is null here. 

Instead of lines 588 & 589, You can do
this.previeousCustomMargins_ = null;

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:589:
this.previousCustomMargins_.copy(this.customMargins_);
Can you create a helper function to do the following:
Copy(srcMarginsObj, destMarginsObj) {
 if (!destMarginsObj)
   destMarginsObj = new Margins(-1, -1, -1, -1);
 destMarginsObj.copy(srcMarginsObj);
}

I see the same pattern in 3 or more places.

[dpapad, 2011-11-09 03:19:00]

Addressed comments.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:205: * @param
{{marginLeft: number, marginTop: number, marginRight: bottom,
On 2011/11/09 00:57:54, kmadhusu wrote:
> "marginRight: bottom" => "marginBottom: number"

Done.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:229: *     custom
margins have not been used before
On 2011/11/09 00:57:54, kmadhusu wrote:
> nit: 
> @param {Object} lastUsedCustomMargins The last used custom margins.
> |lastUsedCustomMargins| is undefined if |lastUsedMarginType| is a not a custom
> type.

As discussed in IM, this is not true anymore, lastUsedCustomMargins is
|undefined| only if custom margins have not been used since the browser was
launched, not just in the exact previous print preview session).

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:239:
this.customMargins = lastUsedCustomMargins;
On 2011/11/09 00:57:54, kmadhusu wrote:
> "this.customMargins" => "this.customMargins_"? Missing underscore..

This is on purpose, leaving the underscore out, causes the set customMargins()
function to execute. This is not just an assignment.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:320:
this.previousCustomMargins_.copy(this.customMargins_);
On 2011/11/09 00:57:54, kmadhusu wrote:
> Do you need to the same in line 239?
> 
> this.customMargins_.copy(lastUsedCustomMargins);

It is not needed there, because a preview request will be issued from
print_preview.js after |setLastUsedMargins| returns. |previousCustomMargins| are
needed only when |MarginSettings.prototype.requestPreviewIfNeeded| is called.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:587: if
(customMarginsNotSpecified) {
On 2011/11/09 00:57:54, kmadhusu wrote:
> customMarginsNotSpecified variable is not required here.
> 
> Just do: if (!this.customMargins_)

this.updatePageData_ will initialize |this.customMargins_| if not already
initialized, therefore I need this boolean, in other words at this line
!this.customMargins is always false.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:589:
this.previousCustomMargins_.copy(this.customMargins_);
On 2011/11/09 00:57:54, kmadhusu wrote:
> this.customMargins_ is null here. 
> 
> Instead of lines 588 & 589, You can do
> this.previeousCustomMargins_ = null;

|this.customMargins_| is not null here, explained in previous comment.

http://codereview.chromium.org/8351048/diff/23006/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:589:
this.previousCustomMargins_.copy(this.customMargins_);
On 2011/11/09 00:57:54, kmadhusu wrote:
> Can you create a helper function to do the following:
> Copy(srcMarginsObj, destMarginsObj) {
>  if (!destMarginsObj)
>    destMarginsObj = new Margins(-1, -1, -1, -1);
>  destMarginsObj.copy(srcMarginsObj);
> }
> 
> I see the same pattern in 3 or more places. 

Done. Created a clone() function which is much more flexible.

[kmadhusu, 2011-11-09 19:26:11]

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_message_handler.h (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_message_handler.h:46: void
OnRequestPrintPreview(bool is_modifiable);
nit: is_modifiable => source_is_modifiable

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:668: if
(this.forceMarginsUIOnPDFLoad_) {
Can you add new tests to verify the sticky behavior?

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:182: function
setInitialSettings(initiatorTabTitle, numberFormat, measurementSystem,
Instead of 8 arguments, please send an object.

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:606: function
setDefaultPrinter(printer_name, cloudPrintData) {
style nit: printer_name => printerName

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:657: string16 number_format;
if (print_preview_ui->is_modifiable()) {
   lines 656 - 663
}

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:670: std::vector<const
base::Value*> args;
Instead of a vector, Can you send a dictionary.

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:139: bool is_modifiable_;
nit: is_modifiable_ => is_preview_modifiable

http://codereview.chromium.org/8351048/diff/23023/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/8351048/diff/23023/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1287:
print_preview_context_.IsModifiable()));
nit: this can fit in the previous line.

[vandebo (ex-Chrome), 2011-11-09 19:29:07]

Are you ready for further review?  The last status I saw was that you were still
working on PDF sources.

[dpapad, 2011-11-09 19:31:12]

On 2011/11/09 19:29:07, vandebo wrote:
> Are you ready for further review?  The last status I saw was that you were
still
> working on PDF sources.

Yes please review. I resolved the case where the source is PDF. The only
outstanding issues are preserving the margins when changing printers and layout
mode. I think this should be addressed in a separate CL.

[dpapad, 2011-11-09 23:42:38]

Addressed kamdhusu's comments. Still looking on adding backend tests for sticky
margins.

[vandebo (ex-Chrome), 2011-11-10 18:39:14]

LGTM

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:314: if
(this.previousCustomMargins_ &&
Can you incorporate this check into the isEqual function?

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:671:
GetNumberFormatAndMeasurementSystem(&number_format, &measurement_system);
Why not pass &initial_settings to GetNumberFormatAndMeasurementSystem and mirror
GetLastUsedMarginSettings ?

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
printing/print_job_constants.cc:124: const char kSettingInitiatorTabTitle[] =
"initiatorTabTitle";
Add comments describing what each of these is.

[kmadhusu, 2011-11-10 18:39:56]

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:176:
console.log(initialSettings);
nit: remove console.log

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
printing/print_job_constants.h:50: PRINTING_EXPORT extern const char
kSettingNumberFormat[];
Declare these variables in print_preview_handler.cc

[dpapad, 2011-11-10 20:04:01]

Addressed comments and also fixed a bug (see below).

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/margin_settings.js (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/margin_settings.js:314: if
(this.previousCustomMargins_ &&
On 2011/11/10 18:39:14, vandebo wrote:
> Can you incorporate this check into the isEqual function?

Done.

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:176:
console.log(initialSettings);
On 2011/11/10 18:39:56, kmadhusu wrote:
> nit: remove console.log

Done.

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/28021/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:671:
GetNumberFormatAndMeasurementSystem(&number_format, &measurement_system);
On 2011/11/10 18:39:14, vandebo wrote:
> Why not pass &initial_settings to GetNumberFormatAndMeasurementSystem and
mirror
> GetLastUsedMarginSettings ?

Done.

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
printing/print_job_constants.cc:124: const char kSettingInitiatorTabTitle[] =
"initiatorTabTitle";
On 2011/11/10 18:39:14, vandebo wrote:
> Add comments describing what each of these is.

Done.

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8351048/diff/28021/printing/print_job_constant...
printing/print_job_constants.h:50: PRINTING_EXPORT extern const char
kSettingNumberFormat[];
On 2011/11/10 18:39:56, kmadhusu wrote:
> Declare these variables in print_preview_handler.cc

Done.

http://codereview.chromium.org/8351048/diff/40002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/8351048/diff/40002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:304: var resetMargins =
lastPreviewRequestID != initialPreviewRequestID;
When we first set the printer, we do not want to reset the margins (it will
override the sticky margins). When switching printers after the 1st preview we
do want to reset.

[kmadhusu, 2011-11-10 20:45:19]

lgtm

LGTM

http://codereview.chromium.org/8351048/diff/40002/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8351048/diff/40002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:101: const char
kCloudPrintData[] = "cloudPrintData";
nit: Declare the constants at the top of the namespace.

[kmadhusu, 2011-11-10 20:49:51]

dpapad: Are you planning to add tests for this behavior?

[dpapad, 2011-11-11 22:19:09]

Added a unit_test.

[Lei Zhang, 2011-11-11 22:56:29]

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:63:
settings.SetDouble(ss.str(), 25.5);
make the values constants?

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:78: args.Append(new
base::StringValue(json_string)); // |args| takes ownership.
nit: 2 spaces

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:95:
printing::BackgroundPrintingManager* bg_printing_manager =
Might want a comment to explain why this is here and why you are deleting
|preview_tab|.

[kmadhusu, 2011-11-11 23:39:40]

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:24:
TEST_F(PrintPreviewHandlerTest, HandlePrintTest) {
Your test case is fine. But you can add more information to it. 

I expected the following sequence:
1. Open a initiator tab.
2. Create a preview tab.
3. Call HandlePrint().
4. Delete the hidden preview tab.
5. Preview the initiator tab again.
6. Query/Test the last used margin type and custom margins value. 

You can create 3 test cases using this sequence:

Test case 1:
User set default margins during the first print request.
Expect the default margin type in the next preview tab.

Test case 2:
User set custom margins during the first print request.
Expect the same custom margins type in the next preview tab.

Test case 3:
User set custom margins during the first print request.
User set default margins during the next print request.
During the next preview request, the last used margin type should be default but
we would still retain the last used margin settings.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:61: std::stringstream
ss;
Any special reason to use the stringstream?

You can use the Set() member function of DictionaryValue class to add a
dictionary to a DictionaryValue.

[dpapad, 2011-11-14 16:31:54]

Added 3 tests for testing that settings are saved correctly, and 2 for testing
that they are retrieved correctly.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:24:
TEST_F(PrintPreviewHandlerTest, HandlePrintTest) {
On 2011/11/11 23:39:40, kmadhusu wrote:
> Your test case is fine. But you can add more information to it. 
> 
> I expected the following sequence:
> 1. Open a initiator tab.
> 2. Create a preview tab.
> 3. Call HandlePrint().
> 4. Delete the hidden preview tab.
> 5. Preview the initiator tab again.
> 6. Query/Test the last used margin type and custom margins value. 
> 
> You can create 3 test cases using this sequence:
> 
> Test case 1:
> User set default margins during the first print request.
> Expect the default margin type in the next preview tab.
> 
> Test case 2:
> User set custom margins during the first print request.
> Expect the same custom margins type in the next preview tab.
> 
> Test case 3:
> User set custom margins during the first print request.
> User set default margins during the next print request.
> During the next preview request, the last used margin type should be default
but
> we would still retain the last used margin settings. 

Done.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:61: std::stringstream
ss;
On 2011/11/11 23:39:40, kmadhusu wrote:
> Any special reason to use the stringstream?
> 
> You can use the Set() member function of DictionaryValue class to add a
> dictionary to a DictionaryValue.

Done. I was following the instructions on the top of values.h to create nested
DictionaryValues. Set() works as well and it is less code.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:63:
settings.SetDouble(ss.str(), 25.5);
On 2011/11/11 22:56:30, Lei Zhang wrote:
> make the values constants?

Done.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:78: args.Append(new
base::StringValue(json_string)); // |args| takes ownership.
On 2011/11/11 22:56:30, Lei Zhang wrote:
> nit: 2 spaces

Done.

http://codereview.chromium.org/8351048/diff/45001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:95:
printing::BackgroundPrintingManager* bg_printing_manager =
On 2011/11/11 22:56:30, Lei Zhang wrote:
> Might want a comment to explain why this is here and why you are deleting
> |preview_tab|.

Done.

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:148:
PrintPreviewHandler::last_used_page_size_margins_ = NULL;
Deleting this instead of just setting it to null gives me an error, still trying
to figure that out.

[vandebo (ex-Chrome), 2011-11-14 17:05:39]

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:184:
ASSERT_TRUE(!PrintPreviewHandler::last_used_page_size_margins_);
ASSERT_FALSE

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:198: 
Check custom margins here too.

[dpapad, 2011-11-14 17:20:14]

Addressed vandebo's comments.

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:184:
ASSERT_TRUE(!PrintPreviewHandler::last_used_page_size_margins_);
On 2011/11/14 17:05:39, vandebo wrote:
> ASSERT_FALSE

Done.

http://codereview.chromium.org/8351048/diff/51001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:198: 
On 2011/11/14 17:05:39, vandebo wrote:
> Check custom margins here too.

Done.

[vandebo (ex-Chrome), 2011-11-14 17:25:35]

LGTM

[dpapad, 2011-11-14 19:59:27]

Fixed a potential memory leak in the test code. Waiting for kmadhusu's reLGTM
before landing.

[Lei Zhang, 2011-11-14 20:00:49]

LGTM with a couple small nits.

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:190: const double
margin_top = 125.5;
nit: constants are kFoo.

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:217:
TEST_F(PrintPreviewHandlerTest, TestGetLastUsedMarginSettingsCustom) {
nit: FooTest.TestBar -> FooTest.Bar.

[kmadhusu1, 2011-11-14 20:06:04]

FYI. I am on sheriff duty today. Please expect a delay in reviewing your
CL.

Thanks.

On Mon, Nov 14, 2011 at 12:00 PM, <thestig@chromium.org> wrote:

> LGTM with a couple small nits.
>
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc>
> File chrome/browser/ui/webui/print_**preview_handler_unittest.cc (right):
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc#**newcode190<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc#newcode190>
> chrome/browser/ui/webui/print_**preview_handler_unittest.cc:**190: const
> double margin_top = 125.5;
> nit: constants are kFoo.
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc#**newcode217<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc#newcode217>
> chrome/browser/ui/webui/print_**preview_handler_unittest.cc:**217:
> TEST_F(**PrintPreviewHandlerTest, TestGetLastUsedMarginSettingsC**ustom) {
> nit: FooTest.TestBar -> FooTest.Bar.
>
>
http://codereview.chromium.**org/8351048/<http://codereview.chromium.org/8351...
>

[dpapad, 2011-11-14 23:19:58]

Addressed nits.

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:190: const double
margin_top = 125.5;
On 2011/11/14 20:00:49, Lei Zhang wrote:
> nit: constants are kFoo.

Done.

http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:217:
TEST_F(PrintPreviewHandlerTest, TestGetLastUsedMarginSettingsCustom) {
On 2011/11/14 20:00:49, Lei Zhang wrote:
> nit: FooTest.TestBar -> FooTest.Bar.

Done.

[kmadhusu1, 2011-11-15 00:08:40]

dpapad: Don't wait for my lgtm. If thestig@ is blocked on this CL, Go ahead
and commit your changes.  I will review them later and you can address the
comments in the separate CL.

On Mon, Nov 14, 2011 at 3:19 PM, <dpapad@chromium.org> wrote:

> Addressed nits.
>
>
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc>
> File chrome/browser/ui/webui/print_**preview_handler_unittest.cc (right):
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc#**newcode190<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc#newcode190>
> chrome/browser/ui/webui/print_**preview_handler_unittest.cc:**190: const
> double margin_top = 125.5;
> On 2011/11/14 20:00:49, Lei Zhang wrote:
>
>> nit: constants are kFoo.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/8351048/diff/56001/chrome/**
>
browser/ui/webui/print_**preview_handler_unittest.cc#**newcode217<http://codereview.chromium.org/8351048/diff/56001/chrome/browser/ui/webui/print_preview_handler_unittest.cc#newcode217>
> chrome/browser/ui/webui/print_**preview_handler_unittest.cc:**217:
> TEST_F(**PrintPreviewHandlerTest, TestGetLastUsedMarginSettingsC**ustom) {
> On 2011/11/14 20:00:49, Lei Zhang wrote:
>
>> nit: FooTest.TestBar -> FooTest.Bar.
>>
>
> Done.
>
>
http://codereview.chromium.**org/8351048/<http://codereview.chromium.org/8351...
>

[Lei Zhang, 2011-11-15 00:12:04]

http://codereview.chromium.org/8351048/diff/57022/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8351048/diff/57022/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:146: friend class
PrintPreviewHandlerTest;
Sorry to keep bugging you with nits, but this should be up by line 121.

[dpapad, 2011-11-15 00:55:04]

Waiting for tryjobs to complete.

http://codereview.chromium.org/8351048/diff/57022/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_ui.h (right):

http://codereview.chromium.org/8351048/diff/57022/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_ui.h:146: friend class
PrintPreviewHandlerTest;
On 2011/11/15 00:12:05, Lei Zhang wrote:
> Sorry to keep bugging you with nits, but this should be up by line 121.

No problem. Done.

[kmadhusu, 2011-11-15 02:31:02]

lgtm with a nit.

http://codereview.chromium.org/8351048/diff/58001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler_unittest.cc (right):

http://codereview.chromium.org/8351048/diff/58001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler_unittest.cc:20: //typedef
BrowserWithTestWindowTest PrintPreviewHandlerTest;
Remove this comment.

[commit-bot: I haz the power, 2011-11-15 02:33:33]

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

[commit-bot: I haz the power, 2011-11-15 03:45:39]

Change committed as 110035