Summary: | [css3-text] Rendering -webkit-hanging value for text-indent from css3-text | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jaehun Lim <ljaehun.lim> | ||||||||||
Component: | CSS | Assignee: | 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
Jaehun Lim
2013-04-15 21:10:38 PDT
Created attachment 198235 [details]
Patch
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 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. Created attachment 198469 [details]
Patch
Comment on attachment 198469 [details]
Patch
Looks good!
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 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
Created attachment 198633 [details]
Patch
Comment on attachment 198633 [details] Patch Clearing flags on attachment: 198633 Committed r148658: <http://trac.webkit.org/changeset/148658> All reviewed patches have been landed. Closing bug. |