Added Header and Footer support using Skia

Added Header and Footer support in Linux, Windows and Mac for Skia


BUG=67514
TEST=
In the preview tab, note added options for printing headers and footers.  Toggle with the checkbox and ensure that the correct headers and footers are displayed.

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

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

[Aayush Kumar, 2011-07-12 20:35:57]

I have uploaded this CL just to get some feedback and make sure that I am
working in the right direction.
These are some of the issues/TODOs I am aware of and I am working them.
(i) This code will compile and has been tested only on Linux.
(ii) Remove previous redundant code for printing headers and footers (I'll
probably make a separate CL for this.)
(iii) Given that we have hard-coded positions for the strings, I might decide to
customize and increase the maximum length of the strings that can be printed in
the headers and footers.
(iv) There are a few private methods I have added to print_web_view_helper and
the implementation is only in the print_web_view_helper_linux.cc file.  I
suspect that a lot of this code will move to print_web_view_helper.cc.

[Aayush Kumar, 2011-07-12 20:52:34]

Added Lei to the reviewer's list :)

[Lei Zhang, 2011-07-12 22:04:49]

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

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:339: function
isHeaderFooter() {
how about hasHeadersFooters() ?

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (left):

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:17: #include "base/values.h"
Why did you remove this?

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

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:454: &isHeaderFooter))
nit: indentation
nit: braces around NOTREACHED()

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:468: if (entry)
nit: braces

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:478: header_footer_info));
nit: indentation

