Bug 108507

Summary:

printing does not use minimum page zoom factor

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

WebKit2

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bdakin, cgarcia, commit-queue, darin, hyatt, mitz, mrobinson, simon.fraser, thorton

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

149368, 149595

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Try to fix CG build	darin: review+
Patch for landing	none
Patch for landing	none

Ojan Vafai

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2015-09-01 03:44:35 PDT

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

Carlos Garcia Campos

Comment 2 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.

Carlos Garcia Campos

Comment 3 2015-09-01 04:11:48 PDT

Created attachment 260360 [details] Patch

Carlos Garcia Campos

Comment 4 2015-09-01 04:21:14 PDT

Created attachment 260361 [details] Try to fix CG build

Carlos Garcia Campos

Comment 5 2015-09-16 01:56:53 PDT

Ping reviewers

Darin Adler

Comment 6 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.

Carlos Garcia Campos

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

Carlos Garcia Campos

Comment 8 2015-09-16 23:34:24 PDT

Created attachment 261373 [details] Patch for landing

Carlos Garcia Campos

Comment 9 2015-09-17 00:25:14 PDT

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

Alexey Proskuryakov

Comment 10 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.

WebKit Commit Bot

Comment 11 2015-09-18 21:45:08 PDT

Re-opened since this is blocked by bug 149368

Alexey Proskuryakov

Comment 12 2015-09-18 21:56:44 PDT

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

Carlos Garcia Campos

Comment 13 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.

Carlos Garcia Campos

Comment 14 2015-09-29 00:49:31 PDT

Created attachment 262050 [details] Patch for landing

Carlos Garcia Campos

Comment 15 2015-09-29 01:39:15 PDT

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

Note You need to log in before you can comment on or make changes to this bug.