WIP: prefer using EphemeralRange for SpellChecker.

WIP: prefer using EphemeralRange for SpellChecker.

R=
BUG=

[sof, 2015-09-09 14:51:12]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-09 14:51:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1329253002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1329253002/20001

[commit-bot: I haz the power, 2015-09-09 15:58:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 15:58:09]

Dry run: This issue passed the CQ dry run.

[haraken, 2015-09-09 23:48:11]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, yosin@chromium.org

[haraken, 2015-09-09 23:48:11]

LGTM if yosin is fine with this.

Let's add more rationale of this change to the CL description.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.c...
Source/core/dom/Range.cpp:140: #if ENABLE(OILPAN)

Add a comment about why we want to call detachRange in oilpan.

[yosin_UTC9, 2015-09-10 01:58:40]

Could you split this patch into three?

1. Introduce firstEphemeralRange() and replace usages
2. Change TextChekingParagraph class to use EphemeralRange
3. Range::dispose() // although, I don't agree to introduce
 - introducing TempRangeScope is here: http://crrev.com/330383004/

Thanks!

P.S.
Tracking bug for temporary Range is http://crbug.com/388681

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.h...
Source/core/dom/Range.h:67: void dispose();
I'm still in double to introduce |Range::dispose()|. Or, it is too early
to introduce. For spell checker, we don't need to use |dispose()|, since
we can replace all of |Range| usage to |EphmeralRange|.

If we really need to have |Range::dispose()|, let's remove "noop" mark
from |Range:detach()| and use |detach()|.

User script may register "non-needed" |Range| objects to |Document| too.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/Vis...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.cpp:226: if (selection.isNone())
nit: Could you use |firstEphemeralRange()| here?

return createRange(firstEphemeralRangeOf(seleciton));

and add TODO to replace |firstRangeOf()| to above pattern.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:520: EphemeralRange checkRange =
fullParagraphToCheck.checkingRange();
nit: |const EphemeralRange|.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:527: checkRange =
fullParagraphToCheck.subrange(currentChunkStart, kChunkSize);
nit: Let's use new variable name. We don't want to reuse |checkRange|.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:624: EphemeralRange
selectionRange = extendedParagraph.subrange(0, selectionOffset);
nit: |const EphemeralRange|

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
Let's avoid to use |Range|.
This could be:

