Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 7348010: Added Header and Footer support using Skia (Closed)

Created:
8 years ago by Aayush Kumar
Modified:
6 years, 9 months ago
CC:
chromium-reviews, arv (Not doing code reviews), darin-cc_chromium.org, brettw-cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Added Headers and Footers support - New Snapshot Uploaded #

Total comments: 42

Patch Set 3 : Added support for windows and moved most of the header footer printing code to print_web_view_helper #

Patch Set 4 : Added comment for PrintHeadersAndFooters #

Patch Set 5 : For some reason the changes I made in windows did not reflect in the previous patch. Updated now. #

Patch Set 6 : Indent Fix #

Total comments: 57

Patch Set 7 : Changes as per comments #

Patch Set 8 : Fixed style issues. Added check to see if there is enough space to print headers and footers. #

Total comments: 16

Patch Set 9 : Custom length strings being printed #

Total comments: 46

Patch Set 10 : Restored changes in elide text. Added mac code. #

Total comments: 8

Patch Set 11 : style changes as per demetrios comments #

Total comments: 12

Patch Set 12 : Separate CL for struct change. Changes as per review comments. #

Total comments: 16

Patch Set 13 : Removed header_footer_info param from IPC call. #

Total comments: 64

Patch Set 14 : Updated as per Chris' comments. #

Total comments: 22

Patch Set 15 : Changes as per Steve's comments. Use of #if define(USE_SKIA) and hidden UI for mac. #

Patch Set 16 : Freeing header_footer_info_ memory #

Total comments: 12

Patch Set 17 : changed header_footer_info to scoped_ptr and some style changes. #

Total comments: 28

Patch Set 18 : Rebased and moved PrintHeaderAndFooter to private member function. #

Total comments: 26

Patch Set 19 : Style changes as per Kausalya and Demetrios' comments #

Total comments: 46

Patch Set 20 : Remove header_footer_info from settings Dictionary. Changes as per Steve and Kausalya's comments. #

Total comments: 2

Patch Set 21 : Rebased Code #

Patch Set 22 : Initialized vars #

Total comments: 36

Patch Set 23 : Now we print either headers or footers (if we have space to print any). #

Total comments: 25

Patch Set 24 : New snapshot based on Steve and Kausalya's comments #

Total comments: 2

Patch Set 25 : Removed js test. Will add js tests in separate CL. #

Patch Set 26 : Removed use of webkit_scale_factor in print_web_view_helper_mac #

Total comments: 2

Patch Set 27 : Fixed ordering of function parameters #

Patch Set 28 : Moved ElideText to browser process #

Total comments: 6

Patch Set 29 : Moved elide text to print_settings_initializer #

Total comments: 13

Patch Set 30 : Changes based on Kausalya's comments #

Total comments: 10

Patch Set 31 : Moved code to printing_context.cc #

Patch Set 32 : Refactored JS code #

Total comments: 19

Patch Set 33 : New snapshot based on Steve, Kausalya and James' comments. #

Total comments: 2

Patch Set 34 : Added include #

Patch Set 35 : Fixed failing tests. #

Patch Set 36 : Added Destructor #

Total comments: 14

Patch Set 37 : New snapshot based on Kausalya and Steve's comments #

Total comments: 2

Patch Set 38 : Reverted change as per Kausalya's comment. #

