Avoid creating duplicate Range objects when handling misspellings.

Avoid creating duplicate Range objects when handling misspellings.

When iterating over the input text and marking its misspellings, avoid
creating duplicate Range objects from the check & paragraph EphemeralRanges
if they're identical.

This addresses a performance regression on trunk (non-Oilpan) for
blink_perf.dom.textarea-edit introduced by the EphemeralRange-switching
https://codereview.chromium.org/1331893002

R=haraken
BUG=530740
Committed: https://crrev.com/70e3de94ab4b4a6ce2e9009f593f89aaf6eafdad
Cr-Commit-Position: refs/heads/master@{#351001}

[sof, 2015-09-25 12:34:07]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, yosin@chromium.org

[sof, 2015-09-25 12:34:08]

please take a look.

This addresses the non-Oilpan regression for textarea-edit. Oilpan performance
improves also, but not by much.

[haraken, 2015-09-25 12:34:50]

On 2015/09/25 12:34:08, sof wrote:
> please take a look.
> 
> This addresses the non-Oilpan regression for textarea-edit. Oilpan performance
> improves also, but not by much.

LGTM on my side.

[sof, 2015-09-25 12:38:38]

On 2015/09/25 12:34:50, haraken wrote:
> On 2015/09/25 12:34:08, sof wrote:
> > please take a look.
> > 
> > This addresses the non-Oilpan regression for textarea-edit. Oilpan
performance
> > improves also, but not by much.
> 
> LGTM on my side.

Thanks; I don't _think_ this is a controversial change to make, but for yosin@
to decide.

[yosin_UTC9, 2015-09-26 04:56:00]

Good catch!
We can make spell check cleaner.

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:520: const
EphemeralRange checkRange = fullParagraphToCheck.checkingRange();
nit: better in another patch.
Sorry, I forgot to add |const|.

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:526: const
EphemeralRange checkRange =
expandRangeToSentenceBoundary(fullParagraphToCheck.subrange(currentChunkStart,
kChunkSize));
nit: better in another patch.
Sorry, I forgot to add |const|.

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:542:
RefPtrWillBeRawPtr<SpellCheckRequest> request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
TextCheckingProcessBatch, checkRangeObject, paragraphRangeObject,
requestNumber);
Could you do this in |SpellCheckRequest::create()| instead of here?
It seems all call sites, just two!, |SpellCheckRequest::create()| can use
|EphemeralRange| as parameters:

1. HERE RefPtrWillBeRawPtr<SpellCheckRequest> request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
TextCheckingProcessBatch, createRange(checkRange), createRange(paragraphRange),
requestNumber);
2. ANOTHER 
    const EphemeralRange range = EphemeralRange::rangeOfContents(element);
    RefPtrWillBeRawPtr<Range> rangeToCheck = Range::create(element.document(),
range.startPosition(), range.endPosition());
m_spellCheckRequester->requestCheckingFor(SpellCheckRequest::create(TextCheckingTypeSpelling
| TextCheckingTypeGrammar, TextCheckingProcessBatch, rangeToCheck,
rangeToCheck));

So, we can make using |Range| object is an implementation detail of 
|SpellCheckRequest|.

[sof, 2015-09-26 06:59:44]

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:520: const
EphemeralRange checkRange = fullParagraphToCheck.checkingRange();
On 2015/09/26 04:56:00, Yosi_UTC9 wrote:
> nit: better in another patch.
> Sorry, I forgot to add |const|.

A minor tidyup in any case, but reverted it from this CL.

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:526: const
EphemeralRange checkRange =
expandRangeToSentenceBoundary(fullParagraphToCheck.subrange(currentChunkStart,
kChunkSize));
On 2015/09/26 04:55:59, Yosi_UTC9 wrote:
> nit: better in another patch.
> Sorry, I forgot to add |const|.

Same.

https://codereview.chromium.org/1369713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:542:
RefPtrWillBeRawPtr<SpellCheckRequest> request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
TextCheckingProcessBatch, checkRangeObject, paragraphRangeObject,
requestNumber);
On 2015/09/26 04:55:59, Yosi_UTC9 wrote:
> Could you do this in |SpellCheckRequest::create()| instead of here?
> It seems all call sites, just two!, |SpellCheckRequest::create()| can use
> |EphemeralRange| as parameters:
> 
> 1. HERE RefPtrWillBeRawPtr<SpellCheckRequest> request =
> SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
> TextCheckingProcessBatch, createRange(checkRange),
createRange(paragraphRange),
> requestNumber);
> 2. ANOTHER 
>     const EphemeralRange range = EphemeralRange::rangeOfContents(element);
>     RefPtrWillBeRawPtr<Range> rangeToCheck = Range::create(element.document(),
> range.startPosition(), range.endPosition());
>
m_spellCheckRequester->requestCheckingFor(SpellCheckRequest::create(TextCheckingTypeSpelling
> | TextCheckingTypeGrammar, TextCheckingProcessBatch, rangeToCheck,
> rangeToCheck));
> 
> So, we can make using |Range| object is an implementation detail of 
> |SpellCheckRequest|.

That's well worthwhile, pushed the Range conversion into there instead.

[yosin_UTC9, 2015-09-26 15:46:06]

Sorry for late response...

https://codereview.chromium.org/1369713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp
(right):

https://codereview.chromium.org/1369713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp:222:
RefPtrWillBeRawPtr<Range> checkingRange = m_processingRequest->checkingRange();
nit: Can we have |SpellCheckRequest::dispose()| or Make
|SpellCheckRequest::clear()| to dispose |checkingRange| and |paragraphRange|?
We may not want to check equality of |checkingRange| and |paragraphRange|
here. It is an implementation details.

[sof, 2015-09-26 16:18:30]

https://codereview.chromium.org/1369713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp
(right):

https://codereview.chromium.org/1369713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellCheckRequester.cpp:222:
RefPtrWillBeRawPtr<Range> checkingRange = m_processingRequest->checkingRange();
On 2015/09/26 15:46:06, Yosi_UTC9 wrote:
> nit: Can we have |SpellCheckRequest::dispose()| or Make
> |SpellCheckRequest::clear()| to dispose |checkingRange| and |paragraphRange|?
> We may not want to check equality of |checkingRange| and |paragraphRange|
> here. It is an implementation details.

Addressed.

[yosin_UTC9, 2015-09-26 16:56:35]

lgtm

Thanks for clean up!

[yosin_UTC9, 2015-09-26 16:56:40]

The CQ bit was checked by yosin@chromium.org

[yosin_UTC9, 2015-09-26 16:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1369713002/#ps60001
(title: "compile fix")

[commit-bot: I haz the power, 2015-09-26 16:56:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369713002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369713002/60001

[commit-bot: I haz the power, 2015-09-26 18:08:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-26 18:09:50]

Patchset 4 (id:??) landed as
https://crrev.com/70e3de94ab4b4a6ce2e9009f593f89aaf6eafdad
Cr-Commit-Position: refs/heads/master@{#351001}