Bug 102795 - [css3-text] Add rendering support for -webkit-text-underline-position
Summary: [css3-text] Add rendering support for -webkit-text-underline-position
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Lamarque V. Souza
URL: http://dev.w3.org/csswg/css-text-deco...
Keywords:
Depends on: 102491
Blocks: 58491 101931
  Show dependency treegraph
 
Reported: 2012-11-20 04:14 PST by Lamarque V. Souza
Modified: 2022-02-27 23:13 PST (History)
28 users (show)

See Also:


Attachments
css3-text-underline-position (18.73 KB, patch)
2012-11-23 09:13 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (23.72 KB, patch)
2012-12-05 10:07 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS version) (54.85 KB, patch)
2013-01-08 20:12 PST, Lamarque V. Souza
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
css3-text-underline-position (EWS version) (63.44 KB, patch)
2013-01-10 08:17 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS version) (64.27 KB, patch)
2013-01-10 20:36 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
Test (9.31 KB, patch)
2013-01-14 08:39 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (176.97 KB, patch)
2013-01-18 14:31 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (221.96 KB, patch)
2013-01-18 14:35 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (222.66 KB, patch)
2013-01-20 19:19 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (222.16 KB, patch)
2013-01-23 16:37 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (175.96 KB, patch)
2013-01-24 05:19 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (75.21 KB, patch)
2013-02-22 05:36 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (122.06 KB, patch)
2013-02-22 05:42 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (121.36 KB, patch)
2013-02-23 09:26 PST, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (88.79 KB, patch)
2013-03-12 20:35 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (EWS only version) (103.27 KB, patch)
2013-03-13 08:15 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff
css3-text-underline-position (105.96 KB, patch)
2013-03-15 07:52 PDT, Lamarque V. Souza
leviw: review+
Details | Formatted Diff | Diff
css3-text-underline-position (117.78 KB, patch)
2013-03-18 12:02 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lamarque V. Souza 2012-11-20 04:14:35 PST
This bug intends to add rendering support for "text-underline-position" CSS3 property. Parsing support is implemented on bug 102491. This is a sub-task for bug 101931.
Comment 1 Lamarque V. Souza 2012-11-23 09:13:51 PST
Created attachment 175815 [details]
css3-text-underline-position

Implement 'alphabetical', 'under' and partially implement 'auto'
Comment 2 Lamarque V. Souza 2012-12-05 10:07:05 PST
Created attachment 177785 [details]
css3-text-underline-position

Update to properly implement TextUnderlinePositionUnder
Comment 3 Lamarque V. Souza 2013-01-08 20:12:32 PST
Created attachment 181828 [details]
css3-text-underline-position (EWS version)

Patch just for EWS
Comment 4 EFL EWS Bot 2013-01-08 21:57:28 PST
Comment on attachment 181828 [details]
css3-text-underline-position (EWS version)

Attachment 181828 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15758406
Comment 5 Lamarque V. Souza 2013-01-10 08:17:58 PST
Created attachment 182145 [details]
css3-text-underline-position (EWS version)

Attempt to fix problem reported by gtk and efl bots
Comment 6 Early Warning System Bot 2013-01-10 08:31:49 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15759987
Comment 7 Early Warning System Bot 2013-01-10 08:32:38 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15763988
Comment 8 Build Bot 2013-01-10 09:04:26 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15795263
Comment 9 Peter Beverloo (cr-android ews) 2013-01-10 09:37:30 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15806005
Comment 10 WebKit Review Bot 2013-01-10 09:59:49 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15795271

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 11 Peter Beverloo (cr-android ews) 2013-01-10 10:38:05 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15766902
Comment 12 Build Bot 2013-01-10 11:56:48 PST
Comment on attachment 182145 [details]
css3-text-underline-position (EWS version)

Attachment 182145 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15760982

New failing tests:
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-06.html
Comment 13 Lamarque V. Souza 2013-01-10 12:15:56 PST
Comment on attachment 177785 [details]
css3-text-underline-position

New version will be added once it passes EWS check.
Comment 14 Lamarque V. Souza 2013-01-10 20:36:52 PST
Created attachment 182255 [details]
css3-text-underline-position (EWS version)

Fixes all remaining known issues
Comment 15 Build Bot 2013-01-10 22:25:34 PST
Comment on attachment 182255 [details]
css3-text-underline-position (EWS version)

Attachment 182255 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15812095

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 16 WebKit Review Bot 2013-01-10 22:31:00 PST
Comment on attachment 182255 [details]
css3-text-underline-position (EWS version)

