Re: Fix the range returned by findFirstMisspelling (issue 908303002 by sbaig1@bloomberg.net)

[yo...@chromium.org]

Sorry for super late response. m(_ _)m


https://codereview.chromium.org/908303002/diff/1/Source/core/editing/TextCheckingHelper.cpp
File Source/core/editing/TextCheckingHelper.cpp (right):

https://codereview.chromium.org/908303002/diff/1/Source/core/editing/TextCheckingHelper.cpp#newcode272
Source/core/editing/TextCheckingHelper.cpp:272: firstMisspellingRange =
Range::create(misspellingStart.containerNode()->document(),
misspellingStart, misspellingEnd);
It seems current code is correct, since |misspellingStart == m_start|
and |misspellingEnd == m_end|.

https://codereview.chromium.org/908303002/

[sba...@bloomberg.net]

It does look like that at first, doesn't it? :)

It took me a while to figure out, but they are actually not the same because
|misspellingStart| and |misspellingEnd| are modified by the
|TextIterator::subrange| function.


https://codereview.chromium.org/908303002/

[yo...@chromium.org]

Good catch! It is difficult to know that. Using non-const reference isn't
good.
We should use pointer.

Could you write test script for your change?
I think DocumentMarkerControllerTest.cpp is the best place.

Thanks!


https://codereview.chromium.org/908303002/

[sba...@bloomberg.net]

On 2015/05/22 01:32:48, Yosi_UTC9 wrote:
> Could you write test script for your change?
> I think DocumentMarkerControllerTest.cpp is the best place.

Hmm, I looked at DocumentMarkerControllerTest.cpp but not sure what's the
best
way to test this there. The TextCheckingHelper needs a SpellCheckerClient,
and
looks like the DummyPageHolder inside DocumentMarkerControllerTest.cpp
doesn't
create a SpellCheckerClient.

I noticed that HTMLTextFormControlElementTest.cpp does have a
DummySpellCheckerClient.

So I wonder if HTMLTextFormControlElementTest.cpp would be a better place?
Or
do you think we should add a DummySpellCheckerClient to
DocumentMarkerControllerTest.cpp?


https://codereview.chromium.org/908303002/

[yo...@chromium.org]

On 2015/05/22 11:51:05, Shezan Baig (Bloomberg) wrote:
> So I wonder if HTMLTextFormControlElementTest.cpp would be a better
> place? Or
> do you think we should add a DummySpellCheckerClient to
> DocumentMarkerControllerTest.cpp?

I prefer DocumentMarkerControllerTest.cpp to have test script in same
directory.


https://codereview.chromium.org/908303002/