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

Issue 11359020: Print headers and footers with WebKit. (Closed)

Created:
6 years, 7 months ago by Vitaly Buka (NO REVIEWS)
Modified:
6 years, 7 months ago
CC:
chromium-reviews, msw, xji, Alexei Svitkine (slow), arthurhsu, vandebo (ex-Chrome)
Visibility:
Public.

Description

Print headers and footers with WebKit. Old implementation with gfx::RenderText had issues with fallback fonts. Sandbox does not allow to read required information from registry. Also WebKit inplementation is smaller and more readable. BUG=152893, 108599, 133548 TEST=manual: make sure that main content with or without headers is in the same place (default margin is exception). Make sure that any custom margins and paper layout produce reasonable result. If margins are to small, header and footer should be hidden. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167311

Patch Set 1 : #

Total comments: 19

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Total comments: 5

Patch Set 4 : #

Total comments: 4

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : linux #

Total comments: 6

Patch Set 8 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+244 lines, -301 lines) Patch
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A chrome/browser/resources/print_preview/print_preview_page.html View 1 2 3 4 5 6 1 chunk +115 lines, -0 lines 0 comments Download
M chrome/renderer/print_web_view_helper.h View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download
M chrome/renderer/print_web_view_helper.cc View 1 2 3 4 5 6 7 3 chunks +83 lines, -229 lines 0 comments Download
M chrome/renderer/print_web_view_helper_linux.cc View 1 2 3 4 5 6 7 2 chunks +11 lines, -6 lines 0 comments Download
M chrome/renderer/print_web_view_helper_mac.mm View 1 2 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/renderer/print_web_view_helper_win.cc View 1 2 1 chunk +13 lines, -6 lines 0 comments Download
M printing/print_job_constants.h View 1 2 3 4 5 6 1 chunk +0 lines, -5 lines 0 comments Download
M printing/print_job_constants.cc View 1 2 3 4 5 6 1 chunk +0 lines, -16 lines 0 comments Download
M printing/print_settings_initializer.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -22 lines 0 comments Download
M printing/units.h View 1 chunk +0 lines, -4 lines 0 comments Download
M printing/units.cc View 1 chunk +0 lines, -9 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
Vitaly Buka (NO REVIEWS)
Please review. It's result of discussion at https://chromiumcodereview.appspot.com/11087016/ Robert, can you take a look at ...
6 years, 7 months ago (2012-11-02 18:12:14 UTC) #1
Alexei Svitkine (slow)
Nice. What kind of manual testing did you? Did you compare the resulting PDFs between ...
6 years, 7 months ago (2012-11-02 18:19:14 UTC) #2
Vitaly Buka (NO REVIEWS)
On 2012/11/02 18:19:14, Alexei Svitkine wrote: > Nice. What kind of manual testing did you? ...
6 years, 7 months ago (2012-11-02 18:32:59 UTC) #3
msw
just a couple bug update reminders https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/print_web_view_helper.cc#oldcode447 chrome/renderer/print_web_view_helper.cc:447: // crbug.com/108599. Please ...
6 years, 7 months ago (2012-11-02 19:09:50 UTC) #4
Vitaly Buka (NO REVIEWS)
Seems both should be closed if it's committed. Attaching to description. On 2012/11/02 19:09:50, msw ...
6 years, 7 months ago (2012-11-02 19:20:49 UTC) #5
Toscano
https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/resources/print_preview/print_preview_page.html File chrome/browser/resources/print_preview/print_preview_page.html (right): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/resources/print_preview/print_preview_page.html#newcode2 chrome/browser/resources/print_preview/print_preview_page.html:2: <style> Can you wrap the <style> and <script> elements ...
6 years, 7 months ago (2012-11-02 19:30:42 UTC) #6
Vitaly Buka (NO REVIEWS)
https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/resources/print_preview/print_preview_page.html File chrome/browser/resources/print_preview/print_preview_page.html (right): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/resources/print_preview/print_preview_page.html#newcode2 chrome/browser/resources/print_preview/print_preview_page.html:2: <style> On 2012/11/02 19:30:42, Toscano wrote: > Can you ...
6 years, 7 months ago (2012-11-02 21:28:14 UTC) #7
Toscano
LGTM for the html
6 years, 7 months ago (2012-11-02 21:43:40 UTC) #8
vandebo (ex-Chrome)
https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/print_web_view_helper_linux.cc File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/print_web_view_helper_linux.cc#newcode199 chrome/renderer/print_web_view_helper_linux.cc:199: page_size, canvas_area, scale_factor); If you do this, then you'll ...
6 years, 7 months ago (2012-11-05 22:56:18 UTC) #9
Vitaly Buka (NO REVIEWS)
https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/print_web_view_helper_linux.cc File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/print_web_view_helper_linux.cc#newcode199 chrome/renderer/print_web_view_helper_linux.cc:199: page_size, canvas_area, scale_factor); On 2012/11/05 22:56:18, vandebo wrote: > ...
6 years, 7 months ago (2012-11-06 00:40:14 UTC) #10
vandebo (ex-Chrome)
LGTM https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/print_web_view_helper.cc#newcode401 chrome/renderer/print_web_view_helper.cc:401: void PrintWebViewHelper::PrintHeaderAndFooter( nit: Should leave the // static ...
6 years, 7 months ago (2012-11-06 23:01:32 UTC) #11
Vitaly Buka (NO REVIEWS)
Thanks Steve. Lei, I still need at least your review. https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/print_web_view_helper.cc#newcode401 ...
6 years, 7 months ago (2012-11-06 23:16:02 UTC) #12
vandebo (ex-Chrome)
https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_settings_initializer.cc File printing/print_settings_initializer.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_settings_initializer.cc#oldcode50 printing/print_settings_initializer.cc:50: date = ui::ElideText(date, font, segment_width, ui::ELIDE_AT_END); On 2012/11/06 23:16:02, ...
6 years, 7 months ago (2012-11-06 23:23:44 UTC) #13
Lei Zhang
lgtm, this CL's awesome. We can find the unused machinery and remove them in a ...
6 years, 7 months ago (2012-11-06 23:55:13 UTC) #14
Vitaly Buka (NO REVIEWS)
https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/print_web_view_helper.cc#newcode449 chrome/renderer/print_web_view_helper.cc:449: if (web_view) Done. On 2012/11/06 23:55:13, Lei Zhang wrote: ...
6 years, 7 months ago (2012-11-07 01:20:51 UTC) #15
Vitaly Buka (NO REVIEWS)
Thanks Lei. Mike and Alexei, should I wait your reviews?
6 years, 7 months ago (2012-11-07 01:23:38 UTC) #16
msw
Not from me. Sorry RenderText didn't suffice, but I'm glad you found a nicer solution; ...
6 years, 7 months ago (2012-11-07 01:27:13 UTC) #17
Alexei Svitkine (slow)
No, although I'd like you to double check with kmadhusu@ on the (manual) test cases ...
6 years, 7 months ago (2012-11-07 02:18:14 UTC) #18
kmadhusu
On 2012/11/07 02:18:14, Alexei Svitkine wrote: > No, although I'd like you to double check ...
6 years, 7 months ago (2012-11-07 02:21:16 UTC) #19
Vitaly Buka (NO REVIEWS)
Done. On 2012/11/07 02:21:16, kmadhusu wrote: > On 2012/11/07 02:18:14, Alexei Svitkine wrote: > > ...
6 years, 7 months ago (2012-11-07 21:18:28 UTC) #20
Vitaly Buka (NO REVIEWS)
kmadhusu@ any updates? On 2012/11/07 21:18:28, Vitaly Buka wrote: > Done. > > On 2012/11/07 ...
6 years, 7 months ago (2012-11-09 17:55:22 UTC) #21
Lei Zhang
On 2012/11/09 17:55:22, Vitaly Buka wrote: > kmadhusu@ any updates? +kmadhusu
6 years, 7 months ago (2012-11-09 19:37:54 UTC) #22
kmadhusu
On 2012/11/09 19:37:54, Lei Zhang wrote: > On 2012/11/09 17:55:22, Vitaly Buka wrote: > > ...
6 years, 7 months ago (2012-11-09 19:50:02 UTC) #23
kmadhusu
Vitaly: I did some manual testing and I found some bugs/regressions and they are as ...
6 years, 7 months ago (2012-11-09 21:52:57 UTC) #24
Vitaly Buka (NO REVIEWS)
what kind of OS have you tested? On 2012/11/09 21:52:57, kmadhusu wrote: > Vitaly: I ...
6 years, 7 months ago (2012-11-09 22:46:24 UTC) #25
kmadhusu
On 2012/11/09 22:46:24, Vitaly Buka wrote: > what kind of OS have you tested? > ...
6 years, 7 months ago (2012-11-09 22:50:18 UTC) #26
Vitaly Buka (NO REVIEWS)
Thanks for feedback Kausalya, I'll take a look into these cases.
6 years, 7 months ago (2012-11-09 23:01:10 UTC) #27
Vitaly Buka (NO REVIEWS)
I have made some improvements and all cases not are working as expected. I tried ...
6 years, 7 months ago (2012-11-10 10:25:54 UTC) #28
Alexei Svitkine (slow)
On Sat, Nov 10, 2012 at 5:25 AM, <vitalybuka@chromium.org> wrote: > I have made some ...
6 years, 7 months ago (2012-11-10 16:47:28 UTC) #29
Alexei Svitkine (slow)
https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/print_web_view_helper.cc#newcode415 chrome/renderer/print_web_view_helper.cc:415: canvas->scale(1/webkit_scale_factor, 1/webkit_scale_factor); Nit: spaces around the /'s. https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/print_web_view_helper_linux.cc File ...
6 years, 7 months ago (2012-11-10 16:52:13 UTC) #30
Vitaly Buka (NO REVIEWS)
yes. On 2012/11/10 16:47:28, Alexei Svitkine wrote: > On Sat, Nov 10, 2012 at 5:25 ...
6 years, 7 months ago (2012-11-12 08:05:05 UTC) #31
Vitaly Buka (NO REVIEWS)
https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/print_web_view_helper.cc File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/print_web_view_helper.cc#newcode415 chrome/renderer/print_web_view_helper.cc:415: canvas->scale(1/webkit_scale_factor, 1/webkit_scale_factor); On 2012/11/10 16:52:13, Alexei Svitkine wrote: > ...
6 years, 7 months ago (2012-11-12 08:20:52 UTC) #32
Alexei Svitkine (slow)
LGTM
6 years, 7 months ago (2012-11-12 16:37:17 UTC) #33
kmadhusu
Vitaly@: Tested with the new patch and all cases works as expected. LGTM. Thanks.
6 years, 7 months ago (2012-11-12 18:58:51 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/11359020/5047
6 years, 7 months ago (2012-11-12 19:13:13 UTC) #35
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
6 years, 7 months ago (2012-11-13 03:52:31 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/11359020/5047
6 years, 7 months ago (2012-11-13 06:09:31 UTC) #37
commit-bot: I haz the power
Change committed as 167311
6 years, 7 months ago (2012-11-13 06:21:56 UTC) #38
tweakthis1_gmail.com
I am having trouble with the header and footer font size when trying to print. ...
6 years, 7 months ago (2012-11-15 17:47:55 UTC) #39
Vitaly Buka (NO REVIEWS)
6 years, 7 months ago (2012-11-15 18:56:04 UTC) #40
tweak@ It's possible. Please file bug at http://crbug.com with full details
about how to reproduce the bug. Attach screenshots or scans if possible.

On 2012/11/15 17:47:55, tweakthis1_gmail.com wrote:
> I am having trouble with the header and footer font size when trying to 
> print. is there any way to make them bigger with one of these patches?
> On Friday, November 9, 2012 12:55:24 PM UTC-5, mailto:vital...@chromium.org
wrote: 
> >
> > kmadhusu@ any updates? 
> >
> > On 2012/11/07 21:18:28, Vitaly Buka wrote: 
> > > Done. 
> >
> > > On 2012/11/07 02:21:16, kmadhusu wrote: 
> > > > On 2012/11/07 02:18:14, Alexei Svitkine wrote: 
> > > > > No, although I'd like you to double check with kmadhusu@ on the 
> > > > > (manual) test cases she has for print headers, to make sure we don't 
> > > > > regress anything. 
> > > > > 
> > > > > On Tue, Nov 6, 2012 at 8:27 PM, 
<mailto:m...@chromium.org<javascript:>>
> 
> > wrote: 
> > > > > > Not from me. Sorry RenderText didn't suffice, but I'm glad you   
> > > found a 
> > > nicer 
> > > > > > solution; thanks! 
> > > > > > 
> > > > > > https://chromiumcodereview.appspot.com/11359020/ 
> > > > 
> > > > Vitaly: I tried to test your changes but I saw several conflicts. 
> > Please 
> > > rebase 
> > > > your changes. I will be happy to do some tests. 
> >
> >
> >
> > https://chromiumcodereview.appspot.com/11359020/ 
> >

Powered by Google App Engine
This is Rietveld 408576698