Bug 65203 - [skia] never draw with GDI, so that all text can be gpu-accelerated
Summary: [skia] never draw with GDI, so that all text can be gpu-accelerated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Reed
URL:
Keywords:
Depends on: 65226
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-26 12:54 PDT by Mike Reed
Modified: 2011-09-06 12:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2011-07-26 13:03 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (27.67 KB, patch)
2011-07-26 14:27 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (29.27 KB, patch)
2011-07-26 14:45 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (29.07 KB, patch)
2011-07-28 11:25 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (29.14 KB, patch)
2011-08-25 09:19 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (31.42 KB, patch)
2011-09-06 08:49 PDT, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (31.41 KB, patch)
2011-09-06 09:36 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-07-26 12:54:18 PDT
[skia] never draw with GDI, so that all text can be gpu-accelerated
Comment 1 Mike Reed 2011-07-26 13:03:33 PDT
Created attachment 102045 [details]
Patch
Comment 2 Mike Reed 2011-07-26 14:27:31 PDT
Created attachment 102052 [details]
Patch
Comment 3 Mike Reed 2011-07-26 14:45:50 PDT
Created attachment 102054 [details]
Patch
Comment 4 Mike Reed 2011-07-26 14:56:21 PDT
don't understand the style complaint. check-webkit-style passes locally.
Comment 5 Kenneth Russell 2011-07-26 17:02:59 PDT
Comment on attachment 102054 [details]
Patch

