Bug 108507 – printing does not use minimum page zoom factor

• Component: WebKit2
• Status: RESOLVED FIXED

[Ojan Vafai]

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(...).

[Carlos Garcia Campos]

*** Bug 144494 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 260360 [details]
Patch

[Carlos Garcia Campos]

Created attachment 260361 [details]
Try to fix CG build

[Carlos Garcia Campos]

Ping reviewers

[Darin Adler]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 261373 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r189908: <http://trac.webkit.org/changeset/189908>

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149368

[Alexey Proskuryakov]

If the fix is straightforward, please feel free to re-land without a new review round.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 262050 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r190306: <http://trac.webkit.org/changeset/190306>