Bug 65203 – [skia] never draw with GDI, so that all text can be gpu-accelerated

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

[Mike Reed]

[skia] never draw with GDI, so that all text can be gpu-accelerated

[Mike Reed]

Created attachment 102045 [details]
Patch

[Mike Reed]

Created attachment 102052 [details]
Patch

[Mike Reed]

Created attachment 102054 [details]
Patch

[Mike Reed]

don't understand the style complaint. check-webkit-style passes locally.

[Kenneth Russell]

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.

[WebKit Review Bot]

Comment on attachment 102054 [details]
Patch

Clearing flags on attachment: 102054

Committed r91805: <http://trac.webkit.org/changeset/91805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

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

[Mike Reed]

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

[Mike Reed]

Created attachment 102272 [details]
Patch

[Mike Reed]

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

[Kenneth Russell]

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

[WebKit Review Bot]

Comment on attachment 102272 [details]
Patch

Clearing flags on attachment: 102272

Committed r92022: <http://trac.webkit.org/changeset/92022>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zhenyao Mo]

I reverted this patch due to a lot of chromium win tests failing.

The script doesn't finish, so I manually reopen this bug.

The reverting patch is r92150.

Here are some logs.

XP failures

   http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%282%29/builds/9330/steps/browser_tests/logs/stdio
   http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%283%29/builds/9888/steps/browser_tests/logs/stdio
   http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%284%29/builds/8610/steps/browser_tests/logs/stdio
   http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%285%29/builds/6299/steps/browser_tests/logs/stdio
   http://build.chromium.org/p/chromium/builders/XP%20Tests%20%28dbg%29%286%29/builds/2969/steps/browser_tests/logs/stdio

Vista bots failed exact the same tests

interactive tests

   http://build.chromium.org/p/chromium/builders/Interactive%20Tests%20%28dbg%29/builds/13437/steps/interactive_ui_tests/logs/stdio

[Mike Reed]

Created attachment 105198 [details]
Patch

[Kenneth Russell]

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?

[WebKit Review Bot]

Comment on attachment 105198 [details]
Patch

Clearing flags on attachment: 105198

Committed r93870: <http://trac.webkit.org/changeset/93870>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Podivilov]

Reverted r93870 for reason:

Broke i18n chromium tests

Committed r93881: <http://trac.webkit.org/changeset/93881>

[Mike Reed]

Created attachment 106424 [details]
Patch

[WebKit Review Bot]

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.

[Mike Reed]

Created attachment 106429 [details]
Patch

[Mike Reed]

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.

[Kenneth Russell]

(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?

[Mike Reed]

Indeed, that is the floating-accents part of the fix.

[Kenneth Russell]

Comment on attachment 106429 [details]
Patch

OK. r=me again

[WebKit Review Bot]

Comment on attachment 106429 [details]
Patch

Clearing flags on attachment: 106429

Committed r94589: <http://trac.webkit.org/changeset/94589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.