Bug 108507 - printing does not use minimum page zoom factor
Summary: printing does not use minimum page zoom factor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 144494 (view as bug list)
Depends on: 149368 149595
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-31 11:10 PST by Ojan Vafai
Modified: 2015-09-29 01:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (21.23 KB, patch)
2015-09-01 04:11 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix CG build (21.25 KB, patch)
2015-09-01 04:21 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (25.57 KB, patch)
2015-09-16 23:34 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (19.73 KB, patch)
2015-09-29 00:49 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2013-01-31 11:10:13 PST
See the tests on https://bugs.webkit.org/show_bug.cgi?id=108382. They pass for mac-wk1 (I tested locally) and fail for mac-wk2. Not sure if it's just a WebKitTestRunner issue or if it's also an issue in Safari. Either way, I'll skip the tests in TestExpectations for now as it looks to be a bug and not intentional.

Looking at WebKit2/WebProcess/WebPage/WebPage.cpp, it definitely looks like the scale factor should be applying since we call m_printContext->begin(...).
Comment 1 Carlos Garcia Campos 2015-09-01 03:44:35 PDT
*** Bug 144494 has been marked as a duplicate of this bug. ***
Comment 2 Carlos Garcia Campos 2015-09-01 03:55:23 PDT
This is because WebKitTestRunner always takes the snapshots from the UI process (expect for IOS), so in the case of printing, the layout in the web view is not the expected one.
Comment 3 Carlos Garcia Campos 2015-09-01 04:11:48 PDT
Created attachment 260360 [details]
Patch
Comment 4 Carlos Garcia Campos 2015-09-01 04:21:14 PDT
Created attachment 260361 [details]
Try to fix CG build
Comment 5 Carlos Garcia Campos 2015-09-16 01:56:53 PDT
Ping reviewers
Comment 6 Darin Adler 2015-09-16 09:34:15 PDT
Comment on attachment 260361 [details]
Try to fix CG build

View in context: https://bugs.webkit.org/attachment.cgi?id=260361&action=review

Looks OK to me.

> Source/WebCore/page/PrintContext.cpp:320
> +int PrintContext::numberOfPages(Frame* frame, const FloatSize& pageSizeInPixels)

Should take a Frame&. The caller is the one that checks for null.

> Source/WebCore/page/PrintContext.cpp:329
> +void PrintContext::spoolAllPagesWithBoundaries(Frame* frame, GraphicsContext& graphicsContext, const FloatSize& pageSizeInPixels)

Should take a Frame&. The caller is the one that checks for null.

> Source/WebCore/page/PrintContext.cpp:347
> +#if PLATFORM(COCOA)
>      graphicsContext.translate(0, totalHeight);
>      graphicsContext.scale(FloatSize(1, -1));
> +#endif

Seems bizarre that this is here. The platform layer is responsible for abstracting differences like this.

> Source/WebCore/page/PrintContext.cpp:357
> +#if PLATFORM(COCOA)
> +            int boundaryLineY = currentHeight;
> +#else
> +            int boundaryLineY = currentHeight - 1;
> +#endif

Same comment about the platform layer.
Comment 7 Carlos Garcia Campos 2015-09-16 09:52:58 PDT
(In reply to comment #6)
> Comment on attachment 260361 [details]
> Try to fix CG build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260361&action=review
> 
> Looks OK to me.

Thanks for the review.

> > Source/WebCore/page/PrintContext.cpp:320
> > +int PrintContext::numberOfPages(Frame* frame, const FloatSize& pageSizeInPixels)
> 
> Should take a Frame&. The caller is the one that checks for null.

Will change that and the all the callers.

> > Source/WebCore/page/PrintContext.cpp:329
> > +void PrintContext::spoolAllPagesWithBoundaries(Frame* frame, GraphicsContext& graphicsContext, const FloatSize& pageSizeInPixels)
> 
> Should take a Frame&. The caller is the one that checks for null.

Same here.

> > Source/WebCore/page/PrintContext.cpp:347
> > +#if PLATFORM(COCOA)
> >      graphicsContext.translate(0, totalHeight);
> >      graphicsContext.scale(FloatSize(1, -1));
> > +#endif
> 
> Seems bizarre that this is here. The platform layer is responsible for
> abstracting differences like this.

I was surprised as well, I guess this is specific to the printing code in mac. 

> > Source/WebCore/page/PrintContext.cpp:357
> > +#if PLATFORM(COCOA)
> > +            int boundaryLineY = currentHeight;
> > +#else
> > +            int boundaryLineY = currentHeight - 1;
> > +#endif
> 
> Same comment about the platform layer.
Comment 8 Carlos Garcia Campos 2015-09-16 23:34:24 PDT
Created attachment 261373 [details]
Patch for landing
Comment 9 Carlos Garcia Campos 2015-09-17 00:25:14 PDT
Committed r189908: <http://trac.webkit.org/changeset/189908>
Comment 10 Alexey Proskuryakov 2015-09-18 21:42:16 PDT
This broke "run-webkit-tests --pixel" behavior, at least on Mac. How it is supposed to work is that --pixel has no effect on tests that call testRunner.dumpAsText() or testRunner.dumpAsText(false). But now, WebKitTestRunner dumps pixels for these, so all text-only tests fail when run in this mode.

This breaks an important testing scenario, so I'll roll out for now.
Comment 11 WebKit Commit Bot 2015-09-18 21:45:08 PDT
Re-opened since this is blocked by bug 149368
Comment 12 Alexey Proskuryakov 2015-09-18 21:56:44 PDT
If the fix is straightforward, please feel free to re-land without a new review round.
Comment 13 Carlos Garcia Campos 2015-09-28 06:33:58 PDT
(In reply to comment #10)
> This broke "run-webkit-tests --pixel" behavior, at least on Mac. How it is
> supposed to work is that --pixel has no effect on tests that call
> testRunner.dumpAsText() or testRunner.dumpAsText(false). But now,
> WebKitTestRunner dumps pixels for these, so all text-only tests fail when
> run in this mode.
> 
> This breaks an important testing scenario, so I'll roll out for now.

Oh, I'm sorry about this, I got confused with all the shouldDumpPixels in TestInvocation, TestRunner, InjectedBundle and InjectedBundlePage. Now that I understand it, I'm going to split the patch, first to avoid creating snapshots in the web process that will be ignored, and then the one to fix the printing snapshots.
Comment 14 Carlos Garcia Campos 2015-09-29 00:49:31 PDT
Created attachment 262050 [details]
Patch for landing
Comment 15 Carlos Garcia Campos 2015-09-29 01:39:15 PDT
Committed r190306: <http://trac.webkit.org/changeset/190306>