Bug 49053 – Refactoring: Return value of TextCheckingHelper::paragraphAlignedRange should form a class.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Hajime Morrita]

TextEditingHelper::paragraphAlignedRange() has one return value and 2 "out" parameters.
They can form single class, like ParagraphRange.

[Hajime Morrita]

class name is wrong: TextEditingHelper -> TextCheckingHelper

[Hajime Morrita]

Created attachment 73038 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 73038 [details]
Patch

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

I like the idea of isolating the code into a class  It's much more readable!

> WebCore/editing/Editor.cpp:2211
> -            RefPtr<Range> offsetAsRange = Range::create(paragraphRange->startContainer(ec)->document(), paragraphRange->startPosition(), paragraphRange->startPosition());
> +            RefPtr<Range> offsetAsRange = paragraph.offsetAsRange();
>              Position caretPosition = m_frame->selection()->end();
>              offsetAsRange->setEnd(caretPosition.containerNode(), caretPosition.computeOffsetInContainerNode(), ec);
>              if (!ec) {
>                  selectionOffset = TextIterator::rangeLength(offsetAsRange.get());
>                  restoreSelectionAfterChange = true;
> -                if (selectionOffset > 0 && (selectionOffset > paragraphLength || paragraphString[selectionOffset - 1] == newlineCharacter))
> +                if (selectionOffset > 0 && (selectionOffset > paragraph.length() || paragraph.charAt(selectionOffset - 1) == newlineCharacter))
>                      adjustSelectionForParagraphBoundaries = true;
> -                if (selectionOffset > 0 && selectionOffset <= paragraphLength && isAmbiguousBoundaryCharacter(paragraphString[selectionOffset - 1]))
> +                if (selectionOffset > 0 && selectionOffset <= paragraph.length() && isAmbiguousBoundaryCharacter(paragraph.charAt(selectionOffset - 1)))

I really want to hide offsetAsRange concept inside the class.  Is there something that prevents us from doing so?

> WebCore/editing/TextCheckingHelper.h:30
> +class ParagraphRange {

ParagraphRange sounds too general.  This is basically a wrapper for Range that extends to Paragraph for spell-checking purposes, right?  Maybe it's better to name it something like TextCheckingRange that emphasize the fact this is used for spelling / grammar checks rather than for general use in editing.

> WebCore/editing/TextCheckingHelper.h:38
> +    PassRefPtr<Range> originalRange() const { return m_originalRange; }
> +    PassRefPtr<Range> range() const;
> +    PassRefPtr<Range> subrange(int characterOffset, int characterCount) const;

I find it odd that we have "original" range.  What does it mean to say original?  I think it's probably better to have range() that returns the original range, rangeInParagraph() which combines range() and subrange() in your implementation.  We can give some special default values (-1 or 0?) to offset & length in the case of obtaining the range for the entire paragraph.

> WebCore/editing/TextCheckingHelper.h:41
> +    int startOffset() const;
> +    int endOffset() const;

These are really offsetToStartOfParagraph and offsetToEndOfParagraph, right?

> WebCore/editing/TextCheckingHelper.h:43
> +    bool isRangeEmpty() const;
> +    bool isValid() const { return m_originalRange; }

These functions are particularly confusing because isRangeEmpty() asks about the range extended to paragraph, and isValid asks about the original range.  We should make these distinction more explicit.  Rename them to something like isParagraphEmpty() and isRangeValid()

> WebCore/editing/TextCheckingHelper.h:51
> +    bool isEmpty() const { return string().isEmpty(); }

It's ambiguous what's empty here.  Rename it to something like isStringEmpty().  But I don't understand why we need both isRangeEmpty and isStringEmpty.  I feel like all we need is isStringEmpty.

[Hajime Morrita]

Created attachment 73052 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for taking a look!
I revised the patch.

In the latest patch, I
- renamed the class to TextCheckingParagraph to emphasize
   - it's a text checking specific class.
   - its intentions is to encapsulate "paragraph" range.
- hid raw accessors and make more caller-aware APIs.
- removed some confusing APIs.

[Jia Pu]

Comment on attachment 73052 [details]
Patch

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

> WebCore/editing/Editor.cpp:2197
> +    if (shouldMarkGrammar ? grammarParagraph.isEmpty() : spellingParagraph.isEmpty())

Not sure if this logic is identical to the one on line 2212 in current code. In current code, spelling range is always checked, while in new code, it's only checked if shouldMarkGrammar is false.

> WebCore/editing/TextCheckingHelper.cpp:108
> +PassRefPtr<Range> TextCheckingParagraph::offsetAsRange() const

Shall we consider caching the return value of this function. This function is called by checkingStart(), which is called frequently throughout the code.

[Hajime Morrita]

Created attachment 73207 [details]
Patch

[Hajime Morrita]

Hi Jian, thank you for taking a look!
I updated the patch to address your feedback.
 
> > WebCore/editing/Editor.cpp:2197
> > +    if (shouldMarkGrammar ? grammarParagraph.isEmpty() : spellingParagraph.isEmpty())
> 
> Not sure if this logic is identical to the one on line 2212 in current code. In current code, spelling range is always checked, while in new code, it's only checked if shouldMarkGrammar is false.

Good catch. Fixed to check range length...

> 
> > WebCore/editing/TextCheckingHelper.cpp:108
> > +PassRefPtr<Range> TextCheckingParagraph::offsetAsRange() const
> 
> Shall we consider caching the return value of this function. This function is called by checkingStart(), which is called frequently throughout the code.

Done. I'd note that checkingStart() result is cached.

[Ryosuke Niwa]

Comment on attachment 73207 [details]
Patch

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

I'm not familiar with spelling/grammar correction code, I'm making comments on clarify of the API.

> WebCore/editing/TextCheckingHelper.cpp:50
> +    : m_checkingRange(checkingRange)
> +    , m_checkingStart(-1)
> +    , m_checkingEnd(-1)
> +    , m_checkingLength(-1)

Can we use targetRange or rangeForTextChecking instead of checkingRange?  It's not really clear what "checking" refers to given that the class itself is called Text"Checking"Paragraph.

> WebCore/editing/TextCheckingHelper.cpp:58
> +void TextCheckingParagraph::expandRangeToNextEnd()

It's not clear to what end you're extending the range here.  It's probably better to call it extendRangeToNextParagraph.

> WebCore/editing/TextCheckingHelper.cpp:76
> +int TextCheckingParagraph::rangeLength() const
> +{
> +    ASSERT(m_checkingRange);
> +    return TextIterator::rangeLength(paragraphRange().get());
> +}

You should really call this function paragraphLength or alike since there are two ranges in this class.

> WebCore/editing/TextCheckingHelper.cpp:86
> +PassRefPtr<Range> TextCheckingParagraph::subrange(int characterOffset, int characterCount) const

subrangeInParagraph?

> WebCore/editing/TextCheckingHelper.cpp:92
> +int TextCheckingParagraph::offsetTo(const Position& position, ExceptionCode& ec) const

I don't understand what this function does.  My guess is that it obtains the offset of the given position from the start of the paragraph.  Then it should be called offsetFromStartOfParagraph

> WebCore/editing/TextCheckingHelper.cpp:105
> +    // Both predicates should have same result, but we check both just for sure.
> +    // We need to investigate to remove this redundancy.

Nice comment.

> WebCore/editing/TextCheckingHelper.cpp:109
> +PassRefPtr<Range> TextCheckingParagraph::offsetAsRange() const

rangeFromStartOfParagraphToStartOfTargetText?  offsetAsRange is too vague IMHO.

> WebCore/editing/TextCheckingHelper.cpp:120
> +const String& TextCheckingParagraph::text() const

I prefer to call it targetText but you can call it checkingText as well.  But when I just see text(), I'm not sure if it obtains the entire paragraph or the target/checking text.

> WebCore/editing/TextCheckingHelper.cpp:128
> +int TextCheckingParagraph::checkingStart() const

startOfTargetTextInParagraph?

> WebCore/editing/TextCheckingHelper.cpp:136
> +int TextCheckingParagraph::checkingEnd() const

Likewise.

> WebCore/editing/TextCheckingHelper.h:48
> +    int textLength() const { return text().length(); }
> +    String textSubstring(unsigned pos, unsigned len = UINT_MAX) const { return text().substring(pos, len); }
> +    const UChar* textCharacters() const { return text().characters(); }
> +    UChar textCharAt(int index) const { return text()[index]; }
> +
> +    bool isEmpty() const;
> +    bool isTextEmpty() const { return text().isEmpty(); }
> +    bool isRangeEmpty() const { return checkingStart() >= checkingEnd(); }

I don't understand why we need these wrapper functions for length, substring, characters, [], and empty if we're just calling text() internally.  Will it be a problem if we just exposed text() instead?

[Hajime Morrita]

Comment on attachment 73207 [details]
Patch

Hi Ryosuke, thank you for taking a look again!
I start thinking that how the class extracted is wrong and bad names imply that.

it contains different concept:
- cached range related values
- relationship between two ranges
and each should have separate classes. So I'll investigate how to represent it.

[Hajime Morrita]

Created attachment 73352 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 73352 [details]
Patch

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

I think splitting up into two classes turned out to be a good decision.

> WebCore/editing/TextCheckingHelper.cpp:80
> +TextCheckingSpan TextCheckingRange::overlapWith(const TextCheckingRange& range) const
> +{
> +    ExceptionCode ec = 0;
> +    return TextCheckingSpan(rangeFromStartTo(range.range()->startPosition(), ec).length(),
> +                            range.length());
> +}

I don't think overlapWith is a good name.  spanForRange maybe?

> WebCore/editing/TextCheckingHelper.cpp:96
> +void TextCheckingRange::expandToNextParagraph()
> +{
> +    setEnd(range(), endOfParagraph(startOfNextParagraph(range()->startPosition())));
> +    invalidateCaches();
> +}

It should be "extend" not "expand".

> WebCore/editing/TextCheckingHelper.cpp:98
> +TextCheckingRange TextCheckingRange::expandToParagraph(PassRefPtr<Range> range)

Ditto.

> WebCore/editing/TextCheckingHelper.h:38
> +class TextCheckingSpan {
> +public:
> +    explicit TextCheckingSpan(int start = 0, int length = 0)
> +        : m_start(start)
> +        , m_length(length)
> +    {}

I'm not sure "span" is a good word for this but then I can't come up with a better one.

[Hajime Morrita]

Created attachment 73458 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for reviewing again!
I updated the patch.

> > WebCore/editing/TextCheckingHelper.cpp:80
> > +TextCheckingSpan TextCheckingRange::overlapWith(const TextCheckingRange& range) const
....
> I don't think overlapWith is a good name.  spanForRange maybe?
I see. It got renamed to spanFor()

> > WebCore/editing/TextCheckingHelper.cpp:96
> > +void TextCheckingRange::expandToNextParagraph()
> > +{
> > +    setEnd(range(), endOfParagraph(startOfNextParagraph(range()->startPosition())));
> > +    invalidateCaches();
> > +}
> 
> It should be "extend" not "expand".

Done.

> 
> > WebCore/editing/TextCheckingHelper.cpp:98
> > +TextCheckingRange TextCheckingRange::expandToParagraph(PassRefPtr<Range> range)
> 
> Ditto.
Done.

> 
> > WebCore/editing/TextCheckingHelper.h:38
> > +class TextCheckingSpan {
> > +public:
> > +    explicit TextCheckingSpan(int start = 0, int length = 0)
> > +        : m_start(start)
> > +        , m_length(length)
> > +    {}
> 
> I'm not sure "span" is a good word for this but then I can't come up with a better one.
I have same feeling.
Range and the Span is essentially same concept. 
Span is something to text that Range is to DOM tree.
I think currently we have no name for that concept and span is a reasonable candidate.

[Kent Tamura]

Comment on attachment 73458 [details]
Patch

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

> WebCore/editing/TextCheckingHelper.cpp:-49
> -PassRefPtr<Range> TextCheckingHelper::paragraphAlignedRange(int& offsetIntoParagraphAlignedRange, String& paragraphString) const

Is this unused?

> WebCore/editing/TextCheckingHelper.h:38
> +    {}

We usually use "{ }" (a space between {}) for an empty function.

> WebCore/editing/TextCheckingHelper.h:58
> +// A class which caches Range related computation result for 
> +// text checking.

We don't need to break the line.

[Hajime Morrita]

Created attachment 73700 [details]
Patch

[Hajime Morrita]

Hi Kent-san, thank you for taking a look!
I update the patch.

> > WebCore/editing/TextCheckingHelper.cpp:-49
> > -PassRefPtr<Range> TextCheckingHelper::paragraphAlignedRange(int& offsetIntoParagraphAlignedRange, String& paragraphString) const
> 
> Is this unused?
Yes. This patch replaced it with TextCheckingRange.

> 
> > WebCore/editing/TextCheckingHelper.h:38
> > +    {}
> 
> We usually use "{ }" (a space between {}) for an empty function.
Fixed.

> 
> > WebCore/editing/TextCheckingHelper.h:58
> > +// A class which caches Range related computation result for 
> > +// text checking.
> 
> We don't need to break the line.
Fixed.

[Kent Tamura]

(In reply to comment #17)
> Hi Kent-san, thank you for taking a look!
> I update the patch.
> 
> > > WebCore/editing/TextCheckingHelper.cpp:-49
> > > -PassRefPtr<Range> TextCheckingHelper::paragraphAlignedRange(int& offsetIntoParagraphAlignedRange, String& paragraphString) const
> > 
> > Is this unused?
> Yes. This patch replaced it with TextCheckingRange.

I wondered where #if in the old code went to.

[Kent Tamura]

(In reply to comment #18)
> (In reply to comment #17)
> > Hi Kent-san, thank you for taking a look!
> > I update the patch.
> > 
> > > > WebCore/editing/TextCheckingHelper.cpp:-49
> > > > -PassRefPtr<Range> TextCheckingHelper::paragraphAlignedRange(int& offsetIntoParagraphAlignedRange, String& paragraphString) const
> > > 
> > > Is this unused?
> > Yes. This patch replaced it with TextCheckingRange.
> 
> I wondered where #if in the old code went to.

I meant "#ifndef BUILDING_ON_TIGER"

[Kent Tamura]

Comment on attachment 73700 [details]
Patch

looks ok.

[Hajime Morrita]

Committed r71898: <http://trac.webkit.org/changeset/71898>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71898 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
platform/gtk/fonts/custom-font-missing-glyphs.html

[Jia Pu]

Comment on attachment 73700 [details]
Patch

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

> WebCore/editing/Editor.cpp:2344
> +                        spellingSpan.setLength(spellingSpan.length() + (replacementLength - resultSpan.length()));

Hmmm, I don't see this line being committed in <http://trac.webkit.org/changeset/71898>. 

I noticed it while working on another bug in this area.

[Jia Pu]

(In reply to comment #23)
> (From update of attachment 73700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73700&action=review
> 
> > WebCore/editing/Editor.cpp:2344
> > +                        spellingSpan.setLength(spellingSpan.length() + (replacementLength - resultSpan.length()));
> 
> Hmmm, I don't see this line being committed in <http://trac.webkit.org/changeset/71898>. 
> 
> I noticed it while working on another bug in this area.

Ah, never mind, I saw the update of spellingRangeEndOffset at the beginning of the for loop. Although how comes the change set is different from the last patch?

[Hajime Morrita]

Hi Jian, thank you for taking look at this!
I should have been waiting for your take before land...

I rewrite the change to use 2 classes: TextCheckingRange and TextCheckingSpan.
So the code might look much different from the last one.
It turns out some part where the last patch couldn't touch now can be replaced with one of these.

I think the most tricky part is in span comparison, like:
"if (resultLocation + detail->location >= grammarRangeStartOffset && resultLocation + detail->location + detail->length <= grammarRangeEndOffset)"
I replaced them using TextCheckingSpan. 
It would be great if you could check around such type of conditionals when you have time,
and cc me when you find any problem.

BTW, one of the goal of the series of refactoring in my mind is splitting grammar checking 
path and spell-checking path, to allow grammar-checking to be (optionally) asynchronous.
I don't have clear idea yet, though.

Regards.


(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 73700 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=73700&action=review
> > 
> > > WebCore/editing/Editor.cpp:2344
> > > +                        spellingSpan.setLength(spellingSpan.length() + (replacementLength - resultSpan.length()));
> > 
> > Hmmm, I don't see this line being committed in <http://trac.webkit.org/changeset/71898>. 
> > 
> > I noticed it while working on another bug in this area.
> 
> Ah, never mind, I saw the update of spellingRangeEndOffset at the beginning of the for loop. Although how comes the change set is different from the last patch?