Bug 81408 - visual word movement: using cache to decrease the number of collectLeafBoxesInLogicalOrder on RootInlineBox
Summary: visual word movement: using cache to decrease the number of collectLeafBoxesI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2012-03-16 14:33 PDT by Xiaomei Ji
Modified: 2012-03-21 17:21 PDT (History)
2 users (show)

See Also:


Attachments
patch w/ layout test (12.82 KB, patch)
2012-03-16 14:44 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (14.68 KB, patch)
2012-03-19 11:30 PDT, Xiaomei Ji
rniwa: review+
Details | Formatted Diff | Diff
patch w/ layout test (14.45 KB, patch)
2012-03-21 15:08 PDT, Xiaomei Ji
rniwa: review+
xji: commit-queue-
Details | Formatted Diff | Diff
patch (14.67 KB, patch)
2012-03-21 15:41 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2012-03-16 14:33:34 PDT
visual word movement: using cache to decrease the number of collectLeafBoxesInLogicalOrder on RootInlineBox
Comment 1 Xiaomei Ji 2012-03-16 14:44:02 PDT
Created attachment 132385 [details]
patch w/ layout test
Comment 2 Ryosuke Niwa 2012-03-16 14:58:12 PDT
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 3 Xiaomei Ji 2012-03-19 11:28:18 PDT
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.
Comment 4 Xiaomei Ji 2012-03-19 11:30:41 PDT
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 5 Ryosuke Niwa 2012-03-21 11:50:07 PDT
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 6 Xiaomei Ji 2012-03-21 14:47:24 PDT
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()".
Comment 7 Xiaomei Ji 2012-03-21 15:08:12 PDT
Created attachment 133120 [details]
patch w/ layout test

sync-ed and address comments.
Comment 8 Xiaomei Ji 2012-03-21 15:12:35 PDT
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 9 Ryosuke Niwa 2012-03-21 15:14:59 PDT
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 :)
Comment 10 Xiaomei Ji 2012-03-21 15:41:43 PDT
Created attachment 133129 [details]
patch

add the change of "moving string declared in visualWordPosition() to outside of loop" in WebCore/ChangeLog.
Comment 11 WebKit Review Bot 2012-03-21 17:21:04 PDT
Comment on attachment 133129 [details]
patch

Clearing flags on attachment: 133129

Committed r111624: <http://trac.webkit.org/changeset/111624>
Comment 12 WebKit Review Bot 2012-03-21 17:21:09 PDT
All reviewed patches have been landed.  Closing bug.