Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11359020:
Print headers and footers with WebKit. (Closed)
|
Created:
8 years, 1 month ago by Vitaly Buka (NO REVIEWS) Modified:
8 years, 1 month ago Reviewers:
vandebo (ex-Chrome), Lei Zhang, Toscano, tweakthis1, kmadhusu, msw, Alexei Svitkine (slow) CC:
chromium-reviews, msw, xji, Alexei Svitkine (slow), arthurhsu, vandebo (ex-Chrome) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrint 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 : #
Messages
Total messages: 40 (0 generated)
Please review. It's result of discussion at https://chromiumcodereview.appspot.com/11087016/ Robert, can you take a look at html file?
Nice. What kind of manual testing did you? Did you compare the resulting PDFs between the old and new path? I think kmadhusu@ had some test cases. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper_mac.mm (right): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... chrome/renderer/print_web_view_helper_mac.mm:172: #else You can delete the #else block here and remove the ifdef, since USE_SKIA is always true for Mac now.
On 2012/11/02 18:19:14, Alexei Svitkine wrote: > Nice. What kind of manual testing did you? Did you compare the resulting PDFs > between the old and new path? I think kmadhusu@ had some test cases. > 1. Checking/uncheking headers options to make sure main content in the same place. 2. Switching portrait/landscape mode. 3. Changing to custom margins and trying various values. I compared visually with origins PDF, pixel to pixel comparison here has no sense. Just to make sure that result is similar. > https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... > File chrome/renderer/print_web_view_helper_mac.mm (right): > > https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... > chrome/renderer/print_web_view_helper_mac.mm:172: #else > You can delete the #else block here and remove the ifdef, since USE_SKIA is > always true for Mac now. Makes sense, but Let me do that in separate CL?
just a couple bug update reminders https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:447: // crbug.com/108599. Please make sure to close/update this bug. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:532: // TODO(asvitkine): The below line is to workaround http://crbug.com/133548. Please make sure to close/update this bug.
Seems both should be closed if it's committed. Attaching to description. On 2012/11/02 19:09:50, msw wrote: > just a couple bug update reminders > > https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... > File chrome/renderer/print_web_view_helper.cc (left): > > https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... > chrome/renderer/print_web_view_helper.cc:447: // > crbug.com/108599. > Please make sure to close/update this bug. > > https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/renderer/pri... > chrome/renderer/print_web_view_helper.cc:532: // TODO(asvitkine): The below line > is to workaround http://crbug.com/133548. > Please make sure to close/update this bug.
https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... File chrome/browser/resources/print_preview/print_preview_page.html (right): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:2: <style> Can you wrap the <style> and <script> elements in a <head> element? https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:2: <style> Can you alpha sort all of the CSS properties within their respective section? https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:5: height: 50.0000px; Why all the extra 0s after the decimal? https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:57: <script> Java script should use single quotes, please be consistent. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:59: function $(o) {return document.getElementById(o);} Instead of using this, plz use document.querySelector. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:77: texts[i].style.fontSize = pixels(options["fontSize"]); Instead of iterating over all of the text elements, you can just add a rule to the stylesheet: document.styleSheets[0].insertRule('.text { font-size: ' + options['fontSize'] + 'px }'); You probably want to get rid of the existing static 'font-size' rule in above and dynamically add the default here. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:78: document["body"].style.height = pixels(options["height"]); I believe you can set offsetHeight and offsetWidth, and you won't need to add the 'px' suffix. Also below can be changed as well https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:86: getTextSpan("date").innerText = options["date"]; Please use querySelector instead of this method.
https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... File chrome/browser/resources/print_preview/print_preview_page.html (right): https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:2: <style> On 2012/11/02 19:30:42, Toscano wrote: > Can you wrap the <style> and <script> elements in a <head> element? Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:2: <style> On 2012/11/02 19:30:42, Toscano wrote: > Can you alpha sort all of the CSS properties within their respective section? Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:5: height: 50.0000px; On 2012/11/02 19:30:42, Toscano wrote: > Why all the extra 0s after the decimal? Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:57: <script> On 2012/11/02 19:30:42, Toscano wrote: > Java script should use single quotes, please be consistent. Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:59: function $(o) {return document.getElementById(o);} On 2012/11/02 19:30:42, Toscano wrote: > Instead of using this, plz use document.querySelector. Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:77: texts[i].style.fontSize = pixels(options["fontSize"]); On 2012/11/02 19:30:42, Toscano wrote: > Instead of iterating over all of the text elements, you can just add a rule to > the stylesheet: > > document.styleSheets[0].insertRule('.text { font-size: ' + options['fontSize'] + > 'px }'); > > You probably want to get rid of the existing static 'font-size' rule in above > and dynamically add the default here. Done. https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:78: document["body"].style.height = pixels(options["height"]); Does not work. On 2012/11/02 19:30:42, Toscano wrote: > I believe you can set offsetHeight and offsetWidth, and you won't need to add > the 'px' suffix. Also below can be changed as well https://chromiumcodereview.appspot.com/11359020/diff/1028/chrome/browser/reso... chrome/browser/resources/print_preview/print_preview_page.html:86: getTextSpan("date").innerText = options["date"]; On 2012/11/02 19:30:42, Toscano wrote: > Please use querySelector instead of this method. Done.
LGTM for the html
https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/pri... File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/pri... chrome/renderer/print_web_view_helper_linux.cc:199: page_size, canvas_area, scale_factor); If you do this, then you'll need to apply a clip to the content area before drawing it. Sometimes webkit draws outside of it's bounds because it knows that it'll get clipped. Also with this then there is no need for device->setDrawingArea(SkPDFDevice::kMargin_DrawingArea); because as far as the PDF device is concerned the entire page is the drawing area. Once you had a clip, you may have to do a save or save layer on the canvas in order to ensure it isn't obliterated by certain combinations of operations, but I don't remember well enough.
https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/pri... File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3004/chrome/renderer/pri... chrome/renderer/print_web_view_helper_linux.cc:199: page_size, canvas_area, scale_factor); On 2012/11/05 22:56:18, vandebo wrote: > If you do this, then you'll need to apply a clip to the content area before > drawing it. Sometimes webkit draws outside of it's bounds because it knows that > it'll get clipped. Also with this then there is no need for > device->setDrawingArea(SkPDFDevice::kMargin_DrawingArea); because as far as the > PDF device is concerned the entire page is the drawing area. > > Once you had a clip, you may have to do a save or save layer on the canvas in > order to ensure it isn't obliterated by certain combinations of operations, but > I don't remember well enough. Seems webkit clip content well, so I didn't add one. But setting one more clip is not going to hurt. Done.
LGTM https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:401: void PrintWebViewHelper::PrintHeaderAndFooter( nit: Should leave the // static comment. https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... File printing/print_settings_initializer.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... printing/print_settings_initializer.cc:50: date = ui::ElideText(date, font, segment_width, ui::ELIDE_AT_END); As I recall, ElideText didn't work in the renderer process, so that's why these strings are set here. With your change I'm not sure that the date, title, and url need to come from print settings initializer at all.
Thanks Steve. Lei, I still need at least your review. https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/8030/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:401: void PrintWebViewHelper::PrintHeaderAndFooter( On 2012/11/06 23:01:32, vandebo wrote: > nit: Should leave the // static comment. Done. https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... File printing/print_settings_initializer.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... printing/print_settings_initializer.cc:50: date = ui::ElideText(date, font, segment_width, ui::ELIDE_AT_END); I use CSS "text-overflow: ellipsis;" On 2012/11/06 23:01:32, vandebo wrote: > As I recall, ElideText didn't work in the renderer process, so that's why these > strings are set here. With your change I'm not sure that the date, title, and > url need to come from print settings initializer at all.
https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... File printing/print_settings_initializer.cc (left): https://chromiumcodereview.appspot.com/11359020/diff/8030/printing/print_sett... printing/print_settings_initializer.cc:50: date = ui::ElideText(date, font, segment_width, ui::ELIDE_AT_END); On 2012/11/06 23:16:02, Vitaly Buka wrote: > I use CSS "text-overflow: ellipsis;" > > On 2012/11/06 23:01:32, vandebo wrote: > > As I recall, ElideText didn't work in the renderer process, so that's why > these > > strings are set here. With your change I'm not sure that the date, title, and > > url need to come from print settings initializer at all. > Right, and as such, I think there's a bunch of machinery you can remove. This might show what's now unnecessary: http://codereview.chromium.org/7348010/#ps130001
lgtm, this CL's awesome. We can find the unused machinery and remove them in a later CL. https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:449: if (web_view) always true? https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.h (right): https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.h:295: static float RenderPageContent(WebKit::WebFrame* frame, Can you document this method? In particular, it's good to know |page_number| is zero-based.
https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.cc:449: if (web_view) Done. On 2012/11/06 23:55:13, Lei Zhang wrote: > always true? https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.h (right): https://chromiumcodereview.appspot.com/11359020/diff/5028/chrome/renderer/pri... chrome/renderer/print_web_view_helper.h:295: static float RenderPageContent(WebKit::WebFrame* frame, On 2012/11/06 23:55:13, Lei Zhang wrote: > Can you document this method? In particular, it's good to know |page_number| is > zero-based. Done.
Thanks Lei. Mike and Alexei, should I wait your reviews?
Not from me. Sorry RenderText didn't suffice, but I'm glad you found a nicer solution; thanks!
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, <msw@chromium.org> wrote: > Not from me. Sorry RenderText didn't suffice, but I'm glad you found a nicer > solution; thanks! > > https://chromiumcodereview.appspot.com/11359020/
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:msw@chromium.org> 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.
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:msw@chromium.org> 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.
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:msw@chromium.org> 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.
On 2012/11/09 17:55:22, Vitaly Buka wrote: > kmadhusu@ any updates? +kmadhusu
On 2012/11/09 19:37:54, Lei Zhang wrote: > On 2012/11/09 17:55:22, Vitaly Buka wrote: > > kmadhusu@ any updates? > > +kmadhusu Vitaly: Sorry, I didn't get your message. Will do the test now and let you know the results soon.
Vitaly: I did some manual testing and I found some bugs/regressions and they are as follows: (1) Printed output does not match preview data. Header and footer text are missing the printed output. Repro steps: Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/mix_lay... Destination: Any printer Margins: Default Header and footers option: Selected Pages: 1 (Print Page Number 1) Click Print button. Observed: Header and footer text are missing in the printed output. But they shown on the preview data. Expected: Header and footer text should be printed in the printer output. (2) Footer text has been changed. If you preview "http://en.wikipedia.org/wiki/Algorithm", we used to see "en.wikipedia.org/wiki/Algorithm" as footer text. But with this CL, I am seeing "http://en.wikipedia.org/wiki/Algorithm". I am not sure if this is the expected text. (3) Display header and footer text as per the custom margins value. Repro steps: Preview: en.wikipedia.org/wiki/Algorithm Select Custom Margins and set the top margin as "0". Observed: Both header and footer are not drawn. Expected: Header text should not be drawn and since the bottom margin is not zero, we should display the footer text. (4) Header and footer text positions and size changes with respect to margins. Repro steps: Preview: en.wikipedia.org/wiki/Algorithm Destination: Any printer Select Custom Margins and set {top: 2.13, right: 1.43, bottom, 0.72, left, 0.00} Notice the header (middle) text position. Change the right margin to 6.60. Observed: When I changed the right margin value, both header and footer text size and position changed. Expected: Margin values should not affect the header and footer size. (5) Missing footer text. Repro steps: Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/absolut... Destination: Any printer Margins: Default Header and footers option: Selected Observed: Footer text missing. Header text is so small and the middle text not centered. Expected: Header and footer should be drawn. (6) Footer text is drawn in the background and a partial text is shown in the preview. Repro steps: Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/absolut... Destination: Save as PDF Margins: Minimum Header and footers option: Selected Observed: If you scroll down to the bottom of first page, you can the footer text starting letter at the middle of the page (and it is drawn in the background). (You also see the problem in page 3). Expected: Footer shown should be fully shown at the bottom of the page. (7) Header and footer missing. Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/css_4_4... Destination: Save As PDF Margins: Default Header and footer option: Selected Observed: Header and footer text is not drawn. Expected: Header and footer should be displayed. (8) Header text is cropped. Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/dummy.html Destination: Save As PDF Margins: Default Header and footer option: Selected Observed: Notice Page 2, the header text is cropped and footer text is missing. Expected: Show full header and footer text. (9) Footer text is drawn at the paper edge. Page number is displayed on the middle of the page. Preview: http://www.corp.google.com/~kmadhusu/no_crawl/print_preview/test_html/dummy.html Destination: Any printer Margins: Default Header and footer option: Selected Observed: Page 3 footer text is drawn at the paper edge. Most printers, cannot print this footer text. You can also notice that the header and footer text size is different on every preview page. Notice that the page number is not in the right side corner of the page. Expected: Either don't display the footer text or draw it little above the paper edge. Please let me know if you need any details. On Fri, Nov 9, 2012 at 11:50 AM, <kmadhusu@chromium.org> wrote: > On 2012/11/09 19:37:54, Lei Zhang wrote: > >> On 2012/11/09 17:55:22, Vitaly Buka wrote: >> > kmadhusu@ any updates? >> > > +kmadhusu >> > > Vitaly: Sorry, I didn't get your message. Will do the test now and let you > know > the results soon. > > https://chromiumcodereview.**appspot.com/11359020/<https://chromiumcodereview... >
what kind of OS have you tested? On 2012/11/09 21:52:57, kmadhusu wrote: > Vitaly: I did some manual testing and I found some bugs/regressions > and they are as follows: > > (1) Printed output does not match preview data. Header and footer text > are missing the printed output. > Repro steps: > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/mix_l... > Destination: Any printer > Margins: Default > Header and footers option: Selected > Pages: 1 (Print Page Number 1) > Click Print button. > > Observed: Header and footer text are missing in the printed output. > But they shown on the preview data. > Expected: Header and footer text should be printed in the printer output. > > (2) Footer text has been changed. If you preview > "http://en.wikipedia.org/wiki/Algorithm", we used to see > "en.wikipedia.org/wiki/Algorithm" as footer text. But with this CL, I > am seeing "http://en.wikipedia.org/wiki/Algorithm". I am not sure if > this is the expected text. > > (3) Display header and footer text as per the custom margins value. > Repro steps: > Preview: en.wikipedia.org/wiki/Algorithm > Select Custom Margins and set the top margin as "0". > > Observed: Both header and footer are not drawn. > Expected: Header text should not be drawn and since the bottom margin > is not zero, we should display the footer text. > > (4) Header and footer text positions and size changes with respect to margins. > Repro steps: > Preview: en.wikipedia.org/wiki/Algorithm > Destination: Any printer > Select Custom Margins and set {top: 2.13, right: 1.43, bottom, 0.72, left, > 0.00} > Notice the header (middle) text position. > Change the right margin to 6.60. > > Observed: When I changed the right margin value, both header and > footer text size and position changed. > Expected: Margin values should not affect the header and footer size. > > (5) Missing footer text. > Repro steps: > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/absol... > Destination: Any printer > Margins: Default > Header and footers option: Selected > > Observed: Footer text missing. Header text is so small and the middle > text not centered. > Expected: Header and footer should be drawn. > > (6) Footer text is drawn in the background and a partial text is shown > in the preview. > Repro steps: > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/absol... > Destination: Save as PDF > Margins: Minimum > Header and footers option: Selected > > Observed: If you scroll down to the bottom of first page, you can the > footer text starting letter at the middle of the page (and it is drawn > in the background). (You also see the problem in page 3). > Expected: Footer shown should be fully shown at the bottom of the page. > > (7) Header and footer missing. > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/css_4... > Destination: Save As PDF > Margins: Default > Header and footer option: Selected > > Observed: Header and footer text is not drawn. > Expected: Header and footer should be displayed. > > (8) Header text is cropped. > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/dummy... > Destination: Save As PDF > Margins: Default > Header and footer option: Selected > > Observed: Notice Page 2, the header text is cropped and footer text is missing. > Expected: Show full header and footer text. > > (9) Footer text is drawn at the paper edge. Page number is displayed > on the middle of the page. > Preview: > http://www.corp.google.com/%7Ekmadhusu/no_crawl/print_preview/test_html/dummy... > Destination: Any printer > Margins: Default > Header and footer option: Selected > > Observed: Page 3 footer text is drawn at the paper edge. Most > printers, cannot print this footer text. You can also notice that the > header and footer text size is different on every preview page. Notice > that the page number is not in the right side corner of the page. > Expected: Either don't display the footer text or draw it little above > the paper edge. > > Please let me know if you need any details. > > > On Fri, Nov 9, 2012 at 11:50 AM, <mailto:kmadhusu@chromium.org> wrote: > > > On 2012/11/09 19:37:54, Lei Zhang wrote: > > > >> On 2012/11/09 17:55:22, Vitaly Buka wrote: > >> > kmadhusu@ any updates? > >> > > > > +kmadhusu > >> > > > > Vitaly: Sorry, I didn't get your message. Will do the test now and let you > > know > > the results soon. > > > > > https://chromiumcodereview.**appspot.com/11359020/%3Chttps://chromiumcoderevi...> > >
On 2012/11/09 22:46:24, Vitaly Buka wrote: > what kind of OS have you tested? > Linux > On 2012/11/09 21:52:57, kmadhusu wrote: > > Vitaly: I did some manual testing and I found some bugs/regressions > > and they are as follows: > > > > (1) Printed output does not match preview data. Header and footer text > > are missing the printed output. > > Repro steps: > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/mix... > > Destination: Any printer > > Margins: Default > > Header and footers option: Selected > > Pages: 1 (Print Page Number 1) > > Click Print button. > > > > Observed: Header and footer text are missing in the printed output. > > But they shown on the preview data. > > Expected: Header and footer text should be printed in the printer output. > > > > (2) Footer text has been changed. If you preview > > "http://en.wikipedia.org/wiki/Algorithm", we used to see > > "en.wikipedia.org/wiki/Algorithm" as footer text. But with this CL, I > > am seeing "http://en.wikipedia.org/wiki/Algorithm". I am not sure if > > this is the expected text. > > > > (3) Display header and footer text as per the custom margins value. > > Repro steps: > > Preview: en.wikipedia.org/wiki/Algorithm > > Select Custom Margins and set the top margin as "0". > > > > Observed: Both header and footer are not drawn. > > Expected: Header text should not be drawn and since the bottom margin > > is not zero, we should display the footer text. > > > > (4) Header and footer text positions and size changes with respect to margins. > > Repro steps: > > Preview: en.wikipedia.org/wiki/Algorithm > > Destination: Any printer > > Select Custom Margins and set {top: 2.13, right: 1.43, bottom, 0.72, left, > > 0.00} > > Notice the header (middle) text position. > > Change the right margin to 6.60. > > > > Observed: When I changed the right margin value, both header and > > footer text size and position changed. > > Expected: Margin values should not affect the header and footer size. > > > > (5) Missing footer text. > > Repro steps: > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/abs... > > Destination: Any printer > > Margins: Default > > Header and footers option: Selected > > > > Observed: Footer text missing. Header text is so small and the middle > > text not centered. > > Expected: Header and footer should be drawn. > > > > (6) Footer text is drawn in the background and a partial text is shown > > in the preview. > > Repro steps: > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/abs... > > Destination: Save as PDF > > Margins: Minimum > > Header and footers option: Selected > > > > Observed: If you scroll down to the bottom of first page, you can the > > footer text starting letter at the middle of the page (and it is drawn > > in the background). (You also see the problem in page 3). > > Expected: Footer shown should be fully shown at the bottom of the page. > > > > (7) Header and footer missing. > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/css... > > Destination: Save As PDF > > Margins: Default > > Header and footer option: Selected > > > > Observed: Header and footer text is not drawn. > > Expected: Header and footer should be displayed. > > > > (8) Header text is cropped. > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/dum... > > Destination: Save As PDF > > Margins: Default > > Header and footer option: Selected > > > > Observed: Notice Page 2, the header text is cropped and footer text is > missing. > > Expected: Show full header and footer text. > > > > (9) Footer text is drawn at the paper edge. Page number is displayed > > on the middle of the page. > > Preview: > > > http://www.corp.google.com/%257Ekmadhusu/no_crawl/print_preview/test_html/dum... > > Destination: Any printer > > Margins: Default > > Header and footer option: Selected > > > > Observed: Page 3 footer text is drawn at the paper edge. Most > > printers, cannot print this footer text. You can also notice that the > > header and footer text size is different on every preview page. Notice > > that the page number is not in the right side corner of the page. > > Expected: Either don't display the footer text or draw it little above > > the paper edge. > > > > Please let me know if you need any details. > > > > > > On Fri, Nov 9, 2012 at 11:50 AM, <mailto:kmadhusu@chromium.org> wrote: > > > > > On 2012/11/09 19:37:54, Lei Zhang wrote: > > > > > >> On 2012/11/09 17:55:22, Vitaly Buka wrote: > > >> > kmadhusu@ any updates? > > >> > > > > > > +kmadhusu > > >> > > > > > > Vitaly: Sorry, I didn't get your message. Will do the test now and let you > > > know > > > the results soon. > > > > > > > > > https://chromiumcodereview.**appspot.com/11359020/%253Chttps://chromiumcodere...> > > >
Thanks for feedback Kausalya, I'll take a look into these cases.
I have made some improvements and all cases not are working as expected. I tried Win, Linux and Mac On 2012/11/09 23:01:10, Vitaly Buka wrote: > Thanks for feedback Kausalya, I'll take a look into these cases.
On Sat, Nov 10, 2012 at 5:25 AM, <vitalybuka@chromium.org> wrote: > I have made some improvements and all cases not are working as expected. Did you mean "are *now* working as expected"? > I tried Win, Linux and Mac > > > On 2012/11/09 23:01:10, Vitaly Buka wrote: >> >> Thanks for feedback Kausalya, I'll take a look into these cases. > > > > > https://chromiumcodereview.appspot.com/11359020/
https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... 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/pri... File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... chrome/renderer/print_web_view_helper_linux.cc:215: scale_factor/1.25, Nit: spaces around the /. https://chromiumcodereview.appspot.com/11359020/diff/3022/printing/print_sett... File printing/print_settings_initializer.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/printing/print_sett... printing/print_settings_initializer.cc:46: print_settings->url = ui::ElideUrl(GURL(url), gfx::Font(), -1, std::string()); Please pass 0 instead of -1, since the behaviour when you pass 0 to ELideUrl() is documented, while -1 isn't (even though it's the same).
yes. On 2012/11/10 16:47:28, Alexei Svitkine wrote: > On Sat, Nov 10, 2012 at 5:25 AM, <mailto:vitalybuka@chromium.org> wrote: > > I have made some improvements and all cases not are working as expected. > > Did you mean "are *now* working as expected"? > > > I tried Win, Linux and Mac > > > > > > On 2012/11/09 23:01:10, Vitaly Buka wrote: > >> > >> Thanks for feedback Kausalya, I'll take a look into these cases. > > > > > > > > > > https://chromiumcodereview.appspot.com/11359020/
https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... File chrome/renderer/print_web_view_helper.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... 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: > Nit: spaces around the /'s. Done. https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... File chrome/renderer/print_web_view_helper_linux.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/chrome/renderer/pri... chrome/renderer/print_web_view_helper_linux.cc:215: scale_factor/1.25, On 2012/11/10 16:52:13, Alexei Svitkine wrote: > Nit: spaces around the /. Done. https://chromiumcodereview.appspot.com/11359020/diff/3022/printing/print_sett... File printing/print_settings_initializer.cc (right): https://chromiumcodereview.appspot.com/11359020/diff/3022/printing/print_sett... printing/print_settings_initializer.cc:46: print_settings->url = ui::ElideUrl(GURL(url), gfx::Font(), -1, std::string()); On 2012/11/10 16:52:13, Alexei Svitkine wrote: > Please pass 0 instead of -1, since the behaviour when you pass 0 to ELideUrl() > is documented, while -1 isn't (even though it's the same). Done.
LGTM
Vitaly@: Tested with the new patch and all cases works as expected. LGTM. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/11359020/5047
Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/11359020/5047
Change committed as 167311
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, 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/ >
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/ > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
