[Webkit-unassigned] [Bug 60910] might need to bundle the functions related to visual word break as a class (VisualWordBreaker)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60910


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #101518|review?                     |review-
               Flag|                            |




--- Comment #9 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-20 15:56:38 PST ---
(From update of attachment 101518)
View in context: https://bugs.webkit.org/attachment.cgi?id=101518&action=review

> Source/WebCore/editing/FrameSelection.cpp:559
> -        pos = rightWordPosition(VisiblePosition(m_selection.extent(), m_selection.affinity()));
> +        pos = VisualWordBreakWindows().rightWordPosition(VisiblePosition(m_selection.extent(), m_selection.affinity()));

Again, I don't like the idea is that these two functions hang off of a class unlike all other functions in visible_units.cpp.  r- because of this.

> Source/WebCore/editing/visible_units.h:33
> +#include "VisiblePosition.h"
> +

If we made the class declaration internal to visible_units.cpp then we wouldn't have to include this new dependency. Header dependencies is a major cause of slow compilation time and we should strive to reduce it as much as possible.  As such, this new dependency further justifies my r-.

> Source/WebCore/editing/visible_units.h:100
> +        ~VisualWordBreakWindows() { }

I'd like to see a constructor that initializes member variables.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.