Attachment 182255 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15795496

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 17 WebKit Review Bot 2013-01-10 23:28:27 PST
Comment on attachment 182255 [details]
css3-text-underline-position (EWS version)

Attachment 182255 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15810186

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 18 Lamarque V. Souza 2013-01-14 08:39:53 PST
Created attachment 182581 [details]
Test

Test bots
Comment 19 Build Bot 2013-01-14 08:45:05 PST
Comment on attachment 182581 [details]
Test

Attachment 182581 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15870555
Comment 20 Lamarque V. Souza 2013-01-18 14:31:23 PST
Created attachment 183547 [details]
css3-text-underline-position

New patch with unit tests updated
Comment 21 Lamarque V. Souza 2013-01-18 14:35:56 PST
Created attachment 183548 [details]
css3-text-underline-position (EWS only version)

Same patch with css3-text enabled by default
Comment 22 WebKit Review Bot 2013-01-18 14:40:20 PST
Attachment 183548 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 WebKit Review Bot 2013-01-18 15:58:59 PST
Comment on attachment 183548 [details]
css3-text-underline-position (EWS only version)

Attachment 183548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15955432

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 24 Build Bot 2013-01-18 16:56:03 PST
Comment on attachment 183548 [details]
css3-text-underline-position (EWS only version)

Attachment 183548 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15949528

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 25 Build Bot 2013-01-18 18:15:47 PST
Comment on attachment 183548 [details]
css3-text-underline-position (EWS only version)

Attachment 183548 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15937960

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 26 Build Bot 2013-01-18 18:28:46 PST
Comment on attachment 183548 [details]
css3-text-underline-position (EWS only version)

Attachment 183548 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15943803
Comment 27 Build Bot 2013-01-19 05:40:30 PST
Comment on attachment 183548 [details]
css3-text-underline-position (EWS only version)

Attachment 183548 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15975194

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 28 Lamarque V. Souza 2013-01-20 19:19:30 PST
Created attachment 183698 [details]
css3-text-underline-position (EWS only version)

Move .png files to proper place.
Comment 29 WebKit Review Bot 2013-01-20 19:30:56 PST
Attachment 183698 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Build Bot 2013-01-20 20:03:44 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15967831
Comment 31 Build Bot 2013-01-20 21:01:46 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15968872
Comment 32 WebKit Review Bot 2013-01-20 21:09:28 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16011126

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 33 Build Bot 2013-01-20 22:05:33 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15967863

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 34 Build Bot 2013-01-20 22:05:40 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15968895
Comment 35 WebKit Review Bot 2013-01-20 22:13:44 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15974876

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 36 Build Bot 2013-01-20 22:27:06 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15975926

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 37 Build Bot 2013-01-20 23:09:15 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15974902
Comment 38 Build Bot 2013-01-20 23:19:51 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15968917

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 39 WebKit Review Bot 2013-01-21 00:14:34 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15975964

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 40 Build Bot 2013-01-21 00:16:01 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15977901

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 41 Build Bot 2013-01-21 00:16:45 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16011194
Comment 42 Build Bot 2013-01-21 00:28:07 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15975972

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 43 Build Bot 2013-01-21 01:15:03 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15974934
Comment 44 WebKit Review Bot 2013-01-21 05:15:15 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16031055

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 45 WebKit Review Bot 2013-01-21 06:14:17 PST
Comment on attachment 183698 [details]
css3-text-underline-position (EWS only version)

Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16033084