(Did you upload the patch with 'webkit-patch upload'? If yes, not sure what's going on with the style bot either.)

Looks fine to me as long as it's been tested.
Comment 6 WebKit Review Bot 2011-07-26 18:10:18 PDT
Comment on attachment 102054 [details]
Patch

Clearing flags on attachment: 102054

Committed r91805: <http://trac.webkit.org/changeset/91805>
Comment 7 WebKit Review Bot 2011-07-26 18:10:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Adrienne Walker 2011-07-26 20:46:53 PDT
Rolled out: http://trac.webkit.org/changeset/91822

This change looks like it breaks the tests listed below.  In particular, the @ symbol is missing the bottom part of its glyph, which is why I rolled this change out rather than just marking them as failures.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#master=ChromiumWebkit&tests=css1/color_and_background/background_attachment.html,editing/selection/end-of-document.html,fast/blockflow/japanese-ruby-horizontal-bt.html,fast/css/first-letter-capitalized.html,fast/css/font-face-implicit-local-font.html,fast/css/line-height-determined-by-primary-font.html,fast/dom/HTMLMeterElement/meter-boundary-values.html,fast/dom/dom-parse-serialize-display.html,fast/dom/dom-parse-serialize.html,fast/images/image-map-anchor-children.html,fast/ruby/nested-ruby.html,fast/text/atsui-negative-spacing-features.html,fast/text/backslash-to-yen-sign-dynamic.html,fast/text/backslash-to-yen-sign-euc.html,fast/text/backslash-to-yen-sign.html,fast/text/international/003.html,fast/text/international/bidi-LDB-2-CSS.html,fast/text/international/bidi-LDB-2-HTML.html,fast/text/international/bidi-LDB-2-formatting-characters.html,fast/text/international/bidi-european-terminators.html,fast/text/international/bidi-mirror-he-ar.html,fast/text/international/hindi-spacing.html,fast/text/international/hindi-whitespace.html,fast/text/international/khmer-selection.html,fast/text/justify-ideograph-leading-expansion.html,fonts/monospace.html,fonts/sans-serif.html,fonts/serif.html,http/tests/uri/css-href.php,svg/batik/text/verticalText.svg,svg/zoom/page/zoom-mask-with-percentages.svg,tables/mozilla/bugs/bug1828.html,tables/mozilla/bugs/bug20804.html,tables/mozilla/bugs/bug2479-3.html,tables/mozilla/bugs/bug2479-4.html,tables/mozilla/other/wa_table_tr_align.html,tables/mozilla_expected_failures/bugs/bug1055-2.html
Comment 9 Mike Reed 2011-07-27 05:03:37 PDT
required skia DEPS did *not* land yesterday (grrr). rev. 60 is required for this (among other things, it fixes the @). Working on that DEPS roll now...
Comment 10 Mike Reed 2011-07-28 11:25:15 PDT
Created attachment 102272 [details]
Patch
Comment 11 Mike Reed 2011-07-28 12:15:19 PDT
new patch just rebaselines the code after syncing, and tweaks the expectations file now that the skia 1960 expectations have been added (by a previous cl).
Comment 12 Kenneth Russell 2011-07-29 14:17:14 PDT
Comment on attachment 102272 [details]
Patch

OK, let's try again.

I'm CC'ing the current WebKit gardener in case more breakage is introduced by this patch. Mike, please keep an eye on the canaries (http://build.chromium.org/p/chromium.webkit/console).
Comment 13 WebKit Review Bot 2011-07-29 15:18:11 PDT
Comment on attachment 102272 [details]
Patch

Clearing flags on attachment: 102272

Committed r92022: <http://trac.webkit.org/changeset/92022>
Comment 14 WebKit Review Bot 2011-07-29 15:18:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Mike Reed 2011-08-25 09:19:03 PDT
Created attachment 105198 [details]
Patch
Comment 17 Kenneth Russell 2011-08-25 11:30:42 PDT
Comment on attachment 105198 [details]
Patch

r=me, but what if anything has changed since the last attempt to land this patch which will prevent the test failures seen the last time?
Comment 18 WebKit Review Bot 2011-08-26 06:13:04 PDT
Comment on attachment 105198 [details]
Patch

Clearing flags on attachment: 105198

Committed r93870: <http://trac.webkit.org/changeset/93870>
Comment 19 WebKit Review Bot 2011-08-26 06:13:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Pavel Podivilov 2011-08-26 09:26:38 PDT
Reverted r93870 for reason:

Broke i18n chromium tests

Committed r93881: <http://trac.webkit.org/changeset/93881>
Comment 21 Mike Reed 2011-09-06 08:49:12 PDT
Created attachment 106424 [details]
Patch
Comment 22 WebKit Review Bot 2011-09-06 09:00:03 PDT
Attachment 106424 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Last 3072 characters of output:
://src.chromium.org/svn/trunk/src/media@99393
    parsed_url: http://src.chromium.org/svn/trunk/src/media@99393
    should_process: True
    processed: True
    requirements: set(['./'])
  
    name: build
    url: http://src.chromium.org/svn/trunk/src/build@99393
    parsed_url: http://src.chromium.org/svn/trunk/src/build@99393
    should_process: True
    processed: True
    requirements: set(['./'])
  
    name: net
    url: http://src.chromium.org/svn/trunk/src/net@99393
    should_process: True
    requirements: set(['./'])
  
    name: tools/data_pack
    url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393
    should_process: True
    requirements: set(['./'])
  
    name: third_party/ffmpeg
    url: From('chromium_deps', 'src/third_party/ffmpeg')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/generate_stubs
    url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393
    should_process: True
    requirements: set(['./'])
  
    name: third_party/libjpeg_turbo
    url: From('chromium_deps', 'src/third_party/libjpeg_turbo')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/v8-i18n
    url: From('chromium_deps', 'src/third_party/v8-i18n')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: tools/grit
    url: http://src.chromium.org/svn/trunk/src/tools/grit@99393
    should_process: True
    requirements: set(['./'])
  
    name: base
    url: http://src.chromium.org/svn/trunk/src/base@99393
    should_process: True
    requirements: set(['./'])
  
    name: sql
    url: http://src.chromium.org/svn/trunk/src/sql@99393
    should_process: True
    requirements: set(['./'])
  
    name: v8
    url: From('chromium_deps', 'src/v8')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: testing/gtest
    url: From('chromium_deps', 'src/testing/gtest')
    should_process: True
    requirements: set(['testing', 'chromium_deps', './'])
  
    name: third_party/libwebp
    url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393
    should_process: True
    requirements: set(['third_party', './'])
  
    name: googleurl
    url: From('chromium_deps', 'src/googleurl')
    should_process: True
    requirements: set(['chromium_deps', './'])
  
    name: third_party/skia/include
    url: From('chromium_deps', 'src/third_party/skia/include')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/ots
    url: From('chromium_deps', 'src/third_party/ots')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])
  
    name: third_party/snappy/src
    url: From('chromium_deps', 'src/third_party/snappy/src')
    should_process: True
    requirements: set(['third_party', 'chromium_deps', './'])

Died at Tools/Scripts/update-webkit-chromium line 80.
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Mike Reed 2011-09-06 09:36:04 PDT
Created attachment 106429 [details]
Patch
Comment 24 Mike Reed 2011-09-06 09:42:04 PDT
fixed floating accents by negating dv in GOFFSET array to match skia's Y-is-down convention.

fixed justified text by passing justify advances to skia.
Comment 25 Kenneth Russell 2011-09-06 10:45:09 PDT
(In reply to comment #24)
> fixed floating accents by negating dv in GOFFSET array to match skia's Y-is-down convention.
> 
> fixed justified text by passing justify advances to skia.

Sounds good. Were you able to reproduce the i18n test failures locally to verify that your latest patch fixes them?
Comment 26 Mike Reed 2011-09-06 11:02:29 PDT
Indeed, that is the floating-accents part of the fix.
Comment 27 Kenneth Russell 2011-09-06 11:18:16 PDT
Comment on attachment 106429 [details]
Patch

OK. r=me again
Comment 28 WebKit Review Bot 2011-09-06 12:21:58 PDT
Comment on attachment 106429 [details]
Patch

Clearing flags on attachment: 106429

Committed r94589: <http://trac.webkit.org/changeset/94589>
Comment 29 WebKit Review Bot 2011-09-06 12:22:04 PDT
All reviewed patches have been landed.  Closing bug.