Bug 57782

Created attachment 88112 [details] Patch

Comment on attachment 88112 [details] Patch Looks fine to me as long as it's been tested.

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

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?

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

Created attachment 88817 [details] Patch

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.

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.

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?

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.

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

Created attachment 88837 [details] Patch

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

Comment on attachment 88837 [details] Patch Thanks.

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 on attachment 88837 [details] Patch Clearing flags on attachment: 88837 Committed r83329: <http://trac.webkit.org/changeset/83329>

All reviewed patches have been landed. Closing bug.