might need to bundle the functions related to visual word break as a class (VisualWordBreaker)

• Component: HTML Editing
• Status: RESOLVED INVALID

[Xiaomei Ji]

Since some functions (related to visual word break) share the same/similar parameters, we might want to bundle them together as a class (such as VisualWordBreak).

[Van Lam]

Created attachment 101416 [details]
Proposed fix

[Ryosuke Niwa]

Comment on attachment 101416 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=101416&action=review

> Source/WebCore/editing/visible_units.h:99
> +class VisualWordBreakBase {

I don't think we want to expose this in the header file.

> Source/WebCore/editing/visible_units.h:175
> +class VisualWordBreakWindows : public VisualWordBreakBase {
> +    public:
> +        ~VisualWordBreakWindows() { }
> +        VisiblePosition leftWordPosition(const VisiblePosition& visiblePosition);
> +        VisiblePosition rightWordPosition(const VisiblePosition& visiblePosition);
> +
> +    private:
> +        void collectWordBreaksInBoxInsideBlockWithSameDirectionality();
> +        void collectWordBreaksInBoxInsideBlockWithDifferntDirectionality();
> +        void collectWordBreaksInBox();
> +};

I'm not sure a class hierarchy is the right way to approach this.  I wouldn't speculatively prepare for other platforms unless we already have some local patch that works on Mac and Unix.

[Van Lam]

Hi Ryosuke. I've implemented a Mac/Linux version but thought it would be better to split the refactoring of the Windows version and adding the Mac/Linux version into two patches. And can you clarify why we don't want to expose the base class definition in the header file? Thanks

(In reply to comment #2)
> (From update of attachment 101416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101416&action=review
> 
> > Source/WebCore/editing/visible_units.h:99
> > +class VisualWordBreakBase {
> 
> I don't think we want to expose this in the header file.
> 
> > Source/WebCore/editing/visible_units.h:175
> > +class VisualWordBreakWindows : public VisualWordBreakBase {
> > +    public:
> > +        ~VisualWordBreakWindows() { }
> > +        VisiblePosition leftWordPosition(const VisiblePosition& visiblePosition);
> > +        VisiblePosition rightWordPosition(const VisiblePosition& visiblePosition);
> > +
> > +    private:
> > +        void collectWordBreaksInBoxInsideBlockWithSameDirectionality();
> > +        void collectWordBreaksInBoxInsideBlockWithDifferntDirectionality();
> > +        void collectWordBreaksInBox();
> > +};
> 
> I'm not sure a class hierarchy is the right way to approach this.  I wouldn't speculatively prepare for other platforms unless we already have some local patch that works on Mac and Unix.

[Ryosuke Niwa]

(In reply to comment #3)
> Hi Ryosuke. I've implemented a Mac/Linux version but thought it would be better to split the refactoring of the Windows version and adding the Mac/Linux version into two patches.

Please upload your work in progress patch somewhere so I can see where you're headed.

> And can you clarify why we don't want to expose the base class definition in the header file? Thanks

We should strive to minimize dependency between cpp files as much as possible.  If you put the class declaration in the header file, then compiler have to parse and create AST for every cpp file that includes visbible_units.h.  Also, no other functions in visible_units are class members so this seems like an anti-pattern.

[Van Lam]

Created attachment 101484 [details]
Just to show where Mac/Linux version is going

[Ryosuke Niwa]

Comment on attachment 101416 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=101416&action=review

> Source/WebCore/editing/visible_units.h:-45
> -VisiblePosition rightWordPosition(const VisiblePosition&);
> -VisiblePosition leftWordPosition(const VisiblePosition&);

How come call sites of these functions are not changed?  I think this is a bad change anyways.  We should expose the internals (i.e. the fact it's implemented as a class) of rightWordPosition and leftWordPosition to the outside world.

[Van Lam]

Created attachment 101518 [details]
Revised fix (now without introducing the class hierarchy)

The patch is still big, but almost every diff in visible_units.cpp can be classified as either:
1) an access to a data member (which was previously passed into the function as an argument)
2) setting a data member for the callee to access
3) a function call in which fewer arguments are passed (because now commonly-passed arguments are saved as data members)
4) labeling previously static functions as member functions

[Ryosuke Niwa]

(In reply to comment #7)
> Created an attachment (id=101518) [details]
> Revised fix (now without introducing the class hierarchy)

It seems like your patch doesn't apply on ToT WebKit.  Please resolve conflict and upload it again.

[Ryosuke Niwa]

Comment on attachment 101518 [details]
Revised fix (now without introducing the class hierarchy)

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.

[Ryosuke Niwa]

You can probably add wrapper functions for leftWordPosition and rightWordPosition that instantiate new class and then call appropriate member functions.

But new patch is much better in that there's no big code moves.

[Ryosuke Niwa]

Eric and Darin might have some opinions on how to best-refactor these functions.

[Van Lam]

Created attachment 101541 [details]
Revised fix

Made requested changes; now:

1) leftWordPosition and rightWordPosition are called through non-member wrapper functions of the same names
2) the class definition is in visible_units.cpp; the header file's dependency on VisiblePosition is removed

[WebKit Review Bot]

Attachment 101541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/editing/visible_units.cpp:1193:  The parameter name "visiblePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/editing/visible_units.cpp:1194:  The parameter name "visiblePosition" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Van Lam]

Created attachment 101542 [details]
Revised fix

Fixed the style problem in previous submission.

[Xiaomei Ji]

Maybe put the class in unnamed namespace?

[Ryosuke Niwa]

Comment on attachment 101542 [details]
Revised fix

View in context: https://bugs.webkit.org/attachment.cgi?id=101542&action=review

I'm not sure if this patch is an improvement or not.  It appears that all we need is RenderedPosition proposed in https://docs.google.com/document/d/1vzzi_wQHO0GtLnu-1IYutUKbcpAYlHqZwQlwbfLuowM/edit?hl=en_US&authkey=CLOa9cgN

> Source/WebCore/ChangeLog:3
> +        might need to bundle the functions related to visual word break as a class (VisualWordBreaker)

We should get rid of "might need to" from the bug title now that we're doing it.

> Source/WebCore/editing/visible_units.cpp:1188
> +        static const int s_invalidOffset = -1;

Again, I don't think we should prefix this constant with "s_" as it's not a static "variable".

> Source/WebCore/editing/visible_units.cpp:1190
> +        struct WordBoundaryEntry;

You can just move the definition of WordBoundaryEntry here since it shouldn't be visible outside of this class.

> Source/WebCore/editing/visible_units.cpp:1521
> +int VisualWordBreakWindows::greatestValueUnder()

Now this function name is very weird.  The greatest value under what?  We need to rename this function so that the semantics will be clear.

> Source/WebCore/editing/visible_units.cpp:1546
> -static int smallestOffsetAbove(int offset, bool boxAndBlockAreInSameDirection, const WordBoundaryVector& orderedWordBoundaries)
> +int VisualWordBreakWindows::smallestOffsetAbove()

Ditto.

[Ryosuke Niwa]

Comment on attachment 101542 [details]
Revised fix

After the discussion with Van in person, I don't think we need this patch anymore.

[Xiaomei Ji]

we wont go this direction, instead, we will probably use RenderedPosition.