New failing tests:
fast/css-generated-content/content-property-change.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html
fast/css-generated-content/quotes-lang.html
fast/css-generated-content/close-quote-negative-depth.html
fast/css-generated-content/nested-quote-more-than-pairs.html
fast/css-generated-content/quotes-lang-case-insensitive.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-06.html
Comment 46 Lamarque V. Souza 2013-01-23 11:46:07 PST
(In reply to comment #45)
> (From update of attachment 183698 [details])
> Attachment 183698 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16033084
> 
> New failing tests:
> fast/css-generated-content/content-property-change.html
> fast/css/content/content-quotes-03.html
> fast/css/content/content-quotes-01.html
> fast/css-generated-content/quotes-lang.html
> fast/css-generated-content/close-quote-negative-depth.html
> fast/css-generated-content/nested-quote-more-than-pairs.html
> fast/css-generated-content/quotes-lang-case-insensitive.html
> fast/css/content/content-quotes-05.html
> fast/css/content/content-quotes-02.html
> fast/css/content/content-quotes-04.html
> fast/css/content/content-quotes-06.html

I have installed a chrooted Ubuntu installation (Lucid64) and none of those unit tests fail here. Anyone have any clue on what could be happening?
Comment 47 Lamarque V. Souza 2013-01-23 16:37:04 PST
Created attachment 184342 [details]
css3-text-underline-position (EWS only version)

Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.
Comment 48 WebKit Review Bot 2013-01-23 16:42:15 PST
Attachment 184342 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-underline-position.html', u'LayoutTests/fast/css3-text/css3-text-decoration/getComputedStyle/script-tests/getComputedStyle-text-underline-position.js', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5.html', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/GNUmakefile.features.am.in', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSPrimitiveValueMappings.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSRule.idl', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineFlowBox.h', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp', u'Source/WebCore/rendering/RootInlineBox.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h', u'Source/WebKit/chromium/features.gypi', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig', u'Source/cmake/OptionsCommon.cmake', u'Source/cmake/WebKitFeatures.cmake', u'Tools/Scripts/webkitperl/FeatureList.pm', u'Tools/qmake/mkspecs/features/features.pri', u'WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops', u'WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops', u'configure.ac']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Build Bot 2013-01-23 20:22:42 PST
Comment on attachment 184342 [details]
css3-text-underline-position (EWS only version)

Attachment 184342 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16084453
Comment 50 Lamarque V. Souza 2013-01-24 05:19:09 PST
Created attachment 184467 [details]
css3-text-underline-position

Remove changes from StyleResolver::adjustRenderStyle(), this fixes the regressions reported by buildbots.
Comment 51 WebKit Review Bot 2013-01-24 05:53:49 PST
Attachment 184467 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-auto5.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under1.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under2.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under3.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under4.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position-under5.html', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-alphabetic5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto5-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under1-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under2-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under3-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under4-expected.png', u'LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-under5-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/InlineFlowBox.cpp', u'Source/WebCore/rendering/InlineFlowBox.h', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/RootInlineBox.cpp', u'Source/WebCore/rendering/RootInlineBox.h']" exit_code: 1
LayoutTests/platform/chromium-linux/fast/css3-text/css3-text-decoration/text-underline-position-auto2-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 EFL EWS Bot 2013-01-24 06:48:08 PST
Comment on attachment 184467 [details]
css3-text-underline-position

Attachment 184467 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16083676
Comment 53 kov's GTK+ EWS bot 2013-01-24 07:22:05 PST
Comment on attachment 184467 [details]
css3-text-underline-position

Attachment 184467 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16078733
Comment 54 Lamarque V. Souza 2013-02-22 05:36:58 PST
Created attachment 189755 [details]
css3-text-underline-position

Update unit tests.
Comment 55 Lamarque V. Souza 2013-02-22 05:42:21 PST
Created attachment 189758 [details]
css3-text-underline-position (EWS only version)

Same patch with css3-text enabled.
Comment 56 EFL EWS Bot 2013-02-22 06:01:48 PST
Comment on attachment 189755 [details]
css3-text-underline-position

Attachment 189755 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16700811
Comment 57 Lamarque V. Souza 2013-02-23 09:26:28 PST
Created attachment 189929 [details]
css3-text-underline-position (EWS only version)

Update parser because of changes in WebKit source code.
Comment 58 Build Bot 2013-02-25 15:07:07 PST
Comment on attachment 189929 [details]
css3-text-underline-position (EWS only version)

Attachment 189929 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16744545
Comment 59 Peter Beverloo (cr-android ews) 2013-02-27 02:06:22 PST
Comment on attachment 189929 [details]
css3-text-underline-position (EWS only version)

Attachment 189929 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16672190
Comment 60 Lamarque V. Souza 2013-03-12 20:35:06 PDT
Created attachment 192863 [details]
css3-text-underline-position

Fix some code style issues in the patch.
Comment 61 Lamarque V. Souza 2013-03-13 08:15:16 PDT
Created attachment 192926 [details]
css3-text-underline-position (EWS only version)

Same patch with css3-text enabled.
Comment 62 Build Bot 2013-03-13 09:38:40 PDT
Comment on attachment 192926 [details]
css3-text-underline-position (EWS only version)

Attachment 192926 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17184026
Comment 63 Julien Chaffraix 2013-03-13 13:49:13 PDT
Comment on attachment 192863 [details]
css3-text-underline-position

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

I don't know much about line layout so someone (e.g. levi!) should chime in on that.

> Source/WebCore/ChangeLog:42
> +        * rendering/style/RenderStyle.h:

Please fill in the ChangeLog with useful information.

> Source/WebCore/rendering/InlineFlowBox.cpp:747
> +        if (curr->renderer()->isOutOfFlowPositioned())
> +            continue; // Positioned placeholders don't affect calculations.

This doesn't seem to be tested.

> Source/WebCore/rendering/InlineTextBox.cpp:974
> +        // FIXME: implement 'under left' and 'under right' and make TextUnderlinePositionAuto default to 'under left'
> +        // for vertical text.

Do we really need to repeat that we miss 'left|right' everywhere?

> Source/WebCore/rendering/InlineTextBox.cpp:983
> +    default:

We don't expect any value so I wouldn't put a 'default' as the compiler would be then able to catch any people forgetting to update this one.

The code in the default would be moved just out of the switch.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/script.js:2
> +if (window.testRunner)
> +    testRunner.dumpAsText(true);

This would be better inlined for clarity in the tests as it doesn't really save much per test.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css:1
> +body { font: 14px Ahem; -webkit-font-smoothing: none; -webkit-text-stroke: 1px black; -webkit-text-fill-color: white; }

Why not 10px? This code is very difficult to read as all the rules are on one line.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Unless this will be submitted to the official test suite, please use the HTML5 doctype: <!DOCTYPE HTML>

All the comments on this file apply to all the other test cases but I won't repeat them.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:7
> +        <title>CSS Test: CSS3 text-underline-position</title>
> +        <link rel="help" href="http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property"/>
> +        <meta name="flags" content="ahem"/>

I found these indications not very helpful as they are not dumped in the output.

If it's important enough, it should be dumped. If not, I think it should not be included.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:11
> +    <body>

You are missing 2 important facts in this test case:
* What are you testing? (a 'description' of the test case)
* What are the condition for passing? Ideally this should be a simple check (like 'there should be no red', 'you should see a 96px * 96px square').

See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree about how to write better test cases.
Comment 64 Levi Weintraub 2013-03-13 14:05:32 PDT
Comment on attachment 192863 [details]
css3-text-underline-position

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

> Source/WebCore/rendering/InlineFlowBox.cpp:750
> +        if (descendantsHaveSameLineHeightAndBaseline())
> +            continue;

This seems wrong. Shouldn't we still max maxLogicalTop with our curr->y() and just not recurse in this case? 0 is passed in initially, but just because this property is true doesn't mean we want to return 0, right?

> Source/WebCore/rendering/InlineFlowBox.h:325
> +    void computeMaxLogicalTops(float& maxLogicalTop);

We're not computing multiple logical tops, we're computing the one max. I'd use computeMaxLogicalTop.
Comment 65 Lamarque V. Souza 2013-03-13 18:31:53 PDT
(In reply to comment #63)
> Please fill in the ChangeLog with useful information.
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:747
> > +        if (curr->renderer()->isOutOfFlowPositioned())
> > +            continue; // Positioned placeholders don't affect calculations.
> 
> This doesn't seem to be tested.

I do not follow. Do you mean curr->renderer()->isOutOfFlowPositioned() always returns the same value? I created computeMaxLogicalTop() based on InlineFlowBox::computeLogicalBoxHeights() and added this test because it and several other methods do the same.
 
> > Source/WebCore/rendering/InlineTextBox.cpp:974
> > +        // FIXME: implement 'under left' and 'under right' and make TextUnderlinePositionAuto default to 'under left'
> > +        // for vertical text.
> 
> Do we really need to repeat that we miss 'left|right' everywhere?

Well, this comment is important because we will need to change how TextUnderlinePositionAuto works to match the specification once 'under left' is implemented. What about change the text to "FIXME: According to the specification TextUnderlinePositionAuto should default to 'under left' for vertical text. Implement that once 'under left' is implemented."? TextUnderlinePositionAuto should default to 'alphabetic' for horizontal text according to the specification.
 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/style.css:1
> > +body { font: 14px Ahem; -webkit-font-smoothing: none; -webkit-text-stroke: 1px black; -webkit-text-fill-color: white; }
> 
> Why not 10px? This code is very difficult to read as all the rules are on one line.

No reason, actually the font size is not important.
 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:7
> > +        <title>CSS Test: CSS3 text-underline-position</title>
> > +        <link rel="help" href="http://dev.w3.org/csswg/css-text-decor-3/#text-underline-position-property"/>
> > +        <meta name="flags" content="ahem"/>
> 
> I found these indications not very helpful as they are not dumped in the output.
> 
> If it's important enough, it should be dumped. If not, I think it should not be included.

I will remove them. All those indications can be found in the bug report too.
 
> > LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic2.html:11
> > +    <body>
> 
> You are missing 2 important facts in this test case:
> * What are you testing? (a 'description' of the test case)
> * What are the condition for passing? Ideally this should be a simple check (like 'there should be no red', 'you should see a 96px * 96px square').

The tests checks at which offset the horizontal lines are placed. There are two possible positions: at the font's baseline plus a offset, which is usually 1 pixel, or at the font box's bottom edge plus the same 1 pixel offset. "text-underline-position: auto" and "alphabetic" use the first option, which is simple to implement. "text-underline-position: under" is more problematic to implement because InlineTextBox::paintDecoration() draws the decoration relative current InlineBox's top edge. I created computeMaxLogicalTop() so I can get the maximum offset and then, in computeUnderlineOffset(), calculate the offset I need to add to which InlineBox's y() so it matches the position of the lower InlineBox (the one with maxLogicalTop). computeMaxLogicalTop() and computeUnderlineOffset() are basically used to translate relative y() coordinate to global y() coordiate.

It is difficult to see what the correct result is when using Ahem font, I used it because it is what WebKit recommends to use in unit tests, right? Do you think I can use another font here?

Some of the tests mixes different text-underline-position values to test if the code correctly handles that cases. vertical-align also affects "text-underline-position: under", so it is also tested. I will add the respective description to each test so they can be understood.
Comment 66 Lamarque V. Souza 2013-03-13 18:39:03 PDT
(In reply to comment #64)
> (From update of attachment 192863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192863&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:750
> > +        if (descendantsHaveSameLineHeightAndBaseline())
> > +            continue;
> 
> This seems wrong. Shouldn't we still max maxLogicalTop with our curr->y() and just not recurse in this case? 0 is passed in initially, but just because this property is true doesn't mean we want to return 0, right?

0 is a valid value for maxLogicalTop. If all InlineBox are aligned at the top of the page they all have y() == 0 and maxLogicalTop is 0. I need the recursion because curr()'s children must also be checked to calculate it, otherwise if one of its children is not aligned to the top of the parent (e.g. using "vertical-align: middle" like in text-underline-position-under5.html) it will have a higher y() than the parent and that must be counted according the text-underline-position's specification.
Comment 67 Julien Chaffraix 2013-03-14 08:04:32 PDT
(In the future, it's better to use the 'Review Patch' link to post side-by-side comments, our reviewing tool is fairly limited and wouldn't include your new comments).

> > > Source/WebCore/rendering/InlineFlowBox.cpp:747
> > > +        if (curr->renderer()->isOutOfFlowPositioned())
> > > +            continue; // Positioned placeholders don't affect calculations.
> > 
> > This doesn't seem to be tested.
> 
> I do not follow. Do you mean curr->renderer()->isOutOfFlowPositioned() always returns the same value? I created computeMaxLogicalTop() based on InlineFlowBox::computeLogicalBoxHeights() and added this test because it and several other methods do the same.

No, I merely talked about testing. There is no test with an out-of-flow positions element (postion: absolute or fixed) so this line is *definitely* not tested in any of them.
Comment 68 Lamarque V. Souza 2013-03-15 07:52:59 PDT
Created attachment 193309 [details]
css3-text-underline-position

Fix issues reported.
Comment 69 Levi Weintraub 2013-03-18 11:28:38 PDT
Comment on attachment 193309 [details]
css3-text-underline-position

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

The pixel tests make me sad, but it seems like they're necessary.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-all.html:4
> +    <!-- Bugzilla link: http://webkit.org/b/102795 -->

I'd rather see this in the description than in a comment in the test.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-alphabetic.html:4
> +    <!-- Bugzilla link: http://webkit.org/b/102795 -->

Ditto.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-auto.html:4
> +    <!-- Bugzilla link: http://webkit.org/b/102795 -->

Ditto.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html:4
> +    <!-- Bugzilla link: http://webkit.org/b/102795 -->

Ditto.

> LayoutTests/fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-under-out-of-flow.html:12
> +    <p>Test if out of flow boxes are ignored when calculating the offset for 'text-underline-position: under'. The horizontal line below should not go outside the box.</p>

Nit observation: Strangely long line when you wrapped the other descriptions.
Comment 70 Lamarque V. Souza 2013-03-18 12:02:09 PDT
Created attachment 193624 [details]
css3-text-underline-position

Patch for landing
Comment 71 WebKit Review Bot 2013-03-18 12:45:47 PDT
Comment on attachment 193624 [details]
css3-text-underline-position

Clearing flags on attachment: 193624

Committed r146104: <http://trac.webkit.org/changeset/146104>