Bug 57782 – Replace SKIA_TEXT with isNativeFontRenderingAllowed() for print-preview to work

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

Replace SKIA_TEXT with isNativeFontRenderingAllowed() for print-preview to work

[Mike Reed]

Created attachment 88112 [details]
Patch

[Kenneth Russell]

Comment on attachment 88112 [details]
Patch

Looks fine to me as long as it's been tested.

[Kenneth Russell]

Also, did you want a cq+? Please mark bugs with cq? if so. ('webkit-patch upload' does this automatically.)

[Chris Guillory]

Shouldn't this change already be landed from this change:
http://trac.webkit.org/changeset/82532/trunk/Source/WebCore/platform/graphics/skia/SkiaFontWin.cpp

Am I missing something?

[WebKit Commit Bot]

Comment on attachment 88112 [details]
Patch

Rejecting attachment 88112 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
 error continues to appear after building again then the build system needs
    to be modified so that the inappropriate files are no longer copied in to the framework.

Command /bin/sh failed with exit code 1
Command /bin/sh failed with exit code 1
** BUILD FAILED **


The following build commands failed:
WebCore:
	PhaseScriptExecution "Check For Inappropriate Files In Framework" /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
(1 failure)


Full output: http://queues.webkit.org/results/8373286

[Mike Reed]

Created attachment 88817 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 88817 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Any way to test this?  We technically have printing tests but no one uses them.

[Mike Reed]

It is also testable when we define SKIA_GPU, which implements the isNativeFontRenderingAllowed() call. It is in that scenario that I saw the bug, where we had missed this one location in an earlier patch change replaced SKIA_TEXT with isNativeFontRenderingAllowed().

Without this patch, we fail to properly setup the paint (calling setupPaintForFont) but we still fall into skiaDrawText, which draws but with the wrong font, etc.

[Eric Seidel (no email)]

So when Chrome runs new-run-webkit-tests in "gpu tests mode" or whatever this path is tested (and baselines will need updates)?  Is that correct?

[Mike Reed]

Not yet. SKIA_GPU is not yet the default for gpu. It is just under developement. Under the existing SW and GPU builds, the only change should affect print-preview, which should get more correct stroked text. I don't expect any image rebaselining related to this change.

[Eric Seidel (no email)]

Ok.  Ideally your ChangeLog should explain why there is no testing.  In this case, this is impossible to test as it only affects print preview.

Unfortunately this chagne can't be cq+'d as is because it has OOPS on the test line.  the cq is smart enough to replace the OOPS in the reviewer line with the actual reviewer, but wont' fix the testing line and thus the pre-commit hook which checks for OOPS in ChangeLog files will fail.  It's kidna confusing, I know. :(

[Mike Reed]

Created attachment 88837 [details]
Patch

[Mike Reed]

updated Tests in ChangeLog (I had just forgotten to do that, sorry)

[Eric Seidel (no email)]

Comment on attachment 88837 [details]
Patch

Thanks.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 88837 [details]:

http/tests/websocket/tests/handshake-fail-by-no-cr.html bug 58156 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 88837 [details]
Patch

Clearing flags on attachment: 88837

Committed r83329: <http://trac.webkit.org/changeset/83329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.