visual word movement: using cache to decrease the number of collectLeafBoxesInLogicalOrder on RootInlineBox
Created attachment 132385 [details] patch w/ layout test
Comment on attachment 132385 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=132385&action=review > Source/WebCore/editing/visible_units.cpp:173 > + const Vector<InlineBox*>& leafBoxesInLogicalOrder() const { return m_leafBoxes; } Can we call these functions just "collect" and "boxes"? It's redundant to repeat it in the function names. > Source/WebCore/editing/visible_units.cpp:174 > + We should probably add a function to find the index of a box in the vector. > Source/WebCore/editing/visible_units.cpp:197 > + const Vector<InlineBox*>& leafBoxesInLogicalOrder = rootAndLeafBoxes.collectLeafBoxesInLogicalOrder(root); It seems like this local variable is redundant. > Source/WebCore/editing/visible_units.cpp:235 > + if (!rootAndLeafBoxes.leafBoxesInLogicalOrder().size()) We should just add size() to simplify the expression here.
Comment on attachment 132385 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=132385&action=review >> Source/WebCore/editing/visible_units.cpp:173 >> + const Vector<InlineBox*>& leafBoxesInLogicalOrder() const { return m_leafBoxes; } > > Can we call these functions just "collect" and "boxes"? It's redundant to repeat it in the function names. changed to "collectBoxes". the other function is removed. >> Source/WebCore/editing/visible_units.cpp:174 >> + > > We should probably add a function to find the index of a box in the vector. done. >> Source/WebCore/editing/visible_units.cpp:197 >> + const Vector<InlineBox*>& leafBoxesInLogicalOrder = rootAndLeafBoxes.collectLeafBoxesInLogicalOrder(root); > > It seems like this local variable is redundant. The code is changed. Most of the functionalities are encapsulate in CachedRootAndLeafBoxesInLogicalOrder now. >> Source/WebCore/editing/visible_units.cpp:235 >> + if (!rootAndLeafBoxes.leafBoxesInLogicalOrder().size()) > > We should just add size() to simplify the expression here. done.
Created attachment 132616 [details] patch w/ layout test Comparing with previous patch, this patch encapsulates information better. previousBoxInLine and nextBoxInLine are removed, and their functionality are moved inside CachedRootAndLeafBoxesInLogicalOrder as previousTextBox and nextTextBox.
Comment on attachment 132616 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=132616&action=review r=me provided numOfBoxes is renamed to something else. > Source/WebCore/editing/visible_units.cpp:160 > +class CachedRootAndLeafBoxesInLogicalOrder { It might be better to call it CachedLogicallyOrderedLeafBoxes since we're not exactly caching root inline boxes. We're caching leaf boxes under a particular root box. > Source/WebCore/editing/visible_units.cpp:167 > + size_t numOfBoxes() const { return m_leafBoxes.size(); } Again, please don't use abbreviations like "num". Here, "size()" or "length()" would suffice since the class is called CachedRootAndLeafBoxesInLogicalOrder.
Comment on attachment 132616 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=132616&action=review >> Source/WebCore/editing/visible_units.cpp:160 >> +class CachedRootAndLeafBoxesInLogicalOrder { > > It might be better to call it CachedLogicallyOrderedLeafBoxes since we're not exactly caching root inline boxes. > We're caching leaf boxes under a particular root box. done. And changed the local variable name from "rootAndLeafBoxes" to "leafBoxes". >> Source/WebCore/editing/visible_units.cpp:167 >> + size_t numOfBoxes() const { return m_leafBoxes.size(); } > > Again, please don't use abbreviations like "num". Here, "size()" or "length()" would suffice since the class is called CachedRootAndLeafBoxesInLogicalOrder. changed to "size()".
Created attachment 133120 [details] patch w/ layout test sync-ed and address comments.
Comment on attachment 133120 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=133120&action=review > Source/WebCore/editing/visible_units.cpp:-374 > - Vector<UChar, 1024> string; I moved this to outside of loop (it is always clear-ed before use in wordBreakIteratorForMinOffsetBoundary() and wordBreakIteratorForMaxOffsetBoundary(). > Source/WebCore/editing/visible_units.cpp:407 > + iter = wordBreakIteratorForMinOffsetBoundary(visiblePosition, textBox, previousBoxLength, previousBoxInDifferentBlock, string, leafBoxes); After sync, this function's parameter becomes a bit lengthy. Do you think it is better to inline those 2 functions (wordBreakIteratorForMinOffsetBoundary and wordBreakIteratorForMaxOffsetBoundary) or worth to introduce a struct to hold those parameters? Neither of them sounds elegant for me.
Comment on attachment 133120 [details] patch w/ layout test View in context: https://bugs.webkit.org/attachment.cgi?id=133120&action=review >> Source/WebCore/editing/visible_units.cpp:407 >> + iter = wordBreakIteratorForMinOffsetBoundary(visiblePosition, textBox, previousBoxLength, previousBoxInDifferentBlock, string, leafBoxes); > > After sync, this function's parameter becomes a bit lengthy. Do you think it is better to inline those 2 functions (wordBreakIteratorForMinOffsetBoundary and wordBreakIteratorForMaxOffsetBoundary) or worth to introduce a struct to hold those parameters? Neither of them sounds elegant for me. We can do that in a follow up patch :)
Created attachment 133129 [details] patch add the change of "moving string declared in visualWordPosition() to outside of loop" in WebCore/ChangeLog.
Comment on attachment 133129 [details] patch Clearing flags on attachment: 133129 Committed r111624: <http://trac.webkit.org/changeset/111624>
All reviewed patches have been landed. Closing bug.