Bug 114663

Summary: [css3-text] Rendering -webkit-hanging value for text-indent from css3-text
Product: WebKit Reporter: Jaehun Lim <ljaehun.lim>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, buildbot, commit-queue, esprehn+autocc, rniwa, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 112755    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Patch none

Description Jaehun Lim 2013-04-15 21:10:38 PDT
This is the rendering part for "-webkit-hanging" value from CSS3 text.
Comment 1 Jaehun Lim 2013-04-15 22:16:03 PDT
Created attachment 198235 [details]
Patch
Comment 2 Beth Dakin 2013-04-16 20:51:49 PDT
Comment on attachment 198235 [details]
Patch

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

Looks pretty close! I just have a few comments that I would like you to address. Let's do another round on this.

> Source/WebCore/ChangeLog:9
> +        "hanging" means "Inverts which lines are affected."

Maybe include a link to the spec?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1000
> +        shouldIndentText == IndentText ? shouldIndentText = DoNotIndentText : shouldIndentText = IndentText;

I find this line very difficult to read. I think it is more common in WebKit to write this with the assignment operator always on the left like so:

shouldIndentText = shouldIndentText == IndentText ? DoNotIndentText : IndentText;

> LayoutTests/ChangeLog:11
> +        * fast/css3-text/css3-text-indent/text-indent-each-line-expected.html: Removed.

Why is this file being removed? Seem like maybe removing it was a mistake? If it was not a mistake, you should explain it in the Changelog.
Comment 3 Jaehun Lim 2013-04-16 22:27:51 PDT
Comment on attachment 198235 [details]
Patch

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

Thanks for your reviewing.

>> Source/WebCore/ChangeLog:9
>> +        "hanging" means "Inverts which lines are affected."
> 
> Maybe include a link to the spec?

I'll add. http://dev.w3.org/csswg/css-text/#text-indent.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1000
>> +        shouldIndentText == IndentText ? shouldIndentText = DoNotIndentText : shouldIndentText = IndentText;
> 
> I find this line very difficult to read. I think it is more common in WebKit to write this with the assignment operator always on the left like so:
> 
> shouldIndentText = shouldIndentText == IndentText ? DoNotIndentText : IndentText;

I'll fix.

>> LayoutTests/ChangeLog:11
>> +        * fast/css3-text/css3-text-indent/text-indent-each-line-expected.html: Removed.
> 
> Why is this file being removed? Seem like maybe removing it was a mistake? If it was not a mistake, you should explain it in the Changelog.

I just renamed and updated text-indent-each-line-expected.html to text-indent-each-line-hanging-expected.html.
But webkit-patch script recognized it as "Remove and Added". I'll fix it "Renamed" manually.
Comment 4 Jaehun Lim 2013-04-16 22:58:35 PDT
Created attachment 198469 [details]
Patch
Comment 5 Beth Dakin 2013-04-17 11:02:16 PDT
Comment on attachment 198469 [details]
Patch

Looks good!
Comment 6 Build Bot 2013-04-17 14:41:53 PDT
Comment on attachment 198469 [details]
Patch

Attachment 198469 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/73242

New failing tests:
http/tests/cache/cached-main-resource.html
Comment 7 Build Bot 2013-04-17 14:41:54 PDT
Created attachment 198613 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2
Comment 8 Jaehun Lim 2013-04-17 17:19:49 PDT
Created attachment 198633 [details]
Patch
Comment 9 WebKit Commit Bot 2013-04-17 21:24:33 PDT
Comment on attachment 198633 [details]
Patch

Clearing flags on attachment: 198633

Committed r148658: <http://trac.webkit.org/changeset/148658>
Comment 10 WebKit Commit Bot 2013-04-17 21:24:35 PDT
All reviewed patches have been landed.  Closing bug.