Summary: | [CSS3] Add support for the word-break:keep-all CSS property | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | KyungTae Kim <ktf.kim> | ||||||||||
Component: | CSS | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bdakin, bfulgham, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, hyatt, kondapallykalyan, macpherson, menard, mmaxfield, pu007pang, simon.fraser, syoichi, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
KyungTae Kim
2013-11-04 20:30:34 PST
Created attachment 215993 [details]
test case
Created attachment 216000 [details]
Patch
Created attachment 216006 [details]
Patch
This is being asked for on Twitter. https://twitter.com/Captainmaumau/status/583148521672433665 Can someone review this patch? Does it still apply to TOT? This looks like a good patch. I'm really sad it was sitting here for so long! Let's see if we can find a good review for this. Comment on attachment 216006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216006&action=review In general, the approach is good. There are just some very small tweaks I'd make. > Source/WebCore/rendering/SimpleLineLayout.cpp:276 > + if (wordEndOffset > lineStartOffset && isBreakable(lineBreakIterator, wordEndOffset, nextBreakable, false, false)) We need to make sure that this new CSS value causes SLL not to be taken > Source/WebCore/rendering/break_lines.cpp:164 > + if (keepAllWords) I'm worried about the performance of this. It seems to me that this if statement should be hoisted into its own loop with an early return. (In reply to comment #7) > Comment on attachment 216006 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=216006&action=review > > In general, the approach is good. There are just some very small tweaks I'd > make. > > > Source/WebCore/rendering/SimpleLineLayout.cpp:276 > > + if (wordEndOffset > lineStartOffset && isBreakable(lineBreakIterator, wordEndOffset, nextBreakable, false, false)) > > We need to make sure that this new CSS value causes SLL not to be taken > > > Source/WebCore/rendering/break_lines.cpp:164 > > + if (keepAllWords) > > I'm worried about the performance of this. It seems to me that this if > statement should be hoisted into its own loop with an early return. Do you think you could have an updated patch by Monday? If not, I'll be happy to make these small changes and commit this on your behalf. Thanks! Myles Comment on attachment 216006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216006&action=review >> Source/WebCore/rendering/break_lines.cpp:164 >> + if (keepAllWords) > > I'm worried about the performance of this. It seems to me that this if statement should be hoisted into its own loop with an early return. Correction: I meant to say that it should be hoisted ::and have:: its own loop and an early return Created attachment 254544 [details]
Patch
Comment on attachment 254544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254544&action=review > Source/WebCore/rendering/break_lines.h:165 > +static inline int nextBreakablePositionKeepingAllWords(const CharacterType* str, unsigned length, unsigned pos) The static here is incorrect. Return type should be unsigned, not int. We use words, not abbreviations, for variable names. Should use names like "character" and "startPosition" rather than "str" and "pos". > Source/WebCore/rendering/break_lines.h:167 > + int len = static_cast<int>(length); Very silly to do this conversion to do something that is entirely suitable to do with unsigned. > Source/WebCore/rendering/break_lines.h:170 > + CharacterType ch = str[i]; > + if (isBreakableSpace<nbspBehavior>(ch)) Local variable just makes code harder to read. > Source/WebCore/rendering/break_lines.h:176 > +inline int nextBreakablePositionKeepingAllWords(LazyLineBreakIterator& lazyBreakIterator, int pos) We use words, not abbreviations, for variable names. Should use "startPosition" rather than "pos". Also, I suggest naming this argument "iterator", not "lazyBreakIterator". I know this matches existing functions below but they should eventually be fixed. Return value should be unsigned. > Source/WebCore/rendering/break_lines.h:184 > +inline int nextBreakablePositionKeepingAllWordsIgnoringNBSP(LazyLineBreakIterator& lazyBreakIterator, int pos) Ditto. > Source/WebCore/rendering/style/RenderStyleConstants.h:288 > +// Word Break Values. Matches both WinIE and CSS3 Comment seems to have almost no value. I suggest deleting it entirely. > LayoutTests/ChangeLog:10 > + * fast/text/word-break-keep-all.html: Added. > + * platform/mac/fast/text/word-break-keep-all-expected.png: Added. > + * platform/mac/fast/text/word-break-keep-all-expected.txt: Added. I think a reference test would be better. Can we please add word-break-keep-all-expected.html with explicit line breaks and avoid the platform/mac thing? Comment on attachment 254544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254544&action=review > LayoutTests/fast/text/word-break-keep-all.html:11 > +<p>On the next line, line should breaks between any two Korean syllables (word-break: normal)</p> Grammatical error: "should break", not "should breaks". > LayoutTests/fast/text/word-break-keep-all.html:14 > +<p>On the next line, line should breaks only at spaces like English (word-break: keep-all)<p> Grammatical error: "should break", not "should breaks". Committed r185729: <http://trac.webkit.org/changeset/185729> |