Bug 114663 – [css3-text] Rendering -webkit-hanging value for text-indent from css3-text

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Jaehun Lim]

This is the rendering part for "-webkit-hanging" value from CSS3 text.

[Jaehun Lim]

Created attachment 198235 [details]
Patch

[Beth Dakin]

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.

[Jaehun Lim]

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.

[Jaehun Lim]

Created attachment 198469 [details]
Patch

[Beth Dakin]

Comment on attachment 198469 [details]
Patch

Looks good!

[Build Bot]

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

[Build Bot]

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

[Jaehun Lim]

Created attachment 198633 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 198633 [details]
Patch

Clearing flags on attachment: 198633

Committed r148658: <http://trac.webkit.org/changeset/148658>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.