return TextItertaor::rangeLength(m_offsetAsRange.startPosition(), position);

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.h (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.h:75: mutable EphemeralRange
m_paragraphRange;
Good catch!

Since, |EphmeralRange| checks DOM tree version, we can detect miss use
of |TextCheckingParagraph|, e.g. mutate DOM tree during
|TextCheckingParagaph| instances alive.

https://codereview.chromium.org/1329253002/diff/40001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:270: range->dispose();
Please introduce |ScopedRangeObject|, or better name, to avoid explicit call
of |Range::dispose()|.

[yosin_UTC9, 2015-09-10 02:24:02]

We also want to rename |VisibleSelection::toNormalizedEphemeralRange()|
to |toNormalizedRange()|.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/Vis...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.cpp:226: if (selection.isNone())
On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> nit: Could you use |firstEphemeralRange()| here?
> 
> return createRange(firstEphemeralRangeOf(seleciton));
> 
> and add TODO to replace |firstRangeOf()| to above pattern.

It is better to remove |firstRangeOf()| by replacing it to
|createRange(firstEphemeralRange(selection))

There are 30 call sites of |firstRangeOf()|, but 18 sites are
in test.

[sof, 2015-09-10 05:09:57]

ok, thanks. i'll spend a day plus getting this into the desired shape.

[sof, 2015-09-10 05:59:02]

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.h
File Source/core/dom/Range.h (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/dom/Range.h...
Source/core/dom/Range.h:67: void dispose();
On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> I'm still in double to introduce |Range::dispose()|. Or, it is too early
> to introduce. For spell checker, we don't need to use |dispose()|, since
> we can replace all of |Range| usage to |EphmeralRange|.
> 
> If we really need to have |Range::dispose()|, let's remove "noop" mark
> from |Range:detach()| and use |detach()|.
> 
> User script may register "non-needed" |Range| objects to |Document| too.

Please see 


https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/bXesw9EnP3Q/Nd7...

for motivation on why dispose() is needed. Rewriting all uses of Range to be
EphemeralRange, if that's possible in the face of tree mutations, isn't a
practical option at this stage.

dispose() is our preferred naming for these operations.

[sof, 2015-09-10 06:07:49]

https://codereview.chromium.org/1329253002/diff/40001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:270: range->dispose();
On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> Please introduce |ScopedRangeObject|, or better name, to avoid explicit call
> of |Range::dispose()|.

We have ScopedDisposal<T>, which is arguably more general. Switched.

[sof, 2015-09-10 06:12:11]

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> Let's avoid to use |Range|.
> This could be:
> 
> return TextItertaor::rangeLength(m_offsetAsRange.startPosition(), position);

Don't we need to account for the exception-raising possibilities of
Range::setEnd() ?

[yosin_UTC9, 2015-09-10 06:37:04]

Still WIP?

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
On 2015/09/10 06:12:10, sof wrote:
> On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> > Let's avoid to use |Range|.
> > This could be:
> > 
> > return TextItertaor::rangeLength(m_offsetAsRange.startPosition(), position);
> 
> Don't we need to account for the exception-raising possibilities of
> Range::setEnd() ?

Since caller is SpellChecker::markAndReplaceFor()
  selectionOffset = paragraph.offsetTo(caretPosition, ASSERT_NO_EXCEPTION);

Range::setEnd() should not raise an exception.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Inp...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:362: EphemeralRange range =
firstEphemeralRangeOf(frame().selection().selection());
nit: |const EphmeralRange|

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Vis...
File Source/core/editing/VisibleSelection.cpp (right):

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.cpp:230: return
Range::create(*start.document(), start, end);
Please change to use |firstEphemeralRangeOf()|.
We don't want to have duplicated code.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.cpp:237: Position start =
selection.start().parentAnchoredEquivalent();
nit: |const Position|.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/Vis...
Source/core/editing/VisibleSelection.cpp:238: Position end =
selection.end().parentAnchoredEquivalent();
nit: |const Position|.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
File Source/core/editing/spellcheck/SpellCheckRequester.cpp (right):

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellCheckRequester.cpp:254: // This assumes that
m_processingRequest's Ranges aren't shared.
It is better to move |dispose()| call to |clear()|.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
File Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:520: EphemeralRange checkRange =
fullParagraphToCheck.checkingRange();
nit: |const EphemeralRange|.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:528: checkRange =
expandRangeToSentenceBoundary(checkRange);
nit: Please use another local variable here instead of reusing |checkRange|

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:619: if (selectionChanged) {
Optional: It is better to use early-return style.

https://codereview.chromium.org/1329253002/diff/60001/Source/core/editing/spe...
Source/core/editing/spellcheck/SpellChecker.cpp:624: EphemeralRange
selectionRange = extendedParagraph.subrange(0, selectionOffset);
nit: |const EphemeralRange|

https://codereview.chromium.org/1329253002/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1329253002/diff/60001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:2359: EphemeralRange firstRange =
firstEphemeralRangeOf(selection.selection());
nit: |const EphemeralRange|

[sof, 2015-09-10 06:42:27]

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
On 2015/09/10 06:37:03, Yosi_UTC9 wrote:
> On 2015/09/10 06:12:10, sof wrote:
> > On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> > > Let's avoid to use |Range|.
> > > This could be:
> > > 
> > > return TextItertaor::rangeLength(m_offsetAsRange.startPosition(),
position);
> > 
> > Don't we need to account for the exception-raising possibilities of
> > Range::setEnd() ?
> 
> Since caller is SpellChecker::markAndReplaceFor()
>   selectionOffset = paragraph.offsetTo(caretPosition, ASSERT_NO_EXCEPTION);
> 
> Range::setEnd() should not raise an exception.

But this is a public function taking an ExceptionState&, so how can you insist
on that assumption? Do you want to drop that extra arg?

[yosin_UTC9, 2015-09-10 07:01:46]

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
On 2015/09/10 06:42:27, sof wrote:
> On 2015/09/10 06:37:03, Yosi_UTC9 wrote:
> > On 2015/09/10 06:12:10, sof wrote:
> > > On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> > > > Let's avoid to use |Range|.
> > > > This could be:
> > > > 
> > > > return TextItertaor::rangeLength(m_offsetAsRange.startPosition(),
> position);
> > > 
> > > Don't we need to account for the exception-raising possibilities of
> > > Range::setEnd() ?
> > 
> > Since caller is SpellChecker::markAndReplaceFor()
> >   selectionOffset = paragraph.offsetTo(caretPosition, ASSERT_NO_EXCEPTION);
> > 
> > Range::setEnd() should not raise an exception.
> 
> But this is a public function taking an ExceptionState&, so how can you insist
> on that assumption? Do you want to drop that extra arg?

Yes, please. We don't want to have V8 thing in spell check module.

[yosin_UTC9, 2015-09-10 07:03:32]

On 2015/09/10 07:01:46, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
> File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):
> 
>
https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
> Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
> RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
> On 2015/09/10 06:42:27, sof wrote:
> > On 2015/09/10 06:37:03, Yosi_UTC9 wrote:
> > > On 2015/09/10 06:12:10, sof wrote:
> > > > On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> > > > > Let's avoid to use |Range|.
> > > > > This could be:
> > > > > 
> > > > > return TextItertaor::rangeLength(m_offsetAsRange.startPosition(),
> > position);
> > > > 
> > > > Don't we need to account for the exception-raising possibilities of
> > > > Range::setEnd() ?
> > > 
> > > Since caller is SpellChecker::markAndReplaceFor()
> > >   selectionOffset = paragraph.offsetTo(caretPosition,
ASSERT_NO_EXCEPTION);
> > > 
> > > Range::setEnd() should not raise an exception.
> > 
> > But this is a public function taking an ExceptionState&, so how can you
insist
> > on that assumption? Do you want to drop that extra arg?
> 
> Yes, please. We don't want to have V8 thing in spell check module.

We can also remove
  #include "bindings/core/v8/ExceptionState.h"
  #include "bindings/core/v8/ExceptionStatePlaceholder.h"
from TextCheckingHelper.cpp

[sof, 2015-09-10 07:16:27]

Just sorting through the issues raised here so as to know what to carry over to
the more focused CLs. Will do that next.

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
File Source/core/editing/spellcheck/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/1329253002/diff/40001/Source/core/editing/spe...
Source/core/editing/spellcheck/TextCheckingHelper.cpp:184:
RefPtrWillBeRawPtr<Range> range = createRange(offsetAsRange());
On 2015/09/10 07:01:46, Yosi_UTC9 wrote:
> On 2015/09/10 06:42:27, sof wrote:
> > On 2015/09/10 06:37:03, Yosi_UTC9 wrote:
> > > On 2015/09/10 06:12:10, sof wrote:
> > > > On 2015/09/10 01:58:39, Yosi_UTC9 wrote:
> > > > > Let's avoid to use |Range|.
> > > > > This could be:
> > > > > 
> > > > > return TextItertaor::rangeLength(m_offsetAsRange.startPosition(),
> > position);
> > > > 
> > > > Don't we need to account for the exception-raising possibilities of
> > > > Range::setEnd() ?
> > > 
> > > Since caller is SpellChecker::markAndReplaceFor()
> > >   selectionOffset = paragraph.offsetTo(caretPosition,
ASSERT_NO_EXCEPTION);
> > > 
> > > Range::setEnd() should not raise an exception.
> > 
> > But this is a public function taking an ExceptionState&, so how can you
insist
> > on that assumption? Do you want to drop that extra arg?
> 
> Yes, please. We don't want to have V8 thing in spell check module.

Good, let's simplify that away.

[sof, 2015-09-10 12:51:35]

The contents of this WIP has been split into:

 - https://codereview.chromium.org/1332823002/ (firstEphemeralRangeOf())
 - https://codereview.chromium.org/1331893002/ (use EphemeralRange where
possible with SpellChecker + utilities.)
 - https://codereview.chromium.org/1335573004/ (Range::dispose())

[sof, 2015-09-11 13:46:19]

Closing this initial bundle of changes; thanks for the reviews & suggested
improvements :)