Bug 109825

Created attachment 188339 [details] Patch

Comment on attachment 188339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188339&action=review Useful addition, and code looks good (with nits), though I have this nagging feeling that CompareIndicesByWidthDifference/getNarrowDescendantsGroupedByDifferenceFromParentTextWidth is slightly overcomplicated. Julien/Kenneth, what do you think? > Source/WebCore/ChangeLog:29 > + different groups if the difference between their deltas is greater than 100 (in CSS That description doesn't seem quite right - if the deltas were [320, 400, 480], then those would all be the same group, right? > Source/WebCore/rendering/TextAutosizer.cpp:579 > +void TextAutosizer::getNarrowDescendantsGroupedByDifferenceFromParentTextWidth(const TextAutosizingClusterInfo& parentClusterInfo, Vector<Vector<TextAutosizingClusterInfo> >& groups) Nit: I wonder if this should return a Vector<Vector<TextAutosizingClusterInfo*> > to avoid copying the clusterinfos; they're moderately heavyweight, as they each contain a vector, though I guess in practice at this stage descendants are guaranteed to have empty narrowDescendants vectors so it's probably fine. > Source/WebCore/rendering/TextAutosizer.cpp:585 > + const float maxDifferenceWithinGroup = 100; Probably want to explain the purpose of this constant, and mention that it was empirically determined. (might also be clearer to move it down to the bottom next to where it gets used?) > Source/WebCore/rendering/TextAutosizer.cpp:588 > + differencesFromParentTextWidth.reserveInitialCapacity(clusterInfos.size()); Nit: it doesn't seem necessary to actually compute the differences. Directly sorting and grouping the cluster widths should be exactly equivalent to sorting and grouping the differences (except you'd need to sort in the opposite order). Though if you want to compress clusterInfos to a Vector of floats anyway in order to make the sort comparisons cheaper, then I guess you may as well use (parentTextWidth - clusterWidth) rather than just clusterWidth. So do what feels best. > Source/WebCore/rendering/TextAutosizer.cpp:595 > + float parentTextWidth = parentClusterInfo.blockContainingAllText->contentLogicalWidth(); Move this out of the loop. > Source/WebCore/rendering/TextAutosizer.cpp:603 > + groups.append(Vector<TextAutosizingClusterInfo>()); Nit: Move this out of the loop (then instead of the repeated if, you just have if (!clusterIndices); and you could remove that if too by returning early towards the beginning of this function if narrowDescendants is empty). You might also want to change this to groups.grow(1) for conciseness. > Source/WebCore/rendering/TextAutosizer.cpp:606 > + const TextAutosizingClusterInfo& currentClusterInfo = clusterInfos[currentIndex]; Redundant variable. > Source/WebCore/rendering/TextAutosizer.cpp:613 > + groups.append(Vector<TextAutosizingClusterInfo>()); Nit: you could do groups.grow(groups.size() + 1), but I'm fine with this if you prefer. > LayoutTests/fast/text-autosizing/narrow-descendants-combined.html:24 > <div style="width: 320px"> Perhaps additionally add a 240px one, to show that 240 and 400 get grouped together because of the 320px one?

Created attachment 188887 [details] Patch

Comment on attachment 188339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188339&action=review >> Source/WebCore/ChangeLog:29 >> + different groups if the difference between their deltas is greater than 100 (in CSS > > That description doesn't seem quite right - if the deltas were [320, 400, 480], then those would all be the same group, right? Right, rephrased according to the new implementation. >> Source/WebCore/rendering/TextAutosizer.cpp:579 >> +void TextAutosizer::getNarrowDescendantsGroupedByDifferenceFromParentTextWidth(const TextAutosizingClusterInfo& parentClusterInfo, Vector<Vector<TextAutosizingClusterInfo> >& groups) > > Nit: I wonder if this should return a Vector<Vector<TextAutosizingClusterInfo*> > to avoid copying the clusterinfos; they're moderately heavyweight, as they each contain a vector, though I guess in practice at this stage descendants are guaranteed to have empty narrowDescendants vectors so it's probably fine. I'd rather change it to Vector<Vector<TextAutosizingClusterInfo*> > in the following patch since narrowDescendants of clusterInfo's here are empty and changing to pointers will touch much unrelated code. >> Source/WebCore/rendering/TextAutosizer.cpp:585 >> + const float maxDifferenceWithinGroup = 100; > > Probably want to explain the purpose of this constant, and mention that it was empirically determined. (might also be clearer to move it down to the bottom next to where it gets used?) Done. >> Source/WebCore/rendering/TextAutosizer.cpp:588 >> + differencesFromParentTextWidth.reserveInitialCapacity(clusterInfos.size()); > > Nit: it doesn't seem necessary to actually compute the differences. Directly sorting and grouping the cluster widths should be exactly equivalent to sorting and grouping the differences (except you'd need to sort in the opposite order). Though if you want to compress clusterInfos to a Vector of floats anyway in order to make the sort comparisons cheaper, then I guess you may as well use (parentTextWidth - clusterWidth) rather than just clusterWidth. So do what feels best. Done. Sorting by the width now. The code is much simpler. >> Source/WebCore/rendering/TextAutosizer.cpp:595 >> + float parentTextWidth = parentClusterInfo.blockContainingAllText->contentLogicalWidth(); > > Move this out of the loop. Done. >> Source/WebCore/rendering/TextAutosizer.cpp:603 >> + groups.append(Vector<TextAutosizingClusterInfo>()); > > Nit: Move this out of the loop (then instead of the repeated if, you just have if (!clusterIndices); and you could remove that if too by returning early towards the beginning of this function if narrowDescendants is empty). You might also want to change this to groups.grow(1) for conciseness. Done. >> Source/WebCore/rendering/TextAutosizer.cpp:606 >> + const TextAutosizingClusterInfo& currentClusterInfo = clusterInfos[currentIndex]; > > Redundant variable. Done. >> Source/WebCore/rendering/TextAutosizer.cpp:613 >> + groups.append(Vector<TextAutosizingClusterInfo>()); > > Nit: you could do groups.grow(groups.size() + 1), but I'm fine with this if you prefer. Done. >> LayoutTests/fast/text-autosizing/narrow-descendants-combined.html:24 >> <div style="width: 320px"> > > Perhaps additionally add a 240px one, to show that 240 and 400 get grouped together because of the 320px one? Done.

Comment on attachment 188887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188887&action=review > Source/WebCore/rendering/TextAutosizer.cpp:564 > +bool greaterClusterInfoRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second) I am not convinced about this name... what about something with compare... people are used to compare methods

Created attachment 188893 [details] Patch

Comment on attachment 188887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188887&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:564 >> +bool greaterClusterInfoRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second) > > I am not convinced about this name... what about something with compare... people are used to compare methods Ok, done. Do I need to reflect in the name that the method compares values in the descending order or will a comment suffice?

Comment on attachment 188893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188893&action=review Much simpler now, thanks. Looks good with teensy nits. > I'd rather change it to Vector<Vector<TextAutosizingClusterInfo*> > in the > following patch since narrowDescendants of clusterInfo's here are empty and > changing to pointers will touch much unrelated code. Oh, I'd forgotten that these get passed to processCompositeCluster. Definitely fine to keep it as non-pointers then. > Source/WebCore/rendering/TextAutosizer.cpp:565 > +bool compareClusterInfosByRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second) Hmm, how about just "clusterWiderThan"? Kenneth, what do you think? > Source/WebCore/rendering/TextAutosizer.cpp:585 > + // this value, the next element should start a new group. Nit: s/value/empirically determined value/

> Hmm, how about just "clusterWiderThan"? Kenneth, what do you think? Sounds good or clusterWiderThanComparisonFn()

Created attachment 188924 [details] Patch

Comment on attachment 188924 [details] Patch Clearing flags on attachment: 188924 Committed r143318: <http://trac.webkit.org/changeset/143318>

All reviewed patches have been landed. Closing bug.