Bug 57782 - Replace SKIA_TEXT with isNativeFontRenderingAllowed() for print-preview to work
Summary: Replace SKIA_TEXT with isNativeFontRenderingAllowed() for print-preview to work
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Mike Reed
Depends on:
Reported: 2011-04-04 13:22 PDT by Mike Reed
Modified: 2011-04-08 13:08 PDT (History)
5 users (show)

See Also:

Patch (1.57 KB, patch)
2011-04-04 13:23 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (1.53 KB, patch)
2011-04-08 07:02 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2011-04-08 10:12 PDT, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-04-04 13:22:44 PDT
Replace SKIA_TEXT with isNativeFontRenderingAllowed() for print-preview to work
Comment 1 Mike Reed 2011-04-04 13:23:46 PDT
Created attachment 88112 [details]
Comment 2 Kenneth Russell 2011-04-06 17:36:43 PDT
Comment on attachment 88112 [details]

Looks fine to me as long as it's been tested.
Comment 3 Kenneth Russell 2011-04-06 19:04:31 PDT
Also, did you want a cq+? Please mark bugs with cq? if so. ('webkit-patch upload' does this automatically.)
Comment 4 Chris Guillory 2011-04-07 14:44:00 PDT
Shouldn't this change already be landed from this change:

Am I missing something?
Comment 5 WebKit Commit Bot 2011-04-07 17:03:48 PDT
Comment on attachment 88112 [details]

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

The following build commands failed:
	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
Comment 6 Mike Reed 2011-04-08 07:02:01 PDT
Created attachment 88817 [details]
Comment 7 Eric Seidel (no email) 2011-04-08 08:08:58 PDT
Comment on attachment 88817 [details]

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.
Comment 8 Mike Reed 2011-04-08 08:23:21 PDT
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.
Comment 9 Eric Seidel (no email) 2011-04-08 08:25:40 PDT
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?
Comment 10 Mike Reed 2011-04-08 08:47:37 PDT
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.
Comment 11 Eric Seidel (no email) 2011-04-08 10:06:20 PDT
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. :(
Comment 12 Mike Reed 2011-04-08 10:12:43 PDT
Created attachment 88837 [details]
Comment 13 Mike Reed 2011-04-08 10:13:21 PDT
updated Tests in ChangeLog (I had just forgotten to do that, sorry)
Comment 14 Eric Seidel (no email) 2011-04-08 10:19:16 PDT
Comment on attachment 88837 [details]

Comment 15 WebKit Commit Bot 2011-04-08 13:04:56 PDT
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: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2011-04-08 13:08:32 PDT
Comment on attachment 88837 [details]

Clearing flags on attachment: 88837

Committed r83329: <http://trac.webkit.org/changeset/83329>
Comment 17 WebKit Commit Bot 2011-04-08 13:08:36 PDT
All reviewed patches have been landed.  Closing bug.