Patch Set 39 : Added SK_API to vector platform device skia #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+738 lines, -20 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/printing/printing_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/header_footer_settings.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/header_footer_settings.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +68 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.css View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/resources/print_preview/print_preview.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 4 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/print_preview_data_source.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/print_preview_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 2 chunks +20 lines, -0 lines 0 comments Download
M chrome/common/print_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/renderer/mock_printer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 2 chunks +58 lines, -0 lines 0 comments Download
M chrome/renderer/mock_printer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 8 chunks +88 lines, -4 lines 0 comments Download
M chrome/renderer/print_web_view_helper.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 3 chunks +22 lines, -0 lines 0 comments Download
M chrome/renderer/print_web_view_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 5 chunks +165 lines, -1 line 0 comments Download
M chrome/renderer/print_web_view_helper_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/print_web_view_helper_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +7 lines, -1 line 0 comments Download
M chrome/renderer/print_web_view_helper_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/renderer/print_web_view_helper_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +9 lines, -0 lines 0 comments Download
M printing/print_job_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +22 lines, -0 lines 0 comments Download
M printing/print_job_constants.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +31 lines, -0 lines 1 comment Download
M printing/print_settings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +10 lines, -0 lines 0 comments Download
A printing/print_settings_initializer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +34 lines, -0 lines 0 comments Download
A printing/print_settings_initializer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +72 lines, -0 lines 0 comments Download
M printing/printing.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -0 lines 0 comments Download
M printing/printing_context.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +10 lines, -4 lines 0 comments Download
M printing/printing_context.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 2 chunks +10 lines, -0 lines 0 comments Download
M printing/printing_context_cairo.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +3 lines, -2 lines 0 comments Download
M printing/printing_context_cairo.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download
M printing/printing_context_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +3 lines, -2 lines 0 comments Download
M printing/printing_context_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download
M printing/printing_context_win.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +3 lines, -2 lines 0 comments Download
M printing/printing_context_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +1 line, -1 line 0 comments Download
M printing/units.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +7 lines, -0 lines 0 comments Download
M printing/units.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 2 chunks +15 lines, -0 lines 0 comments Download
M skia/ext/vector_platform_device_skia.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +5 lines, -1 line 0 comments Download
M skia/ext/vector_platform_device_skia.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 86 (0 generated)
Aayush Kumar
I have uploaded this CL just to get some feedback and make sure that I ...
8 years ago (2011-07-12 20:35:57 UTC) #1
Aayush Kumar
Added Lei to the reviewer's list :)
8 years ago (2011-07-12 20:52:34 UTC) #2
Lei Zhang
http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/print_preview/print_preview.js File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7348010/diff/1020/chrome/browser/resources/print_preview/print_preview.js#newcode339 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/print_preview_handler.cc File ...
8 years ago (2011-07-12 22:04:49 UTC) #3
kmadhusu
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/print_preview/print_preview.html File chrome/browser/resources/print_preview/print_preview.html ...
8 years ago (2011-07-13 01:49:07 UTC) #4
kmadhusu
http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/1020/chrome/browser/ui/webui/print_preview_handler.cc#newcode453 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_view_helper.h File chrome/renderer/print_web_view_helper.h (right): ...
8 years ago (2011-07-13 18:47:52 UTC) #5
Aayush Kumar
In this update, I have added headers and footers support for windows. (This may still ...
8 years ago (2011-07-13 21:52:16 UTC) #6
kmadhusu
http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html File chrome/browser/resources/print_preview/print_preview.html (right): http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html#newcode70 chrome/browser/resources/print_preview/print_preview.html:70: <div class="two-column option visible"> dpapad is refactoring the UI ...
8 years ago (2011-07-14 01:50:15 UTC) #7
dpapad
http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html File chrome/browser/resources/print_preview/print_preview.html (right): http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html#newcode70 chrome/browser/resources/print_preview/print_preview.html:70: <div class="two-column option visible"> Are we planning to add ...
8 years ago (2011-07-14 15:59:35 UTC) #8
dpapad
Also I noticed that some of the js/html files are outdated. Could you rebase please, ...
8 years ago (2011-07-14 16:27:04 UTC) #9
Aayush Kumar
http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html File chrome/browser/resources/print_preview/print_preview.html (right): http://codereview.chromium.org/7348010/diff/14002/chrome/browser/resources/print_preview/print_preview.html#newcode70 chrome/browser/resources/print_preview/print_preview.html:70: <div class="two-column option visible"> Based on our conversation, we'll ...
7 years, 12 months ago (2011-07-19 01:20:30 UTC) #10
dpapad
2 questions: 1) How do we determine the max displayed length of text in the ...
7 years, 12 months ago (2011-07-19 15:38:04 UTC) #11
Aayush Kumar
(i) I was working on customizing the lengths of the strings. This has been uploaded ...
7 years, 12 months ago (2011-07-19 18:37:19 UTC) #12
dpapad
http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/27001/chrome/browser/ui/webui/print_preview_handler.cc#newcode487 chrome/browser/ui/webui/print_preview_handler.cc:487: bool displayHeaderFooter; Rename to display_header_footer, see styleguide for naming ...
7 years, 12 months ago (2011-07-19 20:34:01 UTC) #13
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc#newcode497 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/print_preview_handler.cc#newcode500 ...
7 years, 12 months ago (2011-07-19 21:31:20 UTC) #14
Aayush Kumar
There are still a few UI kinks that are appearing in Windows and Mac. One ...
7 years, 12 months ago (2011-07-21 21:58:53 UTC) #15
dpapad
http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc#newcode51 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_view_helper.cc#newcode123 chrome/renderer/print_web_view_helper.cc:123: // Given ...
7 years, 12 months ago (2011-07-21 23:18:51 UTC) #16
Aayush Kumar
http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc#newcode51 chrome/renderer/print_web_view_helper.cc:51: using printing::ConvertPointsToPixelDouble; Am I missing something? Isn't it already? ...
7 years, 12 months ago (2011-07-21 23:30:47 UTC) #17
dpapad
http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/40001/chrome/renderer/print_web_view_helper.cc#newcode51 chrome/renderer/print_web_view_helper.cc:51: using printing::ConvertPointsToPixelDouble; On 2011/07/21 23:30:47, Aayush Kumar wrote: > ...
7 years, 12 months ago (2011-07-21 23:47:49 UTC) #18
vandebo (ex-Chrome)
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/print_preview_handler.cc ...
7 years, 12 months ago (2011-07-22 22:58:33 UTC) #19
Aayush Kumar
Rebased code as of Friday (july 22) night. http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc#newcode497 chrome/browser/ui/webui/print_preview_handler.cc:497: header_footer_info.SetString(printing::kSettingHeaderFooterTitle, ...
7 years, 11 months ago (2011-07-24 02:09:00 UTC) #20
vandebo (ex-Chrome)
> http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc > File chrome/browser/ui/webui/print_preview_handler.cc (right): > > http://codereview.chromium.org/7348010/diff/21001/chrome/browser/ui/webui/print_preview_handler.cc#newcode497 > chrome/browser/ui/webui/print_preview_handler.cc:497: > header_footer_info.SetString(printing::kSettingHeaderFooterTitle, > Sorry ...
7 years, 11 months ago (2011-07-25 01:06:59 UTC) #21
dpapad
http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/print_preview/print_preview.js File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/print_preview/print_preview.js#newcode809 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/print_preview/print_preview.js#newcode937 ...
7 years, 11 months ago (2011-07-25 15:40:01 UTC) #22
Aayush Kumar
Removed the second parameter from the IPC call as suggested by Steve :) http://codereview.chromium.org/7348010/diff/50002/chrome/browser/resources/print_preview/print_preview.js File ...
7 years, 11 months ago (2011-07-25 18:37:22 UTC) #23
dpapad
http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/print_preview_handler.cc#newcode505 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 ...
7 years, 11 months ago (2011-07-25 20:14:20 UTC) #24
Aayush Kumar
http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/50002/chrome/browser/ui/webui/print_preview_handler.cc#newcode505 chrome/browser/ui/webui/print_preview_handler.cc:505: header_footer_info.SetString(printing::kSettingHeaderFooterURL, It turns out that the url can be ...
7 years, 11 months ago (2011-07-25 22:31:15 UTC) #25
Chris Guillory
http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/58001/chrome/browser/ui/webui/print_preview_handler.cc#newcode487 chrome/browser/ui/webui/print_preview_handler.cc:487: bool display_header_footer; This local isn't used. Do you just ...
7 years, 11 months ago (2011-07-26 00:10:44 UTC) #26
Aayush Kumar
A TODO for me - I'll be adding #if define(USE_SKIA) conditionals in print_web_view_helper.cc once I ...
7 years, 11 months ago (2011-07-26 01:55:41 UTC) #27
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/50004/chrome/renderer/print_web_view_helper.cc#newcode112 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_view_helper.cc#newcode142 chrome/renderer/print_web_view_helper.cc:142: { Don't use ...
7 years, 11 months ago (2011-07-26 06:11:37 UTC) #28
Aayush Kumar
Added #if defined(USE_SKIA) around printing of header and footers. UI for header and footer checkbox ...
7 years, 11 months ago (2011-07-26 07:37:33 UTC) #29
Aayush Kumar
Just uploaded a patch that frees memory for header_footer_info_ var.
7 years, 11 months ago (2011-07-26 07:59:21 UTC) #30
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_view_helper.cc#newcode118 chrome/renderer/print_web_view_helper.cc:118: // Interstice is left at both ends of the ...
7 years, 11 months ago (2011-07-26 08:08:54 UTC) #31
Aayush Kumar
http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/65020/chrome/renderer/print_web_view_helper.cc#newcode118 chrome/renderer/print_web_view_helper.cc:118: // Interstice is left at both ends of the ...
7 years, 11 months ago (2011-07-26 08:52:27 UTC) #32
kmadhusu
Please rebase your CL. http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_view_helper.cc#newcode40 chrome/renderer/print_web_view_helper.cc:40: #include "ui/base/text/text_elider.h" can this header ...
7 years, 11 months ago (2011-07-26 20:27:50 UTC) #33
Aayush Kumar
http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/67028/chrome/renderer/print_web_view_helper.cc#newcode40 chrome/renderer/print_web_view_helper.cc:40: #include "ui/base/text/text_elider.h" This is not skia specific and it ...
7 years, 11 months ago (2011-07-28 17:28:41 UTC) #34
dpapad
LGTM for the UI with nits. http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.css File chrome/browser/resources/print_preview/print_preview.css (right): http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.css#newcode349 chrome/browser/resources/print_preview/print_preview.css:349: html[os=mac] hr[id='options-horizontal-separator'] { ...
7 years, 11 months ago (2011-07-28 20:26:00 UTC) #35
kmadhusu
http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.js File chrome/browser/resources/print_preview/print_preview.js (right): http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.js#newcode793 chrome/browser/resources/print_preview/print_preview.js:793: fadeOutElement($('options-option')); Can you add line #793 in the else ...
7 years, 11 months ago (2011-07-28 20:31:20 UTC) #36
Aayush Kumar
http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.css File chrome/browser/resources/print_preview/print_preview.css (right): http://codereview.chromium.org/7348010/diff/70020/chrome/browser/resources/print_preview/print_preview.css#newcode349 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: > ...
7 years, 11 months ago (2011-07-29 00:21:46 UTC) #37
kmadhusu
http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/print_preview_handler.cc#newcode494 chrome/browser/ui/webui/print_preview_handler.cc:494: settings->SetString(printing::kSettingHeaderFooterTitle, Set the header and footer info in |settings| ...
7 years, 11 months ago (2011-08-01 17:17:08 UTC) #38
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc#newcode49 chrome/renderer/print_web_view_helper.cc:49: #endif // USE_SKIA http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc#newcode127 chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width + I'm actually ...
7 years, 11 months ago (2011-08-01 21:07:37 UTC) #39
Aayush Kumar
http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/79001/chrome/browser/ui/webui/print_preview_handler.cc#newcode494 chrome/browser/ui/webui/print_preview_handler.cc:494: settings->SetString(printing::kSettingHeaderFooterTitle, On 2011/08/01 17:17:08, kmadhusu wrote: > Set the ...
7 years, 11 months ago (2011-08-02 00:18:13 UTC) #40
Aayush Kumar
Made some style changes to the latest patch :)
7 years, 11 months ago (2011-08-02 01:26:14 UTC) #41
Lei Zhang
http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/print_preview_handler.cc#newcode495 chrome/browser/ui/webui/print_preview_handler.cc:495: bool display_header_footer; If you don't initialize |display_header_footer|, you may ...
7 years, 11 months ago (2011-08-03 01:28:30 UTC) #42
Aayush Kumar
http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/print_preview_handler.cc File chrome/browser/ui/webui/print_preview_handler.cc (right): http://codereview.chromium.org/7348010/diff/88019/chrome/browser/ui/webui/print_preview_handler.cc#newcode495 chrome/browser/ui/webui/print_preview_handler.cc:495: bool display_header_footer; On 2011/08/03 01:28:31, Lei Zhang wrote: > ...
7 years, 11 months ago (2011-08-03 15:52:53 UTC) #43
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc#newcode215 chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in the ...
7 years, 11 months ago (2011-08-03 19:00:05 UTC) #44
Aayush Kumar
http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/79001/chrome/renderer/print_web_view_helper.cc#newcode215 chrome/renderer/print_web_view_helper.cc:215: // Set the drawing area to draw in the ...
7 years, 11 months ago (2011-08-04 18:25:04 UTC) #45
kmadhusu
http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_view_helper.cc#newcode127 chrome/renderer/print_web_view_helper.cc:127: page_layout.content_width + style nit: 4 space indentation is not ...
7 years, 11 months ago (2011-08-04 19:15:15 UTC) #46
kmadhusu
http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/102001/chrome/renderer/print_web_view_helper.cc#newcode1005 chrome/renderer/print_web_view_helper.cc:1005: settings.params.margin_top = 0; Why are you resetting the margin ...
7 years, 11 months ago (2011-08-04 19:49:17 UTC) #47
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/95001/chrome/renderer/print_web_view_helper.cc#newcode150 chrome/renderer/print_web_view_helper.cc:150: const SkScalar& space_for_descenders) { On 2011/08/04 18:25:04, Aayush Kumar ...
7 years, 11 months ago (2011-08-04 20:46:21 UTC) #48
Aayush Kumar
To Do: Add some more JS tests. But, that can be a separate CL as ...
7 years, 11 months ago (2011-08-04 23:00:30 UTC) #49
vandebo (ex-Chrome)
C++ LGTM
7 years, 11 months ago (2011-08-05 00:00:49 UTC) #50
dpapad
http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/print_preview.js File chrome/test/data/webui/print_preview.js (right): http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/print_preview.js#newcode292 chrome/test/data/webui/print_preview.js:292: checkSectionVisible($('options-option'), false); This test fails as it is. Why ...
7 years, 11 months ago (2011-08-05 00:34:59 UTC) #51
Aayush Kumar
http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/print_preview.js File chrome/test/data/webui/print_preview.js (right): http://codereview.chromium.org/7348010/diff/100007/chrome/test/data/webui/print_preview.js#newcode292 chrome/test/data/webui/print_preview.js:292: checkSectionVisible($('options-option'), false); On 2011/08/05 00:34:59, dpapad wrote: > This ...
7 years, 11 months ago (2011-08-05 00:52:39 UTC) #52
kmadhusu
C++ code LGTM
7 years, 11 months ago (2011-08-05 15:44:11 UTC) #53
Lei Zhang
http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_view_helper.cc#newcode140 chrome/renderer/print_web_view_helper.cc:140: void PrintHeaderFooterText( nit: PrintHeaderFooterText() and PrintHeaders() have many common ...
7 years, 11 months ago (2011-08-05 18:58:21 UTC) #54
Aayush Kumar
http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/108001/chrome/renderer/print_web_view_helper.cc#newcode140 chrome/renderer/print_web_view_helper.cc:140: void PrintHeaderFooterText( On 2011/08/05 18:58:21, Lei Zhang wrote: > ...
7 years, 11 months ago (2011-08-08 03:19:22 UTC) #55
Aayush Kumar
Moved ElideText call to browser process.
7 years, 11 months ago (2011-08-11 21:20:16 UTC) #56
vandebo (ex-Chrome)
A couple general comments. http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/printing_message_filter.cc File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7348010/diff/130001/chrome/browser/printing/printing_message_filter.cc#newcode320 chrome/browser/printing/printing_message_filter.cc:320: void UpdateHeaderFooterStrings(const base::DictionaryValue& header_footer_info, What ...
7 years, 11 months ago (2011-08-11 23:32:13 UTC) #57
Aayush Kumar
I haven't tested this on windows and mac (it's still building), so there might be ...
7 years, 11 months ago (2011-08-12 08:35:40 UTC) #58
kmadhusu
http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_view_helper.cc#newcode104 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_initializer.h File ...
7 years, 11 months ago (2011-08-12 17:07:00 UTC) #59
kmadhusu
http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/printing_message_filter.cc File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/printing_message_filter.cc#newcode60 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_initializer_gtk.h File ...
7 years, 11 months ago (2011-08-12 17:21:31 UTC) #60
Aayush Kumar
http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/printing_message_filter.cc File chrome/browser/printing/printing_message_filter.cc (right): http://codereview.chromium.org/7348010/diff/130010/chrome/browser/printing/printing_message_filter.cc#newcode60 chrome/browser/printing/printing_message_filter.cc:60: if (!settings.display_header_footer) On 2011/08/12 17:21:32, kmadhusu wrote: > You ...
7 years, 11 months ago (2011-08-12 18:53:42 UTC) #61
kmadhusu
http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/130010/chrome/renderer/print_web_view_helper.cc#newcode104 chrome/renderer/print_web_view_helper.cc:104: newParams.params.display_header_footer && On 2011/08/12 18:53:42, Aayush Kumar wrote: > ...
7 years, 11 months ago (2011-08-12 20:27:11 UTC) #62
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_view_helper.cc#newcode925 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_view_helper_mac.mm ...
7 years, 11 months ago (2011-08-12 22:18:20 UTC) #63
Aayush Kumar
Moved code around as per Steve's suggestions. Looks a lot better now :) http://codereview.chromium.org/7348010/diff/126086/chrome/renderer/print_web_view_helper.cc File ...
7 years, 11 months ago (2011-08-13 04:04:52 UTC) #64
Aayush Kumar
Refactored the JS code
7 years, 11 months ago (2011-08-13 05:47:32 UTC) #65
kmadhusu
http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_view_helper.cc#newcode100 chrome/renderer/print_web_view_helper.cc:100: newParams.params.display_header_footer && As I said earlier, you need to ...
7 years, 11 months ago (2011-08-15 16:46:53 UTC) #66
Aayush Kumar
+jhawkins as a reviewer for the JS refactoring
7 years, 11 months ago (2011-08-15 18:25:37 UTC) #67
vandebo (ex-Chrome)
Just nits - LGTM http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/7348010/diff/138004/chrome/renderer/print_web_view_helper.cc#newcode926 chrome/renderer/print_web_view_helper.cc:926: header_footer_info_.reset(new DictionaryValue()); Why do we ...
7 years, 11 months ago (2011-08-15 19:35:48 UTC) #68
James Hawkins
http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/print_preview/header_footer_settings.html File chrome/browser/resources/print_preview/header_footer_settings.html (right): http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/print_preview/header_footer_settings.html#newcode4 chrome/browser/resources/print_preview/header_footer_settings.html:4: <input id="header-footer" type="checkbox" checked/> Do not close single tags. ...
7 years, 11 months ago (2011-08-15 19:39:35 UTC) #69
Aayush Kumar
http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/print_preview/header_footer_settings.html File chrome/browser/resources/print_preview/header_footer_settings.html (right): http://codereview.chromium.org/7348010/diff/138004/chrome/browser/resources/print_preview/header_footer_settings.html#newcode4 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 ...
7 years, 11 months ago (2011-08-15 21:37:37 UTC) #70
James Hawkins
LGTM with include nit. http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_initializer.h File printing/print_settings_initializer.h (right): http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_initializer.h#newcode8 printing/print_settings_initializer.h:8: #include "base/logging.h" base/basictypes.h
7 years, 11 months ago (2011-08-15 21:41:24 UTC) #71
Aayush Kumar
http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_initializer.h File printing/print_settings_initializer.h (right): http://codereview.chromium.org/7348010/diff/138042/printing/print_settings_initializer.h#newcode8 printing/print_settings_initializer.h:8: #include "base/logging.h" On 2011/08/15 21:41:24, James Hawkins wrote: > ...
7 years, 11 months ago (2011-08-15 22:16:56 UTC) #72
kmadhusu
LGTM
7 years, 11 months ago (2011-08-15 22:19:50 UTC) #73
Aayush Kumar
Fixed failing tests.
7 years, 11 months ago (2011-08-16 06:24:43 UTC) #74
vandebo (ex-Chrome)
http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc File chrome/renderer/mock_printer.cc (right): http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc#newcode58 chrome/renderer/mock_printer.cc:58: assert(sizeof(PrintMsg_Print_Params_Clone) == sizeof(PrintMsg_Print_Params)); Use COMPILE_ASSERT so that the problem ...
7 years, 11 months ago (2011-08-16 17:47:46 UTC) #75
kmadhusu
http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc File chrome/renderer/mock_printer.cc (right): http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc#newcode70 chrome/renderer/mock_printer.cc:70: params->pages = pages_; On Release version, MSVS crashes the ...
7 years, 11 months ago (2011-08-16 18:08:34 UTC) #76
Aayush Kumar
http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc File chrome/renderer/mock_printer.cc (right): http://codereview.chromium.org/7348010/diff/147002/chrome/renderer/mock_printer.cc#newcode58 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: > ...
7 years, 11 months ago (2011-08-16 20:32:52 UTC) #77
vandebo (ex-Chrome)
try jobs = LG
7 years, 11 months ago (2011-08-16 20:35:31 UTC) #78
kmadhusu
http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_printer.cc File chrome/renderer/mock_printer.cc (right): http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_printer.cc#newcode73 chrome/renderer/mock_printer.cc:73: params->pages.clear(); Sorry for the false alarm.. It looks like ...
7 years, 11 months ago (2011-08-16 21:00:28 UTC) #79
Aayush Kumar
http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_printer.cc File chrome/renderer/mock_printer.cc (right): http://codereview.chromium.org/7348010/diff/153001/chrome/renderer/mock_printer.cc#newcode73 chrome/renderer/mock_printer.cc:73: params->pages.clear(); On 2011/08/16 21:00:28, kmadhusu wrote: > Sorry for ...
7 years, 11 months ago (2011-08-16 22:33:18 UTC) #80
kmadhusu
LGTM. Thanks.
7 years, 11 months ago (2011-08-16 22:39:04 UTC) #81
Aayush Kumar
Added SK_API to fix compile error on shared builds.
7 years, 11 months ago (2011-08-17 23:07:35 UTC) #82
Chris Guillory
vector_platform_device_skia.h LGTM. Ship it.
7 years, 11 months ago (2011-08-17 23:08:01 UTC) #83
Vitaly Buka (NO REVIEWS)
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 printing/print_job_constants.cc:40: const char kSettingHeaderFooterFontName[] = "Helvetica"; I have question to ...
6 years, 9 months ago (2012-10-09 06:05:24 UTC) #84
vandebo (ex-Chrome)
On 2012/10/09 06:05:24, Vitaly Buka wrote: > 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 ...
6 years, 9 months ago (2012-10-09 17:19:02 UTC) #85
Aayush Kumar
6 years, 9 months ago (2012-10-10 20:12:27 UTC) #86
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...
>

Powered by Google App Engine
This is Rietveld 408576698