[webkit-reviews] review denied: [Bug 60910] might need to bundle the functions related to visual word break as a class (VisualWordBreaker) : [Attachment 101518] Revised fix (now without introducing the class hierarchy)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Van Lam <vanlam at google.com>'s
request for review:
Bug 60910: might need to bundle the functions related to visual word break as a
class (VisualWordBreaker)
https://bugs.webkit.org/show_bug.cgi?id=60910

Attachment 101518: Revised fix (now without introducing the class hierarchy)
https://bugs.webkit.org/attachment.cgi?id=101518&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.