Bug 138970

Summary: [iOS] Regression(r176202): line-height is wrong on marco.org
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, ddkilzer, kling, koivisto, mmaxfield, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2014-11-21 10:27:57 PST 
line-height is wrong on marco.org after r176202, on iOS.

Radar: <rdar://problem/19039387>
Comment 1 Chris Dumez 2014-11-21 10:59:59 PST 
Created attachment 242052 [details]
Patch
Comment 2 Chris Dumez 2014-11-21 11:00:58 PST 
For reference, this was the code before:
https://bugs.webkit.org/attachment.cgi?id=241714&action=review
Comment 3 Simon Fraser (smfr) 2014-11-21 11:03:58 PST 
Comment on attachment 242052 [details]
Patch

Please add a layout test.
Comment 4 Chris Dumez 2014-11-21 12:57:44 PST 
Created attachment 242065 [details]
Patch
Comment 5 Chris Dumez 2014-11-21 13:08:01 PST 
@dbates: Is there a way I can explicitly NOT skip my new test on iOS (even though fast/css is skipped)? For e.g. Should I mark this specific test as [ Pass ] in the iphonesimulator TestExpectations file?
Comment 6 Myles C. Maxfield 2014-11-21 14:00:29 PST 
Yes. For an example, see LayoutTests/platform/mac/TestExpectations in https://bugs.webkit.org/attachment.cgi?id=242061&action=review
Comment 7 Chris Dumez 2014-11-21 14:05:33 PST 
Created attachment 242070 [details]
Patch
Comment 8 Chris Dumez 2014-11-21 14:07:25 PST 
(In reply to comment #6)
> Yes. For an example, see LayoutTests/platform/mac/TestExpectations in
> https://bugs.webkit.org/attachment.cgi?id=242061&action=review

Ok, done.
Comment 9 Simon Fraser (smfr) 2014-11-21 15:06:07 PST 
Comment on attachment 242070 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        No new tests, iOS specific. Verified manually on marco.org
> +        using an iPhone 6 Plus.

This is a lie!

> Source/WebCore/css/StyleBuilderCustom.h:518
> +template <ApplyTextSizeMultiplierOrNot applyTextMultiplier>
>  inline bool convertLineHeight(StyleResolver& styleResolver, const CSSValue& value, Length& length, float multiplier = 1.f)

Seems bloaty to generate 2 copies of this mostly-identical code. Why not just pass in a param, or just pass in styleResolver.style() or just get it from styleResolver?
Comment 10 Chris Dumez 2014-11-21 15:18:59 PST 
Created attachment 242079 [details]
Patch
Comment 11 Myles C. Maxfield 2014-11-21 15:24:53 PST 
Can't we make a real test? Perhaps that two lines with AHEM with a particular line height is the same as the same two lines except with a third blank line in between them?
Comment 12 Chris Dumez 2014-11-21 15:26:59 PST 
(In reply to comment #11)
> Can't we make a real test? Perhaps that two lines with AHEM with a
> particular line height is the same as the same two lines except with a third
> blank line in between them?

The bug is in the CSS StyleBuilder, I am testing the StyleBuilder.
Comment 13 Chris Dumez 2014-11-21 15:29:26 PST 
Created attachment 242081 [details]
Patch
Comment 14 WebKit Commit Bot 2014-11-21 17:22:34 PST 
Comment on attachment 242081 [details]
Patch

Clearing flags on attachment: 242081

Committed r176490: <http://trac.webkit.org/changeset/176490>
Comment 15 WebKit Commit Bot 2014-11-21 17:22:40 PST 
All reviewed patches have been landed.  Closing bug.