http://codereview.chromium.org/7348010/diff/1020/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/common/print_messages...
chrome/common/print_messages.h:169: DictionaryValue /* header_footer_info */)
Aren't the header/footer info part of the print preview |settings| ? You can
have a dictionary in a dictionary.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:160: header_footer_interstice(7.2) {
where does this magical 7.2 value come from?

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:344: SkScalar margin_left_in_points,
Consider using some additional data structures. When your method has 12 methods,
it's getting pretty ugly.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:123: 
nit: Why the blank spaces? Try reviewing your own code first and weed out these
silly things so others don't waste time on them.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.h#n...
ui/base/text/text_elider.h:62: string16 ElideUrl(const GURL& url,
You shouldn't expose the internal implementation. Same for ElideText.

[kmadhusu, 2011-07-13 01:49:07]

As we discussed, you can revert the changes to print_dialog_gtk.cc,
printing_message_filter.cc, print_settings_initializer_gtk.cc.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:85: <div
id="options-option" class="two-column option visible">
Where do you use this div id?

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:87: <div>
Why do you have two <div> tags here?

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:89: <input
id="header-footer" type="checkbox" name="header-footer"
Do you need this name attribute?

[kmadhusu, 2011-07-13 18:47:52]

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

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:453: if
(!(settings.get())->GetBoolean(printing::kSettingHeaderFooter,
"(settings.get())->" == "settings->"

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:120: const DictionaryValue&
header_footer_info);
nit: Fix indentation.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:303: enum HorizontalHeaderFooterPosition
{
Move these enums to print_job_constants.h &.cc and the new functions to an
unnamed namespace.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:338: // TODO(aayushkumar): Have a
seperate function for printing Headers and Footer
Implement this TODO.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:388: UTF8ToUTF16("/") +
nit: Fix indentation.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:101: void* font_or_paint,//const gfx::Font& font,
Is this comment intentional?

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:112: if (available_width >=  
measureStringWidth(elided_path,
nit: remove extra spaces.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:141: GFX_Font);
Fix indentation

[Aayush Kumar, 2011-07-13 21:52:16]

In this update, I have added headers and footers support for windows.  (This may
still not compile and run on mac)
To achieve that I have moved a lot of the code to print_web_view_helper.cc from
print_web_view_helper_linux.cc

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:85: <div
id="options-option" class="two-column option visible">
On 2011/07/13 01:49:07, kmadhusu wrote:
> Where do you use this div id?

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:87: <div>
On 2011/07/13 01:49:07, kmadhusu wrote:
> Why do you have two <div> tags here?

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.html:89: <input
id="header-footer" type="checkbox" name="header-footer"
On 2011/07/13 01:49:07, kmadhusu wrote:
> Do you need this name attribute?

Done.

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

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:339: function
isHeaderFooter() {
On 2011/07/12 22:04:50, Lei Zhang wrote:
> how about hasHeadersFooters() ?

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (left):

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:17: #include "base/values.h"
On 2011/07/12 22:04:50, Lei Zhang wrote:
> Why did you remove this?

Done.

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

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:453: if
(!(settings.get())->GetBoolean(printing::kSettingHeaderFooter,
On 2011/07/13 18:47:53, kmadhusu wrote:
> "(settings.get())->" == "settings->"

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:454: &isHeaderFooter))
On 2011/07/12 22:04:50, Lei Zhang wrote:
> nit: indentation
> nit: braces around NOTREACHED()

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:468: if (entry)
On 2011/07/12 22:04:50, Lei Zhang wrote:
> nit: braces

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:478: header_footer_info));
On 2011/07/12 22:04:50, Lei Zhang wrote:
> nit: indentation

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/common/print_messages...
chrome/common/print_messages.h:169: DictionaryValue /* header_footer_info */)
As discussed, I decided to have them as a separate parameter since the print
preview settings are passed along to the printer settings and a bunch of other
places which isn't really required for the header_footer info strings.
On 2011/07/12 22:04:50, Lei Zhang wrote:
> Aren't the header/footer info part of the print preview |settings| ? You can
> have a dictionary in a dictionary.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.cc:160: header_footer_interstice(7.2) {
Moved declaration of constant to job_constants.
On 2011/07/12 22:04:50, Lei Zhang wrote:
> where does this magical 7.2 value come from?

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:120: const DictionaryValue&
header_footer_info);
On 2011/07/13 18:47:53, kmadhusu wrote:
> nit: Fix indentation.

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper.h:344: SkScalar margin_left_in_points,
Now private variables in PrintWebViewHelper class
On 2011/07/12 22:04:50, Lei Zhang wrote:
> Consider using some additional data structures. When your method has 12
methods,
> it's getting pretty ugly.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:338: // TODO(aayushkumar): Have a
seperate function for printing Headers and Footer
On 2011/07/13 18:47:53, kmadhusu wrote:
> Implement this TODO.

Done.

http://codereview.chromium.org/7348010/diff/1020/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:388: UTF8ToUTF16("/") +
On 2011/07/13 18:47:53, kmadhusu wrote:
> nit: Fix indentation.

Done.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:101: void* font_or_paint,//const gfx::Font& font,
No, it wasn't, sorry!
On 2011/07/13 18:47:53, kmadhusu wrote:
> Is this comment intentional?

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:101: void* font_or_paint,//const gfx::Font& font,
On 2011/07/13 18:47:53, kmadhusu wrote:
> Is this comment intentional?

Done.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:112: if (available_width >=  
measureStringWidth(elided_path,
On 2011/07/13 18:47:53, kmadhusu wrote:
> nit: remove extra spaces.

Done.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:123: 
On 2011/07/12 22:04:50, Lei Zhang wrote:
> nit: Why the blank spaces? Try reviewing your own code first and weed out
these
> silly things so others don't waste time on them.

Done.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:141: GFX_Font);
On 2011/07/13 18:47:53, kmadhusu wrote:
> Fix indentation

Done.

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/7348010/diff/1020/ui/base/text/text_elider.h#n...
ui/base/text/text_elider.h:62: string16 ElideUrl(const GURL& url,
On 2011/07/12 22:04:50, Lei Zhang wrote:
> You shouldn't expose the internal implementation. Same for ElideText.

Done.

[kmadhusu, 2011-07-14 01:50:15]

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:70: <div
class="two-column option visible">
dpapad is refactoring the UI code (print_preview.html & .js). Its better to ask
him if it is ok to add new options here.

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

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:986: * When the user
selects or de-selects the Headers and footers option then we
Headers -> headers

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

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:488: if
(!settings->GetBoolean(printing::kSettingHeaderFooter, &isHeaderFooter)) {
nit: Remove {}
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:495: // No point going back to
the renderer and then making an explicit call to
Provide a simple comment. No need to specify any details on why you are creating
a dictionary here.

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:507:
header_footer_info.SetString(printing::kSettingHeaderFooterURL, "");
How about: 

std::string footer_url;
NavigationEntry* entry = initiator_tab->controller().GetActiveEntry();
if (entry)
  footer_url = entry->virtual_url().spec();

header_footer_info.SetString(printing::kSettingHeaderFooterURL, footer_url);

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:514: header_footer_info));
As thestig suggested, header_footer_info can be a part of |settings|. You can
remove it from the dictionary in print_web_view_helper.

http://codereview.chromium.org/7348010/diff/14002/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/common/print_message...
chrome/common/print_messages.h:53: IPC_STRUCT_MEMBER(bool, header_footer)
display_header_footer?

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

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:15: #include
"base/string_number_conversions.h"
nit: ordering

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:658:
settings.params.supports_alpha_blend = false;
You need to set "settings.header_footer = false;" here

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:678:
print_pages_params_.get()->params.header_footer = header_footer;
"print_pages_params_.get()" -> "print_pages_params_->"

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:868: switch (pos) {
Why did you use switch() here and if() in GetVerticalCoordinate() function? 

IMO, GetHorizontalCoordinate() and GetVerticalCoordinate() are similar
functions. Use the same set of conditional statements on these functions.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:869: case LEFT:
nit: curly braces.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:872: return
((content_width_in_points+margin_right_in_points) -
nit: add space before and after '+'

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:886: SkScalar
PrintWebViewHelper::GetVerticalCoordinate(
Are you planning to call GetHorizontalCoordinate() and GetVerticalCoordinate()
seperately from other places?  

I think you can merge them to a single function and make the resulting function
return SkPoint.

Sample:
SkPoint GetHeaderFooterLocation(...);

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:893: return
margin_bottom_in_points+content_height_in_points -
nit: add space before and after '+'

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:922: ((skia::VectorPlatformDeviceSkia*
)device)->
Remove outer paranthesis "(skia::VectorPlatformDeviceSkia* )device->"

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:928:
paint.setTextSize(SkIntToScalar(8)/webkit_scale_factor);
Is there any specific reason to use "8"?

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:934: string16 date =
base::TimeFormatShortDateNumeric(now);
|now| is used only once. 

Change line 934 to base::TimeFormatShortDateNumeric(Time::Now());

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:939: if
(!header_footer_info_->GetString("title", &title))
"title" -> printing::kSettingHeaderFooterTitle

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:948: webkit_scale_factor);
Fix indentation.

PrintHeaderFooterText(title, paint, canvas, CENTER, TOP,
                      webkit_scale_factor);

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:952: if
(!header_footer_info_->GetString("url", &url))
"url" -> printing::kSettingHeaderFooterURL

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:958: webkit_scale_factor);
Fix indentation

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:172: int total_pages =
*page_count;
|total_pages| is not required. At this point, you have already stored the page
count in |preview_page_count_|.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:223: int total_pages) {
You don't need |total_pages|. Just use |preview_page_count_|.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:250: PrintHeaderAndFooter(device,
canvas, params.page_number+1, total_pages, 1);
Why the scale factor on linux is '1'?

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:255: PrintHeaderAndFooter(device,
canvas, page_number+1, preview_page_count_,
nit: add space before and after '+'.

[dpapad, 2011-07-14 15:59:35]

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:70: <div
class="two-column option visible">
Are we planning to add more options in this category? If so, all these options
should belong to a new class like PageSettings, CopiesSettings and
LayoutSettings. If not, we can leave it as is for now.

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

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:989: function
onHeaderFooterChanged() {
Are headers and footers always available? For example when a user is previewing
an existing PDF doc can we still add them? If not, the option should be hidden
if the doc is not modifiable (as we do with layout options).

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

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:928:
paint.setTextSize(SkIntToScalar(8)/webkit_scale_factor);
Spaces around /.

[dpapad, 2011-07-14 16:27:04]

Also I noticed that some of the js/html files are outdated. Could you rebase
please, so that I can test your patch?

[Aayush Kumar, 2011-07-19 01:20:30]

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:70: <div
class="two-column option visible">
Based on our conversation, we'll let this be as it is until we know for sure
where these will be placed based on our discussion with rest of print preview
folks.
On 2011/07/14 15:59:35, dpapad wrote:
> Are we planning to add more options in this category? If so, all these options
> should belong to a new class like PageSettings, CopiesSettings and
> LayoutSettings. If not, we can leave it as is for now.

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

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:986: * When the user
selects or de-selects the Headers and footers option then we
On 2011/07/14 01:50:15, kmadhusu wrote:
> Headers -> headers

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:989: function
onHeaderFooterChanged() {
On 2011/07/14 15:59:35, dpapad wrote:
> Are headers and footers always available? For example when a user is
previewing
> an existing PDF doc can we still add them? If not, the option should be hidden
> if the doc is not modifiable (as we do with layout options).

Done.

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

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:488: if
(!settings->GetBoolean(printing::kSettingHeaderFooter, &isHeaderFooter)) {
It's now a multi-line if condition so braces have been retained
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: Remove {}
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:495: // No point going back to
the renderer and then making an explicit call to
On 2011/07/14 01:50:15, kmadhusu wrote:
> Provide a simple comment. No need to specify any details on why you are
creating
> a dictionary here.

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:507:
header_footer_info.SetString(printing::kSettingHeaderFooterURL, "");
On 2011/07/14 01:50:15, kmadhusu wrote:
> How about: 
> 
> std::string footer_url;
> NavigationEntry* entry = initiator_tab->controller().GetActiveEntry();
> if (entry)
>   footer_url = entry->virtual_url().spec();
> 
> header_footer_info.SetString(printing::kSettingHeaderFooterURL, footer_url);

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:514: header_footer_info));
I discussed it with him and in person and put a small comment in the small
review - I think he is now alright regarding the additional parameter since the
header footer strings aren't really a part of print settings and don't need to
be passed around with them.
On 2011/07/14 01:50:15, kmadhusu wrote:
> As thestig suggested, header_footer_info can be a part of |settings|. You can
> remove it from the dictionary in print_web_view_helper.

http://codereview.chromium.org/7348010/diff/14002/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/common/print_message...
chrome/common/print_messages.h:53: IPC_STRUCT_MEMBER(bool, header_footer)
On 2011/07/14 01:50:15, kmadhusu wrote:
> display_header_footer?

Done.

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

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:15: #include
"base/string_number_conversions.h"
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: ordering

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:658:
settings.params.supports_alpha_blend = false;
On 2011/07/14 01:50:15, kmadhusu wrote:
> You need to set "settings.header_footer = false;" here

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:678:
print_pages_params_.get()->params.header_footer = header_footer;
On 2011/07/14 01:50:15, kmadhusu wrote:
> "print_pages_params_.get()" -> "print_pages_params_->"

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:868: switch (pos) {
On 2011/07/14 01:50:15, kmadhusu wrote:
> Why did you use switch() here and if() in GetVerticalCoordinate() function? 
> 
> IMO, GetHorizontalCoordinate() and GetVerticalCoordinate() are similar
> functions. Use the same set of conditional statements on these functions.

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:869: case LEFT:
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: curly braces.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:872: return
((content_width_in_points+margin_right_in_points) -
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: add space before and after '+'

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:886: SkScalar
PrintWebViewHelper::GetVerticalCoordinate(
On 2011/07/14 01:50:15, kmadhusu wrote:
> Are you planning to call GetHorizontalCoordinate() and GetVerticalCoordinate()
> seperately from other places?  
> 
> I think you can merge them to a single function and make the resulting
function
> return SkPoint.
> 
> Sample:
> SkPoint GetHeaderFooterLocation(...);

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:893: return
margin_bottom_in_points+content_height_in_points -
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: add space before and after '+'

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:922: ((skia::VectorPlatformDeviceSkia*
)device)->
That doesn't work.  It tries to call setDrawingArea on SkDevice object rather
than treating it as a VectorPlatformDeviceSkia object.
On 2011/07/14 01:50:15, kmadhusu wrote:
> Remove outer paranthesis "(skia::VectorPlatformDeviceSkia* )device->"

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:928:
paint.setTextSize(SkIntToScalar(8)/webkit_scale_factor);
It's what looked good on the page according to me :)
On 2011/07/14 01:50:15, kmadhusu wrote:
> Is there any specific reason to use "8"?

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:928:
paint.setTextSize(SkIntToScalar(8)/webkit_scale_factor);
On 2011/07/14 15:59:35, dpapad wrote:
> Spaces around /.

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:934: string16 date =
base::TimeFormatShortDateNumeric(now);
On 2011/07/14 01:50:15, kmadhusu wrote:
> |now| is used only once. 
> 
> Change line 934 to base::TimeFormatShortDateNumeric(Time::Now());

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:939: if
(!header_footer_info_->GetString("title", &title))
Thanks! :)
On 2011/07/14 01:50:15, kmadhusu wrote:
> "title" -> printing::kSettingHeaderFooterTitle

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:948: webkit_scale_factor);
On 2011/07/14 01:50:15, kmadhusu wrote:
> Fix indentation.
> 
> PrintHeaderFooterText(title, paint, canvas, CENTER, TOP,
>                       webkit_scale_factor);

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:952: if
(!header_footer_info_->GetString("url", &url))
On 2011/07/14 01:50:15, kmadhusu wrote:
> "url" -> printing::kSettingHeaderFooterURL

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:958: webkit_scale_factor);
On 2011/07/14 01:50:15, kmadhusu wrote:
> Fix indentation

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:172: int total_pages =
*page_count;
On 2011/07/14 01:50:15, kmadhusu wrote:
> |total_pages| is not required. At this point, you have already stored the page
> count in |preview_page_count_|.

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:223: int total_pages) {
On 2011/07/14 01:50:15, kmadhusu wrote:
> You don't need |total_pages|. Just use |preview_page_count_|.

Done.

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:250: PrintHeaderAndFooter(device,
canvas, params.page_number+1, total_pages, 1);
The scale factor on linux is not used and is not relevant.  That's why it's
treated as '1'.
On 2011/07/14 01:50:15, kmadhusu wrote:
> Why the scale factor on linux is '1'?

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/14002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:255: PrintHeaderAndFooter(device,
canvas, page_number+1, preview_page_count_,
On 2011/07/14 01:50:15, kmadhusu wrote:
> nit: add space before and after '+'.

Done.

[dpapad, 2011-07-19 15:38:04]

2 questions:

1) How do we determine the max displayed length of text in the header? When I
tested I got trimmed text (like "August 2010 in sports - Wikipedia, the free
en…") even though there is plenty of space to display the entire title.

2) Do we want headers and footers being enabled by default? Does that match the
behavior of any native printing dialog (win,mac,linux).

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:59: <div
id="options-option" class="two-column option visible">
I feel that the "options" label is really generic (and only contains one
option). How about naming this whole section "Headers and footers" instead of
"Options" and the label of the checkbox to "Enable" instead of "Headers and
footers"?

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

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:122: SkScalar x;
SkScalar x(0); Otherwise I am getting compilation error on Linux,
chrome/renderer/print_web_view_helper.cc:122: error: ‘x’ may be used
uninitialized in this function

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkScalar y;
Same here.

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:223: // Printing the Date.
nit: Comments should end with a period, here and elsewhere.

[Aayush Kumar, 2011-07-19 18:37:19]

(i) I was working on customizing the lengths of the strings.  This has been
uploaded in this change.  Previously, it was constrained to 1/3 the size of the
page.
(ii) I checked on the following and Headers and Footers are enabled by default
on these platform/browser.
Windows: IE, Firefox
Max: Firefox, Safari
Linux: Firefox
I didn't check any other browser after these.

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:59: <div
id="options-option" class="two-column option visible">
Let me hold these changes off until we decide for sure what the UI will look
like.  In one of the e-mails, there was a thought that margins and headers and
footers be clubbed into one zippy.  In that case, 'options' may be an
appropriate name.  :)
On 2011/07/19 15:38:04, dpapad wrote:
> I feel that the "options" label is really generic (and only contains one
> option). How about naming this whole section "Headers and footers" instead of
> "Options" and the label of the checkbox to "Enable" instead of "Headers and
> footers"?

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

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:122: SkScalar x;
On 2011/07/19 15:38:04, dpapad wrote:
> SkScalar x(0); Otherwise I am getting compilation error on Linux,
> chrome/renderer/print_web_view_helper.cc:122: error: ‘x’ may be used
> uninitialized in this function

Done.

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:122: SkScalar x;
Thanks for pointing it out - it's weird that I didn't get them on my side.
On 2011/07/19 15:38:04, dpapad wrote:
> SkScalar x(0); Otherwise I am getting compilation error on Linux,
> chrome/renderer/print_web_view_helper.cc:122: error: ‘x’ may be used
> uninitialized in this function

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkScalar y;
On 2011/07/19 15:38:04, dpapad wrote:
> Same here.

Done.

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:223: // Printing the Date.
On 2011/07/19 15:38:04, dpapad wrote:
> nit: Comments should end with a period, here and elsewhere.

Done.

[dpapad, 2011-07-19 20:34:01]

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

http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:487: bool displayHeaderFooter;
Rename to display_header_footer, see styleguide for naming rules
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names.

http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:494: // and url and send it to
the renderer process.
Nit: Change to "If headers and footers option is checked retrieve the page title
and url and send it to the renderer process."

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

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:7: #include<algorithm>
Space between words.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:84: void PrintHeaderAndFooter(SkDevice
*device,
Document function and parameters.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:207: // The page_number count
starts from '0', so, we add by '1'.
Nit: Replace this line with 
"The page_number count starts from 0, so we add 1."
Same in print_web_view_helper_win.cc.

http://codereview.chromium.org/7348010/diff/27001/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.h (right):

http://codereview.chromium.org/7348010/diff/27001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.h:82: // Sets the drawing area for the
device.  Subsequent draw calls are
Nit: Remove double space.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:82: SkScalar measureStringWidth(const string16&
text,
Nit: Function names should start with capital letter.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:290: pixel_width_url_host + pixel_width_url_path)
Use braces when an if statement spans multiple lines.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:475: return ElideText(text, (void* )&font,
(SkScalar)available_pixel_width,
Nit: No need for space in "(void* )", here and elsewhere.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:51: // Overloaded function.
I do not think that this comment is needed. Also consider what is stated
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl....

[vandebo (ex-Chrome), 2011-07-19 21:31:20]

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

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:497:
header_footer_info.SetString(printing::kSettingHeaderFooterTitle,
Why not just put this in settings?

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:500: std::string footer_url;
nit: url

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:14: #include
"base/utf_string_conversions.h"
In the header, we only include things that are used in the header.  Then, in the
implementation file, we include the rest.  Looks like you've added a bunch of
stuff that's only needed in the implementation file.

http://codereview.chromium.org/7348010/diff/21001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:85: const SkRefPtr<skia::VectorCanvas>&
canvas,
skia::VectorCanvas* canvas,

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

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:121: // Gets the (x,y) coordinate from
where we want to start printing the current
nit: (x, y)

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:134: SkScalar x(0);
nit: = 0; is easier to understand.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: paint.getTextSize();
As we talked about, getTextSize() doesn't tell you the height of the maximum
descender.  The more complicated version of SkPaint::measureText will give you
the bounding box of the text, from which you can calculate the total height,
etc.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: void PrintHeaderAndFooter(SkDevice
*device,
Arguments should line up.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: void PrintHeaderAndFooter(SkDevice
*device,
Maybe this function should take an SkPDFDevice*, doing the casting at the
calling site.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:208: ((skia::VectorPlatformDeviceSkia*
)device) ->
Use C++ style casts.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Ensuring we have enough space
to print above and below the page to print
Is it necessary to do this?  If margins are 0, the headers and footers will
simply be overwritten.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:250: PageSizeMargins
GetPageSizeAndMarginsInPoints(
It looks like this change is orthogonal to your header and footer change.  Maybe
it would be better to pull it into a different CL.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:11: #include
"base/utf_string_conversions.h"
Looks like you've added a bunch of extra includes that aren't needed any more. 
Please only include things that are needed.

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

http://codereview.chromium.org/7348010/diff/27001/printing/print_job_constant...
printing/print_job_constants.cc:37: extern const float
kSettingHeaderFooterHorizontalRegions = 3;
Do we put anything in the center?  Maybe set this to 2 until we do (and note
that in the comment).

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:75: 
I think this is the wrong way to go about things.  You should probably look in
ui/gfx for the platform agnostic way to draw text and use that.  You'll probably
find stuff in there to let you do what you want without having to make major
changes.  estade probably know what you want, though I don't.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:51: // Overloaded function.
Yea, this doesn't look like a valid overload.

[Aayush Kumar, 2011-07-21 21:58:53]

There are still a few UI kinks that are appearing in Windows and Mac.  One of
the reasons for this is that SkPaint::measureText is returning incorrect values
(about two times as long!)
I am working on the core-graphics changes.

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

http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:487: bool displayHeaderFooter;
On 2011/07/19 20:34:01, dpapad wrote:
> Rename to display_header_footer, see styleguide for naming rules
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:494: // and url and send it to
the renderer process.
On 2011/07/19 20:34:01, dpapad wrote:
> Nit: Change to "If headers and footers option is checked retrieve the page
title
> and url and send it to the renderer process."

Done.

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

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:7: #include<algorithm>
On 2011/07/19 20:34:01, dpapad wrote:
> Space between words.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:121: // Gets the (x,y) coordinate from
where we want to start printing the current
On 2011/07/19 21:31:20, vandebo wrote:
> nit: (x, y)

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:134: SkScalar x(0);
On 2011/07/19 21:31:20, vandebo wrote:
> nit: = 0; is easier to understand.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: paint.getTextSize();
On 2011/07/19 21:31:20, vandebo wrote:
> As we talked about, getTextSize() doesn't tell you the height of the maximum
> descender.  The more complicated version of SkPaint::measureText will give you
> the bounding box of the text, from which you can calculate the total height,
> etc.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: void PrintHeaderAndFooter(SkDevice
*device,
On 2011/07/19 21:31:20, vandebo wrote:
> Arguments should line up.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: void PrintHeaderAndFooter(SkDevice
*device,
On 2011/07/19 21:31:20, vandebo wrote:
> Maybe this function should take an SkPDFDevice*, doing the casting at the
> calling site.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:208: ((skia::VectorPlatformDeviceSkia*
)device) ->
On 2011/07/19 21:31:20, vandebo wrote:
> Use C++ style casts.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Ensuring we have enough space
to print above and below the page to print
I am afraid if the margins are more than '0', but, not large enough to be able
to print the text in completely.  In that scenario, the text will appear to be
partially overwritten - I wanted to avoid that.  This may happen either when we
give the user the choice to choose custom margins, or, on funky paper sizes.

On 2011/07/19 21:31:20, vandebo wrote:
> Is it necessary to do this?  If margins are 0, the headers and footers will
> simply be overwritten.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:84: void PrintHeaderAndFooter(SkDevice
*device,
On 2011/07/19 20:34:01, dpapad wrote:
> Document function and parameters.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:250: PageSizeMargins
GetPageSizeAndMarginsInPoints(
I am creating another CL for this, and will remove these changes after the code
rebase :)
On 2011/07/19 21:31:20, vandebo wrote:
> It looks like this change is orthogonal to your header and footer change. 
Maybe
> it would be better to pull it into a different CL.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:11: #include
"base/utf_string_conversions.h"
On 2011/07/19 21:31:20, vandebo wrote:
> Looks like you've added a bunch of extra includes that aren't needed any more.

> Please only include things that are needed.

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:207: // The page_number count
starts from '0', so, we add by '1'.
On 2011/07/19 20:34:01, dpapad wrote:
> Nit: Replace this line with 
> "The page_number count starts from 0, so we add 1."
> Same in print_web_view_helper_win.cc.

Done.

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

http://codereview.chromium.org/7348010/diff/27001/printing/print_job_constant...
printing/print_job_constants.cc:37: extern const float
kSettingHeaderFooterHorizontalRegions = 3;
The title in the header is printed in the center.
On 2011/07/19 21:31:20, vandebo wrote:
> Do we put anything in the center?  Maybe set this to 2 until we do (and note
> that in the comment).

http://codereview.chromium.org/7348010/diff/27001/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.h (right):

http://codereview.chromium.org/7348010/diff/27001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.h:82: // Sets the drawing area for the
device.  Subsequent draw calls are
On 2011/07/19 20:34:01, dpapad wrote:
> Nit: Remove double space.

Done.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:75: 
Restored changes :)  I now have mapping between the gfx::Font and Skia:Typeface
font.
On 2011/07/19 21:31:20, vandebo wrote:
> I think this is the wrong way to go about things.  You should probably look in
> ui/gfx for the platform agnostic way to draw text and use that.  You'll
probably
> find stuff in there to let you do what you want without having to make major
> changes.  estade probably know what you want, though I don't.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:82: SkScalar measureStringWidth(const string16&
text,
On 2011/07/19 20:34:01, dpapad wrote:
> Nit: Function names should start with capital letter.

Done.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:290: pixel_width_url_host + pixel_width_url_path)
On 2011/07/19 20:34:01, dpapad wrote:
> Use braces when an if statement spans multiple lines.

Done.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:475: return ElideText(text, (void* )&font,
(SkScalar)available_pixel_width,
On 2011/07/19 20:34:01, dpapad wrote:
> Nit: No need for space in "(void* )", here and elsewhere.

Done.

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/7348010/diff/27001/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:51: // Overloaded function.
Restored changes.
On 2011/07/19 21:31:20, vandebo wrote:
> Yea, this doesn't look like a valid overload.

[dpapad, 2011-07-21 23:18:51]

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

http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:51: using
printing::ConvertPointsToPixelDouble;
Nit: alphabetical order.

http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:123: // Given a paint object, gets the
maximum possible height of any glyph.
Nit: There is no "paint" class, you mean "Given a SkPaint object".

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

http://codereview.chromium.org/7348010/diff/40001/printing/print_job_constant...
printing/print_job_constants.cc:33: // Default Font size for printing the
headers and footers.
Default font name.

[Aayush Kumar, 2011-07-21 23:30:47]

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

http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:51: using
printing::ConvertPointsToPixelDouble;
Am I missing something?  Isn't it already?
On 2011/07/21 23:18:51, dpapad wrote:
> Nit: alphabetical order.

http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:123: // Given a paint object, gets the
maximum possible height of any glyph.
On 2011/07/21 23:18:51, dpapad wrote:
> Nit: There is no "paint" class, you mean "Given a SkPaint object".

Done.

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

http://codereview.chromium.org/7348010/diff/40001/printing/print_job_constant...
printing/print_job_constants.cc:33: // Default Font size for printing the
headers and footers.
On 2011/07/21 23:18:51, dpapad wrote:
> Default font name.

Done.

http://codereview.chromium.org/7348010/diff/40001/printing/print_job_constant...
printing/print_job_constants.cc:33: // Default Font size for printing the
headers and footers.
thanks! :)
On 2011/07/21 23:18:51, dpapad wrote:
> Default font name.

[dpapad, 2011-07-21 23:47:49]

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

http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:51: using
printing::ConvertPointsToPixelDouble;
On 2011/07/21 23:30:47, Aayush Kumar wrote:
> Am I missing something?  Isn't it already?
> On 2011/07/21 23:18:51, dpapad wrote:
> > Nit: alphabetical order.
> 
You are right, never mind.

[vandebo (ex-Chrome), 2011-07-22 22:58:33]

Have you rebased recently? There's been a lot of changes in this code recently.

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

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:497:
header_footer_info.SetString(printing::kSettingHeaderFooterTitle,
On 2011/07/19 21:31:20, vandebo wrote:
> Why not just put this in settings?

You didn't address this comment.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:250: PageSizeMargins
GetPageSizeAndMarginsInPoints(
Can you do that rebase?  It'll be easier to review the code without this stuff
in the diff.  Ask if you want help managing multiple CLs

On 2011/07/21 21:58:53, Aayush Kumar wrote:
> I am creating another CL for this, and will remove these changes after the
code
> rebase :)
> On 2011/07/19 21:31:20, vandebo wrote:
> > It looks like this change is orthogonal to your header and footer change. 
> Maybe
> > it would be better to pull it into a different CL.
>

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

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:108: size_t
GetString16ByteLength(string16 text) {
I don't think this needs to be a function, go ahead and inline it where it's
called.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:200: SkPoint point =
GetHeaderFooterPosition(text, paint, hor_pos, ver_pos,
Maybe just inline getheaderfooterposition here, since this is the only call site
and this method is short otherwise.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:236: GetMaxTextHeight(paint);
Why not measure the actual text height here, instead of the max text height?

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: date = ui::ElideText(date, font,
ConvertPointsToPixelDouble(segment_width),
Can you pull the ElideText into the PrintHeaderFooterText method?

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:18: #include "skia/ext/vector_canvas.h"
Don't include heads for things you only use as pointers, insted just declare the
type, i.e. namespace skia { class VectorCanvas; }

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:83: bool display_header_footer) {
Why two bools?

[Aayush Kumar, 2011-07-24 02:09:00]

Rebased code as of Friday (july 22) night.

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

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:497:
header_footer_info.SetString(printing::kSettingHeaderFooterTitle,
Sorry about that - I somehow seemed to have missed this file.

The reason I didn't put this in settings is because the settings param contains
a lot of printer related settings that is then sent all over, and used to get
the appropriate printer settings.  The header footer strings are not printer
related and will not be used by it - thus - I created a separate parameter.
On 2011/07/22 22:58:33, vandebo wrote:
> On 2011/07/19 21:31:20, vandebo wrote:
> > Why not just put this in settings?
> 
> You didn't address this comment.

http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:500: std::string footer_url;
On 2011/07/19 21:31:20, vandebo wrote:
> nit: url

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:250: PageSizeMargins
GetPageSizeAndMarginsInPoints(
On 2011/07/22 22:58:33, vandebo wrote:
> Can you do that rebase?  It'll be easier to review the code without this stuff
> in the diff.  Ask if you want help managing multiple CLs
> 
> On 2011/07/21 21:58:53, Aayush Kumar wrote:
> > I am creating another CL for this, and will remove these changes after the
> code
> > rebase :)
> > On 2011/07/19 21:31:20, vandebo wrote:
> > > It looks like this change is orthogonal to your header and footer change. 
> > Maybe
> > > it would be better to pull it into a different CL.
> > 
> 

Done.

http://codereview.chromium.org/7348010/diff/27001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:250: PageSizeMargins
GetPageSizeAndMarginsInPoints(
CL number: 7492031
On 2011/07/22 22:58:33, vandebo wrote:
> Can you do that rebase?  It'll be easier to review the code without this stuff
> in the diff.  Ask if you want help managing multiple CLs
> 
> On 2011/07/21 21:58:53, Aayush Kumar wrote:
> > I am creating another CL for this, and will remove these changes after the
> code
> > rebase :)
> > On 2011/07/19 21:31:20, vandebo wrote:
> > > It looks like this change is orthogonal to your header and footer change. 
> > Maybe
> > > it would be better to pull it into a different CL.
> > 
>

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

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:108: size_t
GetString16ByteLength(string16 text) {
On 2011/07/22 22:58:33, vandebo wrote:
> I don't think this needs to be a function, go ahead and inline it where it's
> called.

Done.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:200: SkPoint point =
GetHeaderFooterPosition(text, paint, hor_pos, ver_pos,
On 2011/07/22 22:58:33, vandebo wrote:
> Maybe just inline getheaderfooterposition here, since this is the only call
site
> and this method is short otherwise.

Done.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:236: GetMaxTextHeight(paint);
On 2011/07/22 22:58:33, vandebo wrote:
> Why not measure the actual text height here, instead of the max text height?

Done.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:246: date = ui::ElideText(date, font,
ConvertPointsToPixelDouble(segment_width),
On 2011/07/22 22:58:33, vandebo wrote:
> Can you pull the ElideText into the PrintHeaderFooterText method?

Done.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:18: #include "skia/ext/vector_canvas.h"
On 2011/07/22 22:58:33, vandebo wrote:
> Don't include heads for things you only use as pointers, insted just declare
the
> type, i.e. namespace skia { class VectorCanvas; }

Done.

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/40004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:83: bool display_header_footer) {
Removed the display_header_footer bool
On 2011/07/22 22:58:33, vandebo wrote:
> Why two bools?

[vandebo (ex-Chrome), 2011-07-25 01:06:59]

>
http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
> File chrome/browser/ui/webui/print_preview_handler.cc (right):
>
>
http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/pri...
> chrome/browser/ui/webui/print_preview_handler.cc:497:
> header_footer_info.SetString(printing::kSettingHeaderFooterTitle,
> Sorry about that - I somehow seemed to have missed this file.
>
> The reason I didn't put this in settings is because the settings param
> contains a lot of printer related settings that is then sent all over,
> and used to get the appropriate printer settings.  The header footer
> strings are not printer related and will not be used by it - thus - I
> created a separate parameter.

It seems to me that including it in the print parameters would be cleaner,
and headers and footers are parameters of the particular print job. Plus the
existing settings param is very generic.

[dpapad, 2011-07-25 15:40:01]

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:809: if
(!previewModifiable) {
Nit: No need for {}.

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:937: * When the user
selects or de-selects the headers and footers option then we
Nit: Don't use "we"n in comments.

When the user selects/deselects headers and footers a new preview is requested.

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:505:
header_footer_info.SetString(printing::kSettingHeaderFooterURL,
Should this statement be within "if (entry)" block?

http://codereview.chromium.org/7348010/diff/50002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/50002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:191: // The page_number count
starts from 0, so we add 1.
Nit: |page_number| is 0-based, so 1 is added.

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

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:33: // Default Font name for printing the
headers and footers.
nit: s/Font/font

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:36: // Default Font size for printing the
headers and footers.
nit: s/Font/font

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:42: // Interstice or gap between different
header footer components.
Nit: More can fit on this line.

[Aayush Kumar, 2011-07-25 18:37:22]

Removed the second parameter from the IPC call as suggested by Steve :)

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:809: if
(!previewModifiable) {
On 2011/07/25 15:40:01, dpapad wrote:
> Nit: No need for {}.

Done.

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:937: * When the user
selects or de-selects the headers and footers option then we
On 2011/07/25 15:40:01, dpapad wrote:
> Nit: Don't use "we"n in comments.
> 
> When the user selects/deselects headers and footers a new preview is
requested.

Done.

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:505:
header_footer_info.SetString(printing::kSettingHeaderFooterURL,
No :)
On 2011/07/25 15:40:01, dpapad wrote:
> Should this statement be within "if (entry)" block?

http://codereview.chromium.org/7348010/diff/50002/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/50002/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:191: // The page_number count
starts from 0, so we add 1.
On 2011/07/25 15:40:01, dpapad wrote:
> Nit: |page_number| is 0-based, so 1 is added.

Done.

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

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:33: // Default Font name for printing the
headers and footers.
On 2011/07/25 15:40:01, dpapad wrote:
> nit: s/Font/font

Done.

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:36: // Default Font size for printing the
headers and footers.
On 2011/07/25 15:40:01, dpapad wrote:
> nit: s/Font/font

Done.

http://codereview.chromium.org/7348010/diff/50002/printing/print_job_constant...
printing/print_job_constants.cc:42: // Interstice or gap between different
header footer components.
I purposely gave it a line break to make it easier to read.  If you're fine,
I'll leave it this way :)
On 2011/07/25 15:40:01, dpapad wrote:
> Nit: More can fit on this line.

[dpapad, 2011-07-25 20:14:20]

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:505:
header_footer_info.SetString(printing::kSettingHeaderFooterURL,
On 2011/07/25 18:37:23, Aayush Kumar wrote:
> No :)
> On 2011/07/25 15:40:01, dpapad wrote:
> > Should this statement be within "if (entry)" block?
> 
Please provide more info. Specifically, what happens if(!entry), do we just pass
an empty string?

[Aayush Kumar, 2011-07-25 22:31:15]

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

http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:505:
header_footer_info.SetString(printing::kSettingHeaderFooterURL,
It turns out that the url can be an empty string, if it isn't valid.  According
to the doc of the spec() function - "If the URL is not valid, this will assert
and return the empty string (for safety in release builds, to keep them from
being misused which might be a security problem)."

On 2011/07/25 20:14:20, dpapad wrote:
> On 2011/07/25 18:37:23, Aayush Kumar wrote:
> > No :)
> > On 2011/07/25 15:40:01, dpapad wrote:
> > > Should this statement be within "if (entry)" block?
> > 
> Please provide more info. Specifically, what happens if(!entry), do we just
pass
> an empty string?

[Chris Guillory, 2011-07-26 00:10:44]

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

http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:487: bool
display_header_footer;
This local isn't used. Do you just want to
DCHECK(settings->HasKey(printing::kSettingHeaderFooter))?

http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:493: // Retrieving the page
title and url and sending it to the renderer process.
Nit: Retrieving -> Retrieve
sending -> send

http://codereview.chromium.org/7348010/diff/58001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/58001/chrome/common/print_message...
chrome/common/print_messages.h:54: // Specifies if the header and footer should
be printed.
Optional: printed -> rendered

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

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:108: // Splits the horizontal width
equally into three segments with an interstice
Nit: three segments -> segments

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:110: SkScalar
GetSegmentWidth(PageSizeMargins& page_layout) {
Make argument const.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:114:
(printing::kSettingHeaderFooterHorizontalRegions + 1) *
Can you explain why you have + 1 here?

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:120: // coordinates and prints the text
at those co-ordinates on the canvas.
Nit: co-ordinates -> coordinates
the positions and -> the positions, and

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:124: const
SkRefPtr<skia::VectorCanvas>& canvas,
Remove SkRefPtr usage if possible.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:125:
printing::HorizontalHeaderFooterPosition hor_pos,
Optional: hor_pos -> horizontal_position

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:126:
printing::VerticalHeaderFooterPosition ver_pos,
Optional: ver_pos -> vertical_position

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:128: PageSizeMargins& page_layout,
const

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:137: // Gets the (x, y) coordinate from
where we want to start printing the current
Nit: Gets -> Get

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:140: {
Can you remove this nested block.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:147: x = page_layout.margin_left * (-1)
+
page_layout.margin_left * (-1) -> -page_layout.margin_left

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: case printing::CENTER: {
Looks like braces are only needed on this case statement. Can you remove them on
the other case statements.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:158: x = (available_width -
page_layout.margin_left +
Don't need the outer parentheses.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:170: y = page_layout.margin_top * (-1)
+
page_layout.margin_top * (-1) -> -page_layout.margin_top

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:187:
paint.setTextSize(paint.getTextSize()/webkit_scale_factor);
To be consistent, add space before and the /.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:198: PageSizeMargins& page_layout,
const

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:213: SkTypeface::kNormal));
This can fit on the previous line.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Ensuring we have enough space
to print above and below the page to print
Nit: Ensuring -> Ensure

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:234: // Ensuring we have enough space
to print above and below the page to print
Nit: Ensuring -> Ensure

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:256: // Printing the Date.
Nit: Printing -> Print
Here and below.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:264: // Calculating the available title
width. If the date string is not long
Nit: Calculating -> Calculate
Here and below.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:950:
print_pages_params_->params.display_header_footer = false;
Can you set display_header_footer on the settings local above instead of
directly setting it on print_pages_params_ here?

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:86: void
PrintHeaderAndFooter(skia::VectorPlatformDeviceSkia* device,
Optional: PrintHeaderAndFooterForPage?

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:87: const SkRefPtr<skia::VectorCanvas>&
canvas,
Can we remove the SkRefPtr usage here.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:88: int page_number, int total_pages,
Can you place these arguments on separate lines.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:90: PageSizeMargins&
page_layout_in_points,
Can this be const.

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

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constant...
printing/print_job_constants.cc:30: // Option to print Headers and Footers: true
if selected, false if not.
Nit: Headers and Footers -> headers and footers

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constant...
printing/print_job_constants.h:38: // Specifies the horizontal alignment of the
Headers and Footers.
Headers and Footers -> headers and footers.
Here and below.

http://codereview.chromium.org/7348010/diff/58001/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.h (right):

http://codereview.chromium.org/7348010/diff/58001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.h:84: virtual void
setDrawingArea(SkPDFDevice::DrawingArea area);
Doesn't need to be virtual.

[Aayush Kumar, 2011-07-26 01:55:41]

A TODO for me - I'll be adding #if define(USE_SKIA) conditionals in
print_web_view_helper.cc once I have the CG changes in here.

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

http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:487: bool
display_header_footer;
Sorry - this was deprecated code.  Removed.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> This local isn't used. Do you just want to
> DCHECK(settings->HasKey(printing::kSettingHeaderFooter))?

http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:493: // Retrieving the page
title and url and sending it to the renderer process.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Retrieving -> Retrieve
> sending -> send

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/common/print_messages.h
File chrome/common/print_messages.h (right):

http://codereview.chromium.org/7348010/diff/58001/chrome/common/print_message...
chrome/common/print_messages.h:54: // Specifies if the header and footer should
be printed.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Optional: printed -> rendered

Done.

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

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:108: // Splits the horizontal width
equally into three segments with an interstice
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: three segments -> segments

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:110: SkScalar
GetSegmentWidth(PageSizeMargins& page_layout) {
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Make argument const.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:114:
(printing::kSettingHeaderFooterHorizontalRegions + 1) *
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can you explain why you have + 1 here?

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:120: // coordinates and prints the text
at those co-ordinates on the canvas.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: co-ordinates -> coordinates
> the positions and -> the positions, and

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:124: const
SkRefPtr<skia::VectorCanvas>& canvas,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Remove SkRefPtr usage if possible.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:125:
printing::HorizontalHeaderFooterPosition hor_pos,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Optional: hor_pos -> horizontal_position

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:126:
printing::VerticalHeaderFooterPosition ver_pos,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Optional: ver_pos -> vertical_position

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:128: PageSizeMargins& page_layout,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> const

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:137: // Gets the (x, y) coordinate from
where we want to start printing the current
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Gets -> Get

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:140: {
It tells us that that is a separate control block which is solely responsible
for getting the coordinates.  The idea to inline this function was actually
suggested by Steve. 

On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can you remove this nested block.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:147: x = page_layout.margin_left * (-1)
+
On 2011/07/26 00:10:45, Chris Guillory wrote:
> page_layout.margin_left * (-1) -> -page_layout.margin_left

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: case printing::CENTER: {
According to the google C++ style guidelines, it seems that it's alright to
insert braces.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...


On 2011/07/26 00:10:45, Chris Guillory wrote:
> Looks like braces are only needed on this case statement. Can you remove them
on
> the other case statements.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:158: x = (available_width -
page_layout.margin_left +
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Don't need the outer parentheses.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:170: y = page_layout.margin_top * (-1)
+
On 2011/07/26 00:10:45, Chris Guillory wrote:
> page_layout.margin_top * (-1) -> -page_layout.margin_top

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:187:
paint.setTextSize(paint.getTextSize()/webkit_scale_factor);
On 2011/07/26 00:10:45, Chris Guillory wrote:
> To be consistent, add space before and the /.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:198: PageSizeMargins& page_layout,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> const

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:213: SkTypeface::kNormal));
On 2011/07/26 00:10:45, Chris Guillory wrote:
> This can fit on the previous line.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Ensuring we have enough space
to print above and below the page to print
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Ensuring -> Ensure

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:234: // Ensuring we have enough space
to print above and below the page to print
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Ensuring -> Ensure

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:256: // Printing the Date.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Printing -> Print
> Here and below.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:264: // Calculating the available title
width. If the date string is not long
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Calculating -> Calculate
> Here and below.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:950:
print_pages_params_->params.display_header_footer = false;
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can you set display_header_footer on the settings local above instead of
> directly setting it on print_pages_params_ here?

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:86: void
PrintHeaderAndFooter(skia::VectorPlatformDeviceSkia* device,
I feel that the function name becomes really long, and, since it's called from
PrintPageInternal, it's somewhat obvious it's printing the headers and footers
for that page.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Optional: PrintHeaderAndFooterForPage?

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:87: const SkRefPtr<skia::VectorCanvas>&
canvas,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can we remove the SkRefPtr usage here.

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:88: int page_number, int total_pages,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can you place these arguments on separate lines. 

Done.

http://codereview.chromium.org/7348010/diff/58001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:90: PageSizeMargins&
page_layout_in_points,
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Can this be const.

Done.

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

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constant...
printing/print_job_constants.cc:30: // Option to print Headers and Footers: true
if selected, false if not.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Nit: Headers and Footers -> headers and footers

Done.

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7348010/diff/58001/printing/print_job_constant...
printing/print_job_constants.h:38: // Specifies the horizontal alignment of the
Headers and Footers.
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Headers and Footers -> headers and footers.
> Here and below.

Done.

http://codereview.chromium.org/7348010/diff/58001/skia/ext/vector_platform_de...
File skia/ext/vector_platform_device_skia.h (right):

http://codereview.chromium.org/7348010/diff/58001/skia/ext/vector_platform_de...
skia/ext/vector_platform_device_skia.h:84: virtual void
setDrawingArea(SkPDFDevice::DrawingArea area);
On 2011/07/26 00:10:45, Chris Guillory wrote:
> Doesn't need to be virtual.

Done.

[vandebo (ex-Chrome), 2011-07-26 06:11:37]

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

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:112: page_layout.content_width +
nit: indent

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:142: {
Don't use an extra scope.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:229: SkScalar text_height =
printing::kSettingHeaderFooterInterstice +
nit: = <newline>

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:237: string16 page_on_page_total =
base::IntToString16(page_number) +
page_of_total_pages ?

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:250: text_height =
printing::kSettingHeaderFooterInterstice +
nit: = <newline>

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:290: SkScalar max_url_width = 3 *
(page_layout.content_width +
nit: newline
foo = 
    (foo + bar +
     baz) * 3 / 4;

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:368: header_footer_info_ = new
DictionaryValue();
put in init list

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:969: // Getting Header and Footer
settings.
Should this go into UpdatePrintSettings?

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:32: template <typename t> class
SkRefPtr;
I don't think this line is needed.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:225: bool is_preview);
metafile is an output param, so is_preview has to come before it.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:192:
PrintHeaderAndFooter(static_cast<skia::VectorPlatformDeviceSkia*>(device),
I didn't realize that doing the casting outside the function was going to lead
to including vector_platform_device_skia.h in all three platform files.  Go
ahead and do the cast in PrintHeaderAndFooter.

[Aayush Kumar, 2011-07-26 07:37:33]

Added #if defined(USE_SKIA) around printing of header and footers.
UI for header and footer checkbox is now hidden on mac.

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

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:112: page_layout.content_width +
On 2011/07/26 06:11:37, vandebo wrote:
> nit: indent

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:142: {
On 2011/07/26 06:11:37, vandebo wrote:
> Don't use an extra scope.

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:229: SkScalar text_height =
printing::kSettingHeaderFooterInterstice +
On 2011/07/26 06:11:37, vandebo wrote:
> nit: = <newline>

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:237: string16 page_on_page_total =
base::IntToString16(page_number) +
On 2011/07/26 06:11:37, vandebo wrote:
> page_of_total_pages ?

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:250: text_height =
printing::kSettingHeaderFooterInterstice +
On 2011/07/26 06:11:37, vandebo wrote:
> nit: = <newline>

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:290: SkScalar max_url_width = 3 *
(page_layout.content_width +
On 2011/07/26 06:11:37, vandebo wrote:
> nit: newline
> foo = 
>     (foo + bar +
>      baz) * 3 / 4;

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:368: header_footer_info_ = new
DictionaryValue();
On 2011/07/26 06:11:37, vandebo wrote:
> put in init list

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:969: // Getting Header and Footer
settings.
I just rebased and it seems that UpdatePrintSettingsLocal is now
UpdatePrintSettings and UpdatePrintSettingsCloud no longer exists.
On 2011/07/26 06:11:37, vandebo wrote:
> Should this go into UpdatePrintSettings?

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:32: template <typename t> class
SkRefPtr;
On 2011/07/26 06:11:37, vandebo wrote:
> I don't think this line is needed.

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:225: bool is_preview);
On 2011/07/26 06:11:37, vandebo wrote:
> metafile is an output param, so is_preview has to come before it.

Done.

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_win.cc:192:
PrintHeaderAndFooter(static_cast<skia::VectorPlatformDeviceSkia*>(device),
On 2011/07/26 06:11:37, vandebo wrote:
> I didn't realize that doing the casting outside the function was going to lead
> to including vector_platform_device_skia.h in all three platform files.  Go
> ahead and do the cast in PrintHeaderAndFooter.

Done.

[Aayush Kumar, 2011-07-26 07:59:21]

Just uploaded a patch that frees memory for header_footer_info_ var.

[vandebo (ex-Chrome), 2011-07-26 08:08:54]

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

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:118: // Interstice is left at both ends
of the page as well as between
Put the comment outside

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:120:
(printing::kSettingHeaderFooterHorizontalRegions + 1) *
Indent the () block one more.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:205:
setDrawingArea(SkPDFDevice::kMargin_DrawingArea);
wrap the argument  foo->bar(<newline>

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:214:
printing::kSettingHeaderFooterFontSize)));
one more space

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:308:
static_cast<skia::VectorPlatformDeviceSkia*>(device)->
Here too

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:380: delete header_footer_info_;
use a scoped_ptr instead

[Aayush Kumar, 2011-07-26 08:52:27]

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

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:118: // Interstice is left at both ends
of the page as well as between
On 2011/07/26 08:08:55, vandebo wrote:
> Put the comment outside

Done.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:120:
(printing::kSettingHeaderFooterHorizontalRegions + 1) *
On 2011/07/26 08:08:55, vandebo wrote:
> Indent the () block one more.

Done.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:205:
setDrawingArea(SkPDFDevice::kMargin_DrawingArea);
On 2011/07/26 08:08:55, vandebo wrote:
> wrap the argument  foo->bar(<newline>

Done.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:214:
printing::kSettingHeaderFooterFontSize)));
On 2011/07/26 08:08:55, vandebo wrote:
> one more space

Done.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:308:
static_cast<skia::VectorPlatformDeviceSkia*>(device)->
On 2011/07/26 08:08:55, vandebo wrote:
> Here too

Done.

http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:380: delete header_footer_info_;
On 2011/07/26 08:08:55, vandebo wrote:
> use a scoped_ptr instead

Done.

[kmadhusu, 2011-07-26 20:27:50]

Please rebase your CL.

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

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:40: #include
"ui/base/text/text_elider.h"
can this header file go inside the #if defined(USE_SKIA) block?

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:151: x = -page_layout.margin_left +
Can you rewrite this statement as follows:

x = printing::kSettingHeaderFooterInterstice - page_layout.margin_left;

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: x = (page_layout.content_width +
page_layout.margin_right -
nit: This statement does not require paranthesis since it involves arithmetic
addition and subtraction.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:175:
printing::kSettingHeaderFooterInterstice;
How about:

y = printing::kSettingHeaderFooterInterstice - page_layout.margin_top -
height_to_baseline;

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:191: text_location.y(), paint);
style nit: Fix the indentation

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:203: const DictionaryValue*
header_footer_info) {
Can you pass header_footer_info as a "const DictionaryValue&"?

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:954: if
(!job_settings.GetString(printing::kSettingHeaderFooterURL, &url))
Can you merge the if conditions @line 948, 954 and 957 together?

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:87: void PrintHeaderAndFooter(SkDevice*
device,
As per style guide, completely global functions should be rarely used. Can you
make this global function as a non member function in an unnamed namespace or a
static member function of PrintWebViewHelper? 

Ref:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

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

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:34: extern const char
kSettingHeaderFooterFontName[] = "Helvetica";
Remove "extern"

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:37: extern const int
kSettingHeaderFooterFontSize = 8;
ditto

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:40: extern const float
kSettingHeaderFooterHorizontalRegions = 3;
ditto

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:44: extern const float
kSettingHeaderFooterInterstice = 7.2f;
ditton

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:48: extern const char
kSettingHeaderFooterTitle[] = "title";
ditto

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:52: extern const char kSettingHeaderFooterURL[]
= "url";
ditto

[Aayush Kumar, 2011-07-28 17:28:41]

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

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:40: #include
"ui/base/text/text_elider.h"
This is not skia specific and it will be used by CoreGraphics as well.  However,
I've put all these header files inside the #if defined (USE_SKIA) block for the
time being. 
On 2011/07/26 20:27:50, kmadhusu wrote:
> can this header file go inside the #if defined(USE_SKIA) block?

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:151: x = -page_layout.margin_left +
On 2011/07/26 20:27:50, kmadhusu wrote:
> Can you rewrite this statement as follows:
> 
> x = printing::kSettingHeaderFooterInterstice - page_layout.margin_left;

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:156: x = (page_layout.content_width +
page_layout.margin_right -
On 2011/07/26 20:27:50, kmadhusu wrote:
> nit: This statement does not require paranthesis since it involves arithmetic
> addition and subtraction.

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:175:
printing::kSettingHeaderFooterInterstice;
On 2011/07/26 20:27:50, kmadhusu wrote:
> How about:
> 
> y = printing::kSettingHeaderFooterInterstice - page_layout.margin_top -
> height_to_baseline;

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:191: text_location.y(), paint);
On 2011/07/26 20:27:50, kmadhusu wrote:
> style nit: Fix the indentation

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:203: const DictionaryValue*
header_footer_info) {
On 2011/07/26 20:27:50, kmadhusu wrote:
> Can you pass header_footer_info as a "const DictionaryValue&"?

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:954: if
(!job_settings.GetString(printing::kSettingHeaderFooterURL, &url))
On 2011/07/26 20:27:50, kmadhusu wrote:
> Can you merge the if conditions @line 948, 954 and 957 together?

Done.

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:87: void PrintHeaderAndFooter(SkDevice*
device,
On 2011/07/26 20:27:50, kmadhusu wrote:
> As per style guide, completely global functions should be rarely used. Can you
> make this global function as a non member function in an unnamed namespace or
a
> static member function of PrintWebViewHelper? 
> 
> Ref:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

Done.

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

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:34: extern const char
kSettingHeaderFooterFontName[] = "Helvetica";
On 2011/07/26 20:27:50, kmadhusu wrote:
> Remove "extern"

Done.

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:37: extern const int
kSettingHeaderFooterFontSize = 8;
On 2011/07/26 20:27:50, kmadhusu wrote:
> ditto

Done.

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:40: extern const float
kSettingHeaderFooterHorizontalRegions = 3;
On 2011/07/26 20:27:50, kmadhusu wrote:
> ditto

Done.

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:44: extern const float
kSettingHeaderFooterInterstice = 7.2f;
On 2011/07/26 20:27:50, kmadhusu wrote:
> ditton

Done.

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:48: extern const char
kSettingHeaderFooterTitle[] = "title";
On 2011/07/26 20:27:50, kmadhusu wrote:
> ditto

Done.

http://codereview.chromium.org/7348010/diff/67028/printing/print_job_constant...
printing/print_job_constants.cc:52: extern const char kSettingHeaderFooterURL[]
= "url";
On 2011/07/26 20:27:50, kmadhusu wrote:
> ditto

Done.

[dpapad, 2011-07-28 20:26:00]

LGTM for the UI with nits.

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:349: html[os=mac]
hr[id='options-horizontal-separator'] {
Instead of hr[id='options-horizontal-separator'], I believe you can use
#options-horizontal-separator directly.

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:353: html[os=mac]
div[id='options-option'] {
Same here.

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:53: </label>
Nit: </label> can fit in previous line.

[kmadhusu, 2011-07-28 20:31:20]

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

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:793:
fadeOutElement($('options-option'));
Can you add line #793 in the else block of line #787?

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

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:165:
(printing::kSettingHeaderFooterInterstice + text_width_in_points);
nit: () is not required.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: }  // namespace
nit: Add blank line before line #202

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:234: &title)) {
nit: Fix indentation for the second argument.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:244:
printing::kSettingHeaderFooterInterstice + header_bounds.height();
nit: "printing::kSettingHeaderFooterInterstic" can go to the previous line.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:256: &url)) {
nit: Fix indentation.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:266:
printing::kSettingHeaderFooterInterstice + footer_bounds.height();
" printing::kSettingHeaderFooterInterstice " can go to the previous line.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:293: page_layout, segment_width, font,
|page_layout| can go to the previous line.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: // and footers using strings from
|header_footer_info| on to the canvas.
Add comments after #defined(USE_SKIA)

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:302: void PrintHeaderAndFooter(SkDevice*
device,
Add the "static" keyword to the function signature.

[Aayush Kumar, 2011-07-29 00:21:46]

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.css (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:349: html[os=mac]
hr[id='options-horizontal-separator'] {
On 2011/07/28 20:26:00, dpapad wrote:
> Instead of hr[id='options-horizontal-separator'], I believe you can use
> #options-horizontal-separator directly.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.css:353: html[os=mac]
div[id='options-option'] {
On 2011/07/28 20:26:00, dpapad wrote:
> Same here.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.html (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.html:53: </label>
On 2011/07/28 20:26:00, dpapad wrote:
> Nit: </label> can fit in previous line.

Done.

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

http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:793:
fadeOutElement($('options-option'));
On 2011/07/28 20:31:20, kmadhusu wrote:
> Can you add line #793 in the else block of line #787?

Done.

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

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:165:
(printing::kSettingHeaderFooterInterstice + text_width_in_points);
On 2011/07/28 20:31:20, kmadhusu wrote:
> nit: () is not required.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:202: }  // namespace
On 2011/07/28 20:31:20, kmadhusu wrote:
> nit: Add blank line before line #202

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:234: &title)) {
On 2011/07/28 20:31:20, kmadhusu wrote:
> nit: Fix indentation for the second argument.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:244:
printing::kSettingHeaderFooterInterstice + header_bounds.height();
Steve actually suggested this for better readability.  See:
http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_v...

On 2011/07/28 20:31:20, kmadhusu wrote:
> nit: "printing::kSettingHeaderFooterInterstic" can go to the previous line.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:256: &url)) {
On 2011/07/28 20:31:20, kmadhusu wrote:
> nit: Fix indentation.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:266:
printing::kSettingHeaderFooterInterstice + footer_bounds.height();
Same as above comment.
On 2011/07/28 20:31:20, kmadhusu wrote:
> " printing::kSettingHeaderFooterInterstice " can go to the previous line.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:293: page_layout, segment_width, font,
On 2011/07/28 20:31:20, kmadhusu wrote:
> |page_layout| can go to the previous line.

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:300: // and footers using strings from
|header_footer_info| on to the canvas.
On 2011/07/28 20:31:20, kmadhusu wrote:
> Add comments after #defined(USE_SKIA)

Done.

http://codereview.chromium.org/7348010/diff/70020/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:302: void PrintHeaderAndFooter(SkDevice*
device,
On 2011/07/28 20:31:20, kmadhusu wrote:
> Add the "static" keyword to the function signature.

Done.

[kmadhusu, 2011-08-01 17:17:08]

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

http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:494:
settings->SetString(printing::kSettingHeaderFooterTitle,
Set the header and footer info in  |settings| only if we need to display the
header and footer.

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

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1000: if
(!job_settings.GetBoolean(printing::kSettingHeaderFooter,
(repeating our in-person conversation): Remove the header & footer settings info
from job_settings after reading those values.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1008:
header_footer_info_->SetString(printing::kSettingHeaderFooterURL, url);
Populate the header_footer_info_ only if |display_header_footer| is true.

[vandebo (ex-Chrome), 2011-08-01 21:07:37]

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

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:49: #endif
  // USE_SKIA

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
I'm actually not sure of the indent here... should this go in four to account
for the ( ?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:130:
printing::kSettingHeaderFooterInterstice) /
nit: extra space.  Sorry if I mislead you here before.  The the ()'s matched on
the previous line.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:138: SkPaint paint,
const SkPaint& paint

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:145: gfx::Font font,
const gfx::Font& font

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:146: SkScalar height_to_baseline) {
nit: this might be better named space_for_descenders ?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
I don't think you need both this and x & y; use one or the other.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:182: y =
printing::kSettingHeaderFooterInterstice -
This is putting the text just above the content area.  Do we want to put this
just below the top of the page?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in
the margins.
nit: comment isn't needed.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Setting up styles for the
headers and footers text.
nit: comment isn't needed

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:231: // headers.
I think this comment and the one on 250 should go down to 269?  But then it
might not be needed?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:248: return;
What if there is room for the footers, but not the headers?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:299: #if defined(USE_SKIA)
nit: put this in the helper section with the other static methods.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:304: skia::VectorCanvas& canvas,
We don't use non-const references, make this const or a pointer.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:167:
PrintPageInternal(page_params, canvas_size, frame, false, metafile);
Instead of changing the signature of this method, why not just set
params.params.display_header_footer = false before the for loop?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:40: page_number, page_slot, frame,
false, &metafile);
Same here.

[Aayush Kumar, 2011-08-02 00:18:13]

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

http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:494:
settings->SetString(printing::kSettingHeaderFooterTitle,
On 2011/08/01 17:17:08, kmadhusu wrote:
> Set the header and footer info in  |settings| only if we need to display the
> header and footer. 

Done.

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

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:49: #endif
On 2011/08/01 21:07:37, vandebo wrote:
>   // USE_SKIA

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
On 2011/08/01 21:07:37, vandebo wrote:
> I'm actually not sure of the indent here... should this go in four to account
> for the ( ?

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:130:
printing::kSettingHeaderFooterInterstice) /
On 2011/08/01 21:07:37, vandebo wrote:
> nit: extra space.  Sorry if I mislead you here before.  The the ()'s matched
on
> the previous line.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:138: SkPaint paint,
I don't want to pass a reference here, since, I am changing the text size within
paint which I don't want reflected in the parent function.
On 2011/08/01 21:07:37, vandebo wrote:
> const SkPaint& paint

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:145: gfx::Font font,
On 2011/08/01 21:07:37, vandebo wrote:
> const gfx::Font& font

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:146: SkScalar height_to_baseline) {
On 2011/08/01 21:07:37, vandebo wrote:
> nit: this might be better named space_for_descenders ?

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
On 2011/08/01 21:07:37, vandebo wrote:
> I don't think you need both this and x & y; use one or the other.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:182: y =
printing::kSettingHeaderFooterInterstice -
No, this actually doesn't do that.  It places it right below the top of the
page.  (Notice the "-page_layout.margin_top"
On 2011/08/01 21:07:37, vandebo wrote:
> This is putting the text just above the content area.  Do we want to put this
> just below the top of the page?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in
the margins.
On 2011/08/01 21:07:37, vandebo wrote:
> nit: comment isn't needed.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Setting up styles for the
headers and footers text.
Can I leave this comment?  I found it a lot easier to read my own code with
these comments :)
On 2011/08/01 21:07:37, vandebo wrote:
> nit: comment isn't needed

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:231: // headers.
This is actually checked for on line 244.  
On 2011/08/01 21:07:37, vandebo wrote:
> I think this comment and the one on 250 should go down to 269?  But then it
> might not be needed?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:248: return;
I am going for all or nothing.  Just want to be consistent. :)
On 2011/08/01 21:07:37, vandebo wrote:
> What if there is room for the footers, but not the headers?

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1000: if
(!job_settings.GetBoolean(printing::kSettingHeaderFooter,
On 2011/08/01 17:17:08, kmadhusu wrote:
> (repeating our in-person conversation): Remove the header & footer settings
info
> from job_settings after reading those values.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1008:
header_footer_info_->SetString(printing::kSettingHeaderFooterURL, url);
On 2011/08/01 17:17:08, kmadhusu wrote:
> Populate the header_footer_info_ only if |display_header_footer| is true.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:299: #if defined(USE_SKIA)
On 2011/08/01 21:07:37, vandebo wrote:
> nit: put this in the helper section with the other static methods.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:304: skia::VectorCanvas& canvas,
On 2011/08/01 21:07:37, vandebo wrote:
> We don't use non-const references, make this const or a pointer.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:167:
PrintPageInternal(page_params, canvas_size, frame, false, metafile);
On 2011/08/01 21:07:37, vandebo wrote:
> Instead of changing the signature of this method, why not just set
> params.params.display_header_footer = false before the for loop?

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_mac.mm:40: page_number, page_slot, frame,
false, &metafile);
On 2011/08/01 21:07:37, vandebo wrote:
> Same here.

Done.

[Aayush Kumar, 2011-08-02 01:26:14]

Made some style changes to the latest patch :)

[Lei Zhang, 2011-08-03 01:28:30]

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

http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:495: bool
display_header_footer;
If you don't initialize |display_header_footer|, you may get nondeterministic
behavior in release builds if you ever hit the NOTREACHED().

[Aayush Kumar, 2011-08-03 15:52:53]

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

http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:495: bool
display_header_footer;
On 2011/08/03 01:28:31, Lei Zhang wrote:
> If you don't initialize |display_header_footer|, you may get nondeterministic
> behavior in release builds if you ever hit the NOTREACHED().

Done.

[vandebo (ex-Chrome), 2011-08-03 19:00:05]

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

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in
the margins.
On 2011/08/02 00:18:13, Aayush Kumar wrote:
> On 2011/08/01 21:07:37, vandebo wrote:
> > nit: comment isn't needed.
> 
> Done.

Done in spirit only ;-)

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Setting up styles for the
headers and footers text.
On 2011/08/02 00:18:13, Aayush Kumar wrote:
> Can I leave this comment?  I found it a lot easier to read my own code with
> these comments :)
> On 2011/08/01 21:07:37, vandebo wrote:
> > nit: comment isn't needed
> 

Meta comment - it's harder to see replies when you top post, please bottom post,
at least for short comments.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:231: // headers.
On 2011/08/02 00:18:13, Aayush Kumar wrote:
> This is actually checked for on line 244.  
> On 2011/08/01 21:07:37, vandebo wrote:
> > I think this comment and the one on 250 should go down to 269?  But then it
> > might not be needed?
> 

Why don't we do this... move the conditionals about fitting or not down to the
calls to PrintHeaderFooterText (to satisfy my next comment) then the code
between here and there will be calculating the height the text, and you can have
a comment, "Get the height of the headers and footers to ensure it will fit in
the margins" or something along those lines.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:248: return;
On 2011/08/02 00:18:13, Aayush Kumar wrote:
> I am going for all or nothing.  Just want to be consistent. :)
> On 2011/08/01 21:07:37, vandebo wrote:
> > What if there is room for the footers, but not the headers?
> 

It seems to me that if there's still room for the footers and the user said they
want footers, we should print footers.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
nit: indent should be 4.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:135: // coordinates and prints the text
at those coordinates on the canvas.
nit: indent should be 4.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:148: false);
POD should be passed in directly... i.e. SkScalar max_text_width,

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
and here

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
I guess I misunderstood before... this isn't the space for descenders when
printing to the top.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: }
{}'s are not needed on the cases statements in this switch or the next.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:242: &header_bounds, 0);
Put header_bounds below header_text

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:264: &footer_bounds, 0);
Put footer_bounds below footer_text

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:276: header_bounds.top());
Redundant comment.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:281: // Calculate the available title
width. If the date string is not long
Redundant comment.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:294: segment_width, font,
footer_bounds.bottom());
Redundant comment.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:299: page_of_total_pages.c_str(),
Redundant comment.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:320: }
Redundant comment.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1002:
!job_settings.GetString(printing::kSettingHeaderFooterTitle, &title) ||
I don't think it's worth the effort to remove these settings.  The const cast is
much uglier than just passing on the unneeded settings.

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

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constant...
printing/print_job_constants.cc:31: const char kSettingHeaderFooter[] =
"headerFooter";
headerFooterEnabled ?

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constant...
printing/print_job_constants.h:17: extern const char kSettingHeaderFooter[];
In the block, it's not clear what this one is... Maybe
kSettingHeaderFooterEnabled[]

[Aayush Kumar, 2011-08-04 18:25:04]

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

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in
the margins.
On 2011/08/03 19:00:05, vandebo wrote:
> On 2011/08/02 00:18:13, Aayush Kumar wrote:
> > On 2011/08/01 21:07:37, vandebo wrote:
> > > nit: comment isn't needed.
> > 
> > Done.
> 
> Done in spirit only ;-)

Done this time - sorry :)

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:219: // Setting up styles for the
headers and footers text.
On 2011/08/03 19:00:05, vandebo wrote:
> On 2011/08/02 00:18:13, Aayush Kumar wrote:
> > Can I leave this comment?  I found it a lot easier to read my own code with
> > these comments :)
> > On 2011/08/01 21:07:37, vandebo wrote:
> > > nit: comment isn't needed
> > 
> 
> Meta comment - it's harder to see replies when you top post, please bottom
post,
> at least for short comments.

Ahh okay :)  It's weird though because when I click the reply button, the cursor
is on top of the box, whereas when I click on "Done" - it automatically adds
that to the end.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:231: // headers.
On 2011/08/03 19:00:05, vandebo wrote:
> On 2011/08/02 00:18:13, Aayush Kumar wrote:
> > This is actually checked for on line 244.  
> > On 2011/08/01 21:07:37, vandebo wrote:
> > > I think this comment and the one on 250 should go down to 269?  But then
it
> > > might not be needed?
> > 
> 
> Why don't we do this... move the conditionals about fitting or not down to the
> calls to PrintHeaderFooterText (to satisfy my next comment) then the code
> between here and there will be calculating the height the text, and you can
have
> a comment, "Get the height of the headers and footers to ensure it will fit in
> the margins" or something along those lines.

Done.

http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:248: return;
On 2011/08/03 19:00:05, vandebo wrote:
> On 2011/08/02 00:18:13, Aayush Kumar wrote:
> > I am going for all or nothing.  Just want to be consistent. :)
> > On 2011/08/01 21:07:37, vandebo wrote:
> > > What if there is room for the footers, but not the headers?
> > 
> 
> It seems to me that if there's still room for the footers and the user said
they
> want footers, we should print footers.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
On 2011/08/03 19:00:05, vandebo wrote:
> nit: indent should be 4.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:135: // coordinates and prints the text
at those coordinates on the canvas.
On 2011/08/03 19:00:05, vandebo wrote:
> nit: indent should be 4.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:148: false);
On 2011/08/03 19:00:05, vandebo wrote:
> POD should be passed in directly... i.e. SkScalar max_text_width,

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
On 2011/08/03 19:00:05, vandebo wrote:
> and here

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: SkPoint text_location;
On 2011/08/03 19:00:05, vandebo wrote:
> I guess I misunderstood before... this isn't the space for descenders when
> printing to the top.

Renamed back to "height_to_baseline"

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: }
On 2011/08/03 19:00:05, vandebo wrote:
> {}'s are not needed on the cases statements in this switch or the next.

According to the google C++ style guide, braces seem to be allowed.
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...)

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:242: &header_bounds, 0);
On 2011/08/03 19:00:05, vandebo wrote:
> Put header_bounds below header_text

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:264: &footer_bounds, 0);
On 2011/08/03 19:00:05, vandebo wrote:
> Put footer_bounds below footer_text

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:276: header_bounds.top());
On 2011/08/03 19:00:05, vandebo wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:281: // Calculate the available title
width. If the date string is not long
On 2011/08/03 19:00:05, vandebo wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:294: segment_width, font,
footer_bounds.bottom());
On 2011/08/03 19:00:05, vandebo wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:299: page_of_total_pages.c_str(),
On 2011/08/03 19:00:05, vandebo wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:320: }
On 2011/08/03 19:00:05, vandebo wrote:
> Redundant comment.

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1002:
!job_settings.GetString(printing::kSettingHeaderFooterTitle, &title) ||
On 2011/08/03 19:00:05, vandebo wrote:
> I don't think it's worth the effort to remove these settings.  The const cast
is
> much uglier than just passing on the unneeded settings.

Done.

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

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constant...
printing/print_job_constants.cc:31: const char kSettingHeaderFooter[] =
"headerFooter";
On 2011/08/03 19:00:05, vandebo wrote:
> headerFooterEnabled ?

Done.

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7348010/diff/95001/printing/print_job_constant...
printing/print_job_constants.h:17: extern const char kSettingHeaderFooter[];
On 2011/08/03 19:00:05, vandebo wrote:
> In the block, it's not clear what this one is... Maybe
> kSettingHeaderFooterEnabled[]

Done.

[kmadhusu, 2011-08-04 19:15:15]

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
style nit: 4 space indentation is not required here.

Ref: http://www.chromium.org/developers/coding-style

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:292: page_layout.margin_right) * 3 / 4;
style nit: 4 space indentation is not required here.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1016: &display_header_footer)) {
Fix indentation. Remove a blank space in front of "&display_header_footer".

[kmadhusu, 2011-08-04 19:49:17]

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1005: settings.params.margin_top = 0;
Why are you resetting the margin top to zero?

[vandebo (ex-Chrome), 2011-08-04 20:46:21]

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

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: const SkScalar&
space_for_descenders) {
On 2011/08/04 18:25:04, Aayush Kumar wrote:
> On 2011/08/03 19:00:05, vandebo wrote:
> > I guess I misunderstood before... this isn't the space for descenders when
> > printing to the top.
> 
> Renamed back to "height_to_baseline"

nit: hmm, maybe offset_to_baseline would be better - for the top, it's not
really height (since it's downward).

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: case printing::LEFT: {
On 2011/08/04 18:25:04, Aayush Kumar wrote:
> On 2011/08/03 19:00:05, vandebo wrote:
> > {}'s are not needed on the cases statements in this switch or the next.
> 
> According to the google C++ style guide, braces seem to be allowed.
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...)

Indeed, but the overriding rule is to follow existing style.  Chrome's existing
style is to only include the {}'s if needed - i.e. if there are variables
introduced inside the {}'s.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:9: #include <cmath>
what's cmath for?

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:124: // between each segment. Returns
the width of this segment.
nit: "...of this segment." => of a segment.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
On 2011/08/04 19:15:15, kmadhusu wrote:
> style nit: 4 space indentation is not required here.
> 
> Ref: http://www.chromium.org/developers/coding-style

Oops, Mea culpa

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:217: const DictionaryValue&
header_footer_info) {
nit: I don't follow your code sectioning.  I suggest bringing 219 up to the
previous block and remove the new lines at 223 and 226.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:235: string16 date =
base::TimeFormatShortDateNumeric(Time::Now());
nit: Push this below the comment (240).

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:238: &title)) {
nit: ", then, we " => ", "
"we have " => ""

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:249: 
Same comments as PrintHeaders

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:319: 
nit: comment seems unnecessary.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:332:
dpi_(static_cast<int>(print_params.dpi)),
nit: extra space.

[Aayush Kumar, 2011-08-04 23:00:30]

To Do:  Add some more JS tests.  But, that can be a separate CL as well, if
that's not ready in time :)

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

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:150: const SkScalar&
space_for_descenders) {
On 2011/08/04 20:46:21, vandebo wrote:
> On 2011/08/04 18:25:04, Aayush Kumar wrote:
> > On 2011/08/03 19:00:05, vandebo wrote:
> > > I guess I misunderstood before... this isn't the space for descenders when
> > > printing to the top.
> > 
> > Renamed back to "height_to_baseline"
> 
> nit: hmm, maybe offset_to_baseline would be better - for the top, it's not
> really height (since it's downward).

Done.

http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:162: case printing::LEFT: {
On 2011/08/04 20:46:21, vandebo wrote:
> On 2011/08/04 18:25:04, Aayush Kumar wrote:
> > On 2011/08/03 19:00:05, vandebo wrote:
> > > {}'s are not needed on the cases statements in this switch or the next.
> > 
> > According to the google C++ style guide, braces seem to be allowed.
> >
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_...)
> 
> Indeed, but the overriding rule is to follow existing style.  Chrome's
existing
> style is to only include the {}'s if needed - i.e. if there are variables
> introduced inside the {}'s.

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:9: #include <cmath>
On 2011/08/04 20:46:22, vandebo wrote:
> what's cmath for?

I use ceil (while declaring gfx::font), as gfx::font takes an int as the size.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:124: // between each segment. Returns
the width of this segment.
On 2011/08/04 20:46:22, vandebo wrote:
> nit: "...of this segment." => of a segment.

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width +
On 2011/08/04 19:15:15, kmadhusu wrote:
> style nit: 4 space indentation is not required here.
> 
> Ref: http://www.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:217: const DictionaryValue&
header_footer_info) {
On 2011/08/04 20:46:22, vandebo wrote:
> nit: I don't follow your code sectioning.  I suggest bringing 219 up to the
> previous block and remove the new lines at 223 and 226.

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:235: string16 date =
base::TimeFormatShortDateNumeric(Time::Now());
On 2011/08/04 20:46:22, vandebo wrote:
> nit: Push this below the comment (240).

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:238: &title)) {
On 2011/08/04 20:46:22, vandebo wrote:
> nit: ", then, we " => ", "
> "we have " => ""

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:249: 
On 2011/08/04 20:46:22, vandebo wrote:
> Same comments as PrintHeaders

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:292: max_title_width, font,
header_bounds.top());
On 2011/08/04 19:15:15, kmadhusu wrote:
> style nit: 4 space indentation is not required here. 

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:319: 
On 2011/08/04 20:46:22, vandebo wrote:
> nit: comment seems unnecessary.

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:332:
dpi_(static_cast<int>(print_params.dpi)),
On 2011/08/04 20:46:22, vandebo wrote:
> nit: extra space.

Done.

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1005: 
On 2011/08/04 19:49:17, kmadhusu wrote:
> Why are you resetting the margin top to zero?

sorry about that! :)

http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:1016: 
On 2011/08/04 19:15:15, kmadhusu wrote:
> Fix indentation. Remove a blank space in front of "&display_header_footer".

Done.

[vandebo (ex-Chrome), 2011-08-05 00:00:49]

C++ LGTM

[dpapad, 2011-08-05 00:34:59]

http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/pri...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/pri...
chrome/test/data/webui/print_preview.js:292:
checkSectionVisible($('options-option'), false);
This test fails as it is. Why should the headers/footers been hidden here?

Also I dont think this is the right test to check for the visibility of the
headers and footers. This test checks whether the settings reflect the
capabilities of the currently selected printer, but headers and footers do not
depend on the printer, right? If so, I believe a new test, specific to headers
and footers.

[Aayush Kumar, 2011-08-05 00:52:39]

http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/pri...
File chrome/test/data/webui/print_preview.js (right):

http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/pri...
chrome/test/data/webui/print_preview.js:292:
checkSectionVisible($('options-option'), false);
On 2011/08/05 00:34:59, dpapad wrote:
> This test fails as it is. Why should the headers/footers been hidden here?
> 
> Also I dont think this is the right test to check for the visibility of the
> headers and footers. This test checks whether the settings reflect the
> capabilities of the currently selected printer, but headers and footers do not
> depend on the printer, right? If so, I believe a new test, specific to headers
> and footers.

Restored change - will create a separate CL for JS tests.

[kmadhusu, 2011-08-05 15:44:11]

C++ code LGTM

[Lei Zhang, 2011-08-05 18:58:21]

http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:140: void PrintHeaderFooterText(
nit: PrintHeaderFooterText() and PrintHeaders() have many common parameters, but
they are in a different order. Can you reorder them? Have you considered
combining some of the data into a struct instead of having 10 parameters to the
function?

[Aayush Kumar, 2011-08-08 03:19:22]

http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:140: void PrintHeaderFooterText(
On 2011/08/05 18:58:21, Lei Zhang wrote:
> nit: PrintHeaderFooterText() and PrintHeaders() have many common parameters,
but
> they are in a different order. Can you reorder them? Have you considered
> combining some of the data into a struct instead of having 10 parameters to
the
> function?

Reordered function params.
As discussed in person, decided not to create a struct since we have mostly
dis-similar items, and also, that this function is within an unnamed namespace.

[Aayush Kumar, 2011-08-11 21:20:16]

Moved ElideText call to browser process.

[vandebo (ex-Chrome), 2011-08-11 23:32:13]

A couple general comments.

http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:320: void
UpdateHeaderFooterStrings(const base::DictionaryValue& header_footer_info,
What process adds the strings to params?  Can the ElideText calls be done there?

http://codereview.chromium.org/7348010/diff/130001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/130001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:200: void
PrintFooters(skia::VectorCanvas* canvas,
PrintHeaders and PrintFooters don't do much.  I think you can push the content
of these two functions into PrintHeaderAndFooter with the actual calls to
PrintHeaderFooterText in if blocks.

http://codereview.chromium.org/7348010/diff/130001/printing/units.h
File printing/units.h (right):

http://codereview.chromium.org/7348010/diff/130001/printing/units.h#newcode48
printing/units.h:48: int GetDPI(const PrintMsg_Print_Params* print_params);
const ref?

[Aayush Kumar, 2011-08-12 08:35:40]

I haven't tested this on windows and mac (it's still building), so there might
be a couple of small compile errors.  But, for the most part, the flow will
remain as it is.

http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:320: void
UpdateHeaderFooterStrings(const base::DictionaryValue& header_footer_info,
On 2011/08/11 23:32:13, vandebo wrote:
> What process adds the strings to params?  Can the ElideText calls be done
there?

As discussed, this is the browser process. :)

http://codereview.chromium.org/7348010/diff/130001/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/130001/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:200: void
PrintFooters(skia::VectorCanvas* canvas,
On 2011/08/11 23:32:13, vandebo wrote:
> PrintHeaders and PrintFooters don't do much.  I think you can push the content
> of these two functions into PrintHeaderAndFooter with the actual calls to
> PrintHeaderFooterText in if blocks.

Done.

http://codereview.chromium.org/7348010/diff/130001/printing/units.h
File printing/units.h (right):

http://codereview.chromium.org/7348010/diff/130001/printing/units.h#newcode48
printing/units.h:48: int GetDPI(const PrintMsg_Print_Params* print_params);
On 2011/08/11 23:32:13, vandebo wrote:
> const ref?

I have restored this change since I no longer need it.  Will make the change in
the function header as a separate CL :)

[kmadhusu, 2011-08-12 17:07:00]

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:104:
newParams.params.display_header_footer &&
Also check for individual values.

http://codereview.chromium.org/7348010/diff/130010/printing/header_footer_ini...
File printing/header_footer_initializer.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/header_footer_ini...
printing/header_footer_initializer.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
nit: 2011

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
File printing/printing_context_mac.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
printing/printing_context_mac.h:52: const DictionaryValue& header_footer_info);
document |header_footer_info|

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
printing/printing_context_win.cc:398: bool display_header_footer;
Move this common code to a helper function in print_context.cc

[kmadhusu, 2011-08-12 17:21:31]

http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:60: if
(!settings.display_header_footer)
You need to set params->display_header_footer

http://codereview.chromium.org/7348010/diff/130010/printing/print_settings_in...
File printing/print_settings_initializer_gtk.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/print_settings_in...
printing/print_settings_initializer_gtk.h:17: using base::DictionaryValue;
nit: Add a blank line before this.

[Aayush Kumar, 2011-08-12 18:53:42]

http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:60: if
(!settings.display_header_footer)
On 2011/08/12 17:21:32, kmadhusu wrote:
> You need to set params->display_header_footer

Done.

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:104:
newParams.params.display_header_footer &&
On 2011/08/12 17:07:00, kmadhusu wrote:
> Also check for individual values.

Is there a likely scenario when this would change and the others wouldn't?  
Currently, I initialize the individual header footer strings only if headers and
footers are enabled.

http://codereview.chromium.org/7348010/diff/130010/printing/header_footer_ini...
File printing/header_footer_initializer.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/header_footer_ini...
printing/header_footer_initializer.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/08/12 17:07:00, kmadhusu wrote:
> nit: 2011

Done.

http://codereview.chromium.org/7348010/diff/130010/printing/print_settings_in...
File printing/print_settings_initializer_gtk.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/print_settings_in...
printing/print_settings_initializer_gtk.h:17: using base::DictionaryValue;
On 2011/08/12 17:21:32, kmadhusu wrote:
> nit: Add a blank line before this.

Done.

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
File printing/printing_context_mac.h (right):

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
printing/printing_context_mac.h:52: const DictionaryValue& header_footer_info);
On 2011/08/12 17:07:00, kmadhusu wrote:
> document |header_footer_info|

Done.

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/7348010/diff/130010/printing/printing_context_...
printing/printing_context_win.cc:398: bool display_header_footer;
On 2011/08/12 17:07:00, kmadhusu wrote:
> Move this common code to a helper function in print_context.cc

Done.

[kmadhusu, 2011-08-12 20:27:11]

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:104:
newParams.params.display_header_footer &&
On 2011/08/12 18:53:42, Aayush Kumar wrote:
> On 2011/08/12 17:07:00, kmadhusu wrote:
> > Also check for individual values.
> 
> Is there a likely scenario when this would change and the others wouldn't?  
> Currently, I initialize the individual header footer strings only if headers
and
> footers are enabled.

User can change the system date & time in between two preview requests.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:203: string16 date;
nit: Fix indentation

http://codereview.chromium.org/7348010/diff/126086/printing/printing_context_...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/7348010/diff/126086/printing/printing_context_...
printing/printing_context_mac.mm:101:
header_footer_info.SetBoolean(printing::kSettingHeaderFooterEnabled,
nit: If possible, have a function that populates this default header footer info
(which is not to display the header and footer in the preview data).

[vandebo (ex-Chrome), 2011-08-12 22:18:20]

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:925: // Getting Header and Footer
settings.
Unnecessary comment.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:152: PrintMsg_Print_Params
printParams = print_preview_context_.print_params();
const ref variable, otherwise you're making a copy.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:191: if (is_preview &&
params.display_header_footer) {
is_preview isn't needed

[Aayush Kumar, 2011-08-13 04:04:52]

Moved code around as per Steve's suggestions.  Looks a lot better now :)

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:203:
!header_footer_info.GetString(printing::kSettingHeaderFooterDate,
On 2011/08/12 20:27:11, kmadhusu wrote:
> nit: Fix indentation

Done.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:925: // Getting Header and Footer
settings.
On 2011/08/12 22:18:20, vandebo wrote:
> Unnecessary comment.

Done.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_mac.mm:152: PrintMsg_Print_Params
printParams = print_preview_context_.print_params();
On 2011/08/12 22:18:20, vandebo wrote:
> const ref variable, otherwise you're making a copy.

Done.

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper_win.cc:191: if (is_preview &&
params.display_header_footer) {
On 2011/08/12 22:18:20, vandebo wrote:
> is_preview isn't needed

Done.

http://codereview.chromium.org/7348010/diff/126086/printing/printing_context_...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/7348010/diff/126086/printing/printing_context_...
printing/printing_context_mac.mm:101:
header_footer_info.SetBoolean(printing::kSettingHeaderFooterEnabled,
On 2011/08/12 20:27:11, kmadhusu wrote:
> nit: If possible, have a function that populates this default header footer
info
> (which is not to display the header and footer in the preview data). 

Done.

[Aayush Kumar, 2011-08-13 05:47:32]

Refactored the JS code

[kmadhusu, 2011-08-15 16:46:53]

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:100:
newParams.params.display_header_footer &&
As I said earlier, you need to compare the individual header footer text.

http://codereview.chromium.org/7348010/diff/138004/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/7348010/diff/138004/printing/printing_context....
printing/printing_context.h:56: virtual Result UpdatePrinterSettings(
How about "UpdatePrintJobSettings"?

[Aayush Kumar, 2011-08-15 18:25:37]

+jhawkins as a reviewer for the JS refactoring

[vandebo (ex-Chrome), 2011-08-15 19:35:48]

Just nits - LGTM

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:926: header_footer_info_.reset(new
DictionaryValue());
Why do we need header_footer_info_ ? Can't we just access print_pages_params_
directly from the context where we call PrintHeaderAndFooter ?

I guess that would provide a bunch of irrelevant information to the RenderPage
functions.... nevermind.

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:330: // Contains strings generated by
the browser process to be printed as headers
nit: remove "Contains"

http://codereview.chromium.org/7348010/diff/138004/printing/printing_context.h
File printing/printing_context.h (right):

http://codereview.chromium.org/7348010/diff/138004/printing/printing_context....
printing/printing_context.h:56: virtual Result UpdatePrinterSettings(
On 2011/08/15 16:46:54, kmadhusu wrote:
> How about "UpdatePrintJobSettings"?

I think the reason for changing the name is to distinguish between the printer /
platform specific settings and the platform independent ones, right?  If so, I
think UpdatePrinterSettings does that better than UpdatePrintJobSettings.

http://codereview.chromium.org/7348010/diff/138004/printing/units.h
File printing/units.h (right):

http://codereview.chromium.org/7348010/diff/138004/printing/units.h#newcode44
printing/units.h:44: double GetSegmentWidth(double page_width);
The name of this could be improved in this file.... GetHeaderFooterSegmentWidth
?

[James Hawkins, 2011-08-15 19:39:35]

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/header_footer_settings.html (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/header_footer_settings.html:4: <input
id="header-footer" type="checkbox" checked/>
Do not close single tags.

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/header_footer_settings.js:31: *
Remove blank comment line.

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:969:
this.hasHeaderFooter = '';
This is a boolean var, is it not?

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:44: #endif  // USE_SKIA
#endif  // defined(USE_SKIA)

Here and elsewhere.

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:142: case printing::CENTER: {
Either use braces for all cases or none.

[Aayush Kumar, 2011-08-15 21:37:37]

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/header_footer_settings.html (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/header_footer_settings.html:4: <input
id="header-footer" type="checkbox" checked/>
On 2011/08/15 19:39:35, James Hawkins wrote:
> Do not close single tags.

Done.

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/header_footer_settings.js (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/header_footer_settings.js:31: *
On 2011/08/15 19:39:35, James Hawkins wrote:
> Remove blank comment line.

Done.

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/p...
chrome/browser/resources/print_preview/print_preview.js:969:
this.hasHeaderFooter = '';
On 2011/08/15 19:39:35, James Hawkins wrote:
> This is a boolean var, is it not?

Rebased and this section has now disappeared :)

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:44: #endif  // USE_SKIA
On 2011/08/15 19:39:35, James Hawkins wrote:
> #endif  // defined(USE_SKIA)
> 
> Here and elsewhere.

Done.

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:100:
newParams.params.display_header_footer &&
On 2011/08/15 16:46:54, kmadhusu wrote:
> As I said earlier, you need to compare the individual header footer text.

Done.

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.cc:142: case printing::CENTER: {
On 2011/08/15 19:39:35, James Hawkins wrote:
> Either use braces for all cases or none.

Done.

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_...
chrome/renderer/print_web_view_helper.h:330: // Contains strings generated by
the browser process to be printed as headers
On 2011/08/15 19:35:49, vandebo wrote:
> nit: remove "Contains"

Done.

http://codereview.chromium.org/7348010/diff/138004/printing/units.h
File printing/units.h (right):

http://codereview.chromium.org/7348010/diff/138004/printing/units.h#newcode44
printing/units.h:44: double GetSegmentWidth(double page_width);
On 2011/08/15 19:35:49, vandebo wrote:
> The name of this could be improved in this file....
GetHeaderFooterSegmentWidth
> ?

Done.

[James Hawkins, 2011-08-15 21:41:24]

LGTM with include nit.

http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_in...
File printing/print_settings_initializer.h (right):

http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_in...
printing/print_settings_initializer.h:8: #include "base/logging.h"
base/basictypes.h

[Aayush Kumar, 2011-08-15 22:16:56]

http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_in...
File printing/print_settings_initializer.h (right):

http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_in...
printing/print_settings_initializer.h:8: #include "base/logging.h"
On 2011/08/15 21:41:24, James Hawkins wrote:
> base/basictypes.h

Done.

[kmadhusu, 2011-08-15 22:19:50]

LGTM

[Aayush Kumar, 2011-08-16 06:24:43]

Fixed failing tests.

[vandebo (ex-Chrome), 2011-08-16 17:47:46]

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:58: assert(sizeof(PrintMsg_Print_Params_Clone)
== sizeof(PrintMsg_Print_Params));
Use COMPILE_ASSERT so that the problem is caught at compile time, not run time.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:72:
assert(sizeof(PrintMsg_PrintPages_Params_Clone) ==
and here

[kmadhusu, 2011-08-16 18:08:34]

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:70: params->pages = pages_;
On Release version, MSVS crashes the application if we try to assign an empty
vector to an empty vector. Change this to
if (pages_.size() != 0)
  params->pages = pages_;

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:154: // Resets the members of |params| to 0.
Checks to see if
Add a blank line before this.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:158: private:
Add a blank line before this.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:176: };
Add "DISALLOW_COPY_AND_ASSIGN" in both the classes.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:183: // Resets the members of |params| to 0.
Checks to see if
Add a blank line before this.

[Aayush Kumar, 2011-08-16 20:32:52]

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:58: assert(sizeof(PrintMsg_Print_Params_Clone)
== sizeof(PrintMsg_Print_Params));
On 2011/08/16 17:47:46, vandebo wrote:
> Use COMPILE_ASSERT so that the problem is caught at compile time, not run
time.

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:70: params->pages = pages_;
On 2011/08/16 18:08:34, kmadhusu wrote:
> On Release version, MSVS crashes the application if we try to assign an empty
> vector to an empty vector. Change this to
> if (pages_.size() != 0)
>   params->pages = pages_;

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:72:
assert(sizeof(PrintMsg_PrintPages_Params_Clone) ==
On 2011/08/16 17:47:46, vandebo wrote:
> and here

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.h (right):

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:154: // Resets the members of |params| to 0.
Checks to see if
On 2011/08/16 18:08:34, kmadhusu wrote:
> Add a blank line before this. 

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:158: private:
On 2011/08/16 18:08:34, kmadhusu wrote:
> Add a blank line before this. 

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:176: };
On 2011/08/16 18:08:34, kmadhusu wrote:
> Add "DISALLOW_COPY_AND_ASSIGN" in both the classes.

Done.

http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_print...
chrome/renderer/mock_printer.h:183: // Resets the members of |params| to 0.
Checks to see if
On 2011/08/16 18:08:34, kmadhusu wrote:
> Add a blank line before this.

Done.

[vandebo (ex-Chrome), 2011-08-16 20:35:31]

try jobs = LG

[kmadhusu, 2011-08-16 21:00:28]

http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:73: params->pages.clear();
Sorry for the false alarm.. It looks like memset() was the main reason for the
crash which I was talking about. Since you have removed the memset, It should be
safe to assign "pages" to "params->pages". Please revert this change.

[Aayush Kumar, 2011-08-16 22:33:18]

http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_print...
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_print...
chrome/renderer/mock_printer.cc:73: params->pages.clear();
On 2011/08/16 21:00:28, kmadhusu wrote:
> Sorry for the false alarm.. It looks like memset() was the main reason for the
> crash which I was talking about. Since you have removed the memset, It should
be
> safe to assign "pages" to "params->pages". Please revert this change. 

Done.

[kmadhusu, 2011-08-16 22:39:04]

LGTM.

Thanks.

[Aayush Kumar, 2011-08-17 23:07:35]

Added SK_API to fix compile error on shared builds.

[Chris Guillory, 2011-08-17 23:08:01]

vector_platform_device_skia.h LGTM. Ship it.

[Vitaly Buka (NO REVIEWS), 2012-10-09 06:05:24]

http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constan...
File printing/print_job_constants.cc (right):

http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constan...
printing/print_job_constants.cc:40: const char kSettingHeaderFooterFontName[] =
"Helvetica";
I have question to author and reviewers.
Why do we need both kSettingHeaderFooterFontFamilyName and
kSettingHeaderFooterFontName

I see that kSettingHeaderFooterFontName is used only for ElideText. Why can't we
use just kSettingHeaderFooterFontFamilyName everywhere?

[vandebo (ex-Chrome), 2012-10-09 17:19:02]

On 2012/10/09 06:05:24, Vitaly Buka wrote:
>
http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constan...
> File printing/print_job_constants.cc (right):
> 
>
http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constan...
> printing/print_job_constants.cc:40: const char kSettingHeaderFooterFontName[]
=
> "Helvetica";
> I have question to author and reviewers.
> Why do we need both kSettingHeaderFooterFontFamilyName and
> kSettingHeaderFooterFontName
> 
> I see that kSettingHeaderFooterFontName is used only for ElideText. Why can't
we
> use just kSettingHeaderFooterFontFamilyName everywhere?

I can't think of a good reason.  Simply a review oversight.

[Aayush Kumar, 2012-10-10 20:12:27]

From what I remember, and it's been a while unfortunately, I think it was
on the mac that elide text would return "weirdly" elided text if I didn't
specify a specific font name (such as Helvetica).
And, if I specified the font name ('Helvetica') instead of the font family
name in PrintHeaderFooterText (of print_web_view_helper.cc), then it would
expect that font to be installed on the system and return an error if it
wasn't (which is often the case on Windows).

I was trying to verify if what I just said makes sense by cloning a
chromium repo.  I've been having some problems getting this to work on Mac
though.  Replacing font name for font family name on linux has no visible
affect though (this is what I sort of remember too).

On Tue, Oct 9, 2012 at 10:19 AM, <vandebo@chromium.org> wrote:

> On 2012/10/09 06:05:24, Vitaly Buka wrote:
>
> http://codereview.chromium.**org/7348010/diff/156002/**
>
printing/print_job_constants.**cc<http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constants.cc>
>
>> File printing/print_job_constants.**cc (right):
>>
>
>
> http://codereview.chromium.**org/7348010/diff/156002/**
>
printing/print_job_constants.**cc#newcode40<http://codereview.chromium.org/7348010/diff/156002/printing/print_job_constants.cc#newcode40>
>
>> printing/print_job_constants.**cc:40: const char
>> kSettingHeaderFooterFontName[]
>>
> =
>
>> "Helvetica";
>> I have question to author and reviewers.
>> Why do we need both kSettingHeaderFooterFontFamily**Name and
>> kSettingHeaderFooterFontName
>>
>
>  I see that kSettingHeaderFooterFontName is used only for ElideText. Why
>> can't
>>
> we
>
>> use just kSettingHeaderFooterFontFamily**Name everywhere?
>>
>
> I can't think of a good reason.  Simply a review oversight.
>
>
http://codereview.chromium.**org/7348010/<http://codereview.chromium.org/7348...
>