[SelectMisspellingAsync #1] Move SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

Move SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

This change improves encapsulation of the spell check code and helps hide
implementation details from ContextMenuClient.cpp.

BUG=

Review-Url: https://codereview.chromium.org/2966223003
Cr-Commit-Position: refs/heads/master@{#489134}
Committed: https://chromium.googlesource.com/chromium/src/+/91df98d36219b24cb8897d1c8aaf31503f8400be

[rlanday, 2017-07-05 19:20:20]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 19:20:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-07-05 19:20:56]

rlanday@chromium.org changed reviewers:
+ xiaochengh@chromium.org, yosin@chromium.org

[commit-bot: I haz the power, 2017-07-05 19:50:04]

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

[commit-bot: I haz the power, 2017-07-05 19:50:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[rlanday, 2017-07-11 20:02:48]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 20:03:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-12 01:01:34]

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

[commit-bot: I haz the power, 2017-07-12 01:01:37]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-07-12 05:11:49]

It is better make this patch independent from other patches since this patch can
be committed
w/o others.

https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h (right):

https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:80: String
SelectMisspellingAsync(String& description);
Could you return std::pair<String, String> to avoid out parameter?

[rlanday, 2017-07-12 19:02:39]

On 2017/07/12 at 05:11:49, yosin wrote:
> It is better make this patch independent from other patches since this patch
can be committed
> w/o others.

Ok, I've done that.

>
https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h (right):
> 
>
https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:80: String
SelectMisspellingAsync(String& description);
> Could you return std::pair<String, String> to avoid out parameter?

I will do this in a separate CL as this involves updating callers.

[rlanday, 2017-07-12 19:02:41]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-12 19:03:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-07-12 19:56:01]

On 2017/07/12 at 19:02:39, rlanday wrote:
> On 2017/07/12 at 05:11:49, yosin wrote:
> > It is better make this patch independent from other patches since this patch
can be committed
> > w/o others.
> 
> Ok, I've done that.
> 
> >
https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h
(right):
> > 
> >
https://codereview.chromium.org/2966223003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:80: String
SelectMisspellingAsync(String& description);
> > Could you return std::pair<String, String> to avoid out parameter?
> 
> I will do this in a separate CL as this involves updating callers.

Uploaded here: https://codereview.chromium.org/2978823002

[commit-bot: I haz the power, 2017-07-12 22:30:54]

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

[commit-bot: I haz the power, 2017-07-12 22:30:55]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-07-20 00:56:32]

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:912:
VisibleSelection selection =
nit: s/VisbileSelection/const VisbileSelection/

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952: Range*
const marker_range =
We should avoid to use temporary Range object for Oilpan friendly.
We can use PlainText() with EphemeralRange here.

String Range::GetText() const {
  DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
  return PlainText(EphemeralRange(this),
                   TextIteratorBehavior::Builder()
                       .SetEmitsObjectReplacementCharacter(true)
                       .Build());
}

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963: return
marker_range->GetText();
Please hold restul of Range::GetText() to avoid calling PlainText() twice.

[rlanday, 2017-07-20 20:49:23]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-07-20 20:49:31]

On 2017/07/20 at 00:56:32, yosin wrote:
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
(right):
> 
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:912:
VisibleSelection selection =
> nit: s/VisbileSelection/const VisbileSelection/
> 
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952: Range*
const marker_range =
> We should avoid to use temporary Range object for Oilpan friendly.
> We can use PlainText() with EphemeralRange here.
> 
> String Range::GetText() const {
>   DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
>   return PlainText(EphemeralRange(this),
>                    TextIteratorBehavior::Builder()
>                        .SetEmitsObjectReplacementCharacter(true)
>                        .Build());
> }
> 
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963: return
marker_range->GetText();
> Please hold restul of Range::GetText() to avoid calling PlainText() twice.

Updated

[commit-bot: I haz the power, 2017-07-20 20:49:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-07-20 21:22:18]

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right):

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952: Range*
const marker_range =
On 2017/07/20 at 00:56:32, yosin_UTC9 wrote:
> We should avoid to use temporary Range object for Oilpan friendly.
> We can use PlainText() with EphemeralRange here.
> 
> String Range::GetText() const {
>   DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
>   return PlainText(EphemeralRange(this),
>                    TextIteratorBehavior::Builder()
>                        .SetEmitsObjectReplacementCharacter(true)
>                        .Build());
> }

Shouldn't we do this change in a separate CL?

Doing moving and refactoring in the same CL makes it harder to track code
changes.

https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963: return
marker_range->GetText();
On 2017/07/20 at 00:56:31, yosin_UTC9 wrote:
> Please hold restul of Range::GetText() to avoid calling PlainText() twice.

Ditto.

[rlanday, 2017-07-20 21:23:26]

On 2017/07/20 at 21:22:18, xiaochengh wrote:
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
(right):
> 
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952: Range*
const marker_range =
> On 2017/07/20 at 00:56:32, yosin_UTC9 wrote:
> > We should avoid to use temporary Range object for Oilpan friendly.
> > We can use PlainText() with EphemeralRange here.
> > 
> > String Range::GetText() const {
> >   DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
> >   return PlainText(EphemeralRange(this),
> >                    TextIteratorBehavior::Builder()
> >                        .SetEmitsObjectReplacementCharacter(true)
> >                        .Build());
> > }
> 
> Shouldn't we do this change in a separate CL?
> 
> Doing moving and refactoring in the same CL makes it harder to track code
changes.
> 
>
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963: return
marker_range->GetText();
> On 2017/07/20 at 00:56:31, yosin_UTC9 wrote:
> > Please hold restul of Range::GetText() to avoid calling PlainText() twice.
> 
> Ditto.

Maybe we can make these changes in https://codereview.chromium.org/2959553002,
where I'm already refactoring the method?

[Xiaocheng, 2017-07-20 21:45:11]

On 2017/07/20 at 21:23:26, rlanday wrote:
> On 2017/07/20 at 21:22:18, xiaochengh wrote:
> >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
(right):
> > 
> >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952:
Range* const marker_range =
> > On 2017/07/20 at 00:56:32, yosin_UTC9 wrote:
> > > We should avoid to use temporary Range object for Oilpan friendly.
> > > We can use PlainText() with EphemeralRange here.
> > > 
> > > String Range::GetText() const {
> > >   DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
> > >   return PlainText(EphemeralRange(this),
> > >                    TextIteratorBehavior::Builder()
> > >                        .SetEmitsObjectReplacementCharacter(true)
> > >                        .Build());
> > > }
> > 
> > Shouldn't we do this change in a separate CL?
> > 
> > Doing moving and refactoring in the same CL makes it harder to track code
changes.
> > 
> >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963:
return marker_range->GetText();
> > On 2017/07/20 at 00:56:31, yosin_UTC9 wrote:
> > > Please hold restul of Range::GetText() to avoid calling PlainText() twice.
> > 
> > Ditto.
> 
> Maybe we can make these changes in https://codereview.chromium.org/2959553002,
where I'm already refactoring the method?

sgtm.

[rlanday, 2017-07-20 22:04:00]

On 2017/07/20 at 21:45:11, xiaochengh wrote:
> On 2017/07/20 at 21:23:26, rlanday wrote:
> > On 2017/07/20 at 21:22:18, xiaochengh wrote:
> > >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:952:
Range* const marker_range =
> > > On 2017/07/20 at 00:56:32, yosin_UTC9 wrote:
> > > > We should avoid to use temporary Range object for Oilpan friendly.
> > > > We can use PlainText() with EphemeralRange here.
> > > > 
> > > > String Range::GetText() const {
> > > >   DCHECK(!owner_document_->NeedsLayoutTreeUpdate());
> > > >   return PlainText(EphemeralRange(this),
> > > >                    TextIteratorBehavior::Builder()
> > > >                        .SetEmitsObjectReplacementCharacter(true)
> > > >                        .Build());
> > > > }
> > > 
> > > Shouldn't we do this change in a separate CL?
> > > 
> > > Doing moving and refactoring in the same CL makes it harder to track code
changes.
> > > 
> > >
https://codereview.chromium.org/2966223003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:963:
return marker_range->GetText();
> > > On 2017/07/20 at 00:56:31, yosin_UTC9 wrote:
> > > > Please hold restul of Range::GetText() to avoid calling PlainText()
twice.
> > > 
> > > Ditto.
> > 
> > Maybe we can make these changes in
https://codereview.chromium.org/2959553002, where I'm already refactoring the
method?
> 
> sgtm.

Ok, I've undone the changes requested by yosin@, I will make sure they're
applied in that other CL.

[rlanday, 2017-07-20 22:04:02]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-20 22:04:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-20 23:22:19]

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

[commit-bot: I haz the power, 2017-07-20 23:22:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[rlanday, 2017-07-21 00:05:54]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-21 00:06:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yosin_UTC9, 2017-07-21 00:57:19]

lgtm

OK, we'll refactor SelectMisspellingAsync() in following patch.

For me it is OK that moving and refactoring in one patch for this size of
function.
Since, this change looks like new code.

If I could do this:
1. Move SelectMisspllingAsync() to SpellChecker in-place.
  static String SelectMisspellingAsync(...)
=>
   String SpellChecker::SelectMIsspellingAsync()
2. Move it to SpellChecker.cpp
3. Refactor it 

In this way, we can easy to blame when refactoring causes issue.

[rlanday, 2017-07-21 02:05:36]

On 2017/07/21 at 00:57:19, yosin wrote:
> lgtm
> 
> OK, we'll refactor SelectMisspellingAsync() in following patch.
> 
> For me it is OK that moving and refactoring in one patch for this size of
function.
> Since, this change looks like new code.
> 
> If I could do this:
> 1. Move SelectMisspllingAsync() to SpellChecker in-place.
>   static String SelectMisspellingAsync(...)
> =>
>    String SpellChecker::SelectMIsspellingAsync()
> 2. Move it to SpellChecker.cpp
> 3. Refactor it 
> 
> In this way, we can easy to blame when refactoring causes issue.

Wait, so do you want me to land this as-is, or split it up further?

[commit-bot: I haz the power, 2017-07-21 02:06:47]

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

[commit-bot: I haz the power, 2017-07-21 02:06:48]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-07-21 03:50:59]

On 2017/07/21 at 02:05:36, rlanday wrote:
> On 2017/07/21 at 00:57:19, yosin wrote:
> > lgtm
> > 
> > OK, we'll refactor SelectMisspellingAsync() in following patch.
> > 
> > For me it is OK that moving and refactoring in one patch for this size of
function.
> > Since, this change looks like new code.
> > 
> > If I could do this:
> > 1. Move SelectMisspllingAsync() to SpellChecker in-place.
> >   static String SelectMisspellingAsync(...)
> > =>
> >    String SpellChecker::SelectMIsspellingAsync()
> > 2. Move it to SpellChecker.cpp
> > 3. Refactor it 
> > 
> > In this way, we can easy to blame when refactoring causes issue.
> 
> Wait, so do you want me to land this as-is, or split it up further?

Sorry for confusion, it is OK to land as-is.
Your patch does step 1 and step 2 same time.
Also, I know this function well and review as new function, this why I asked to
refactor.

Moving code around looks like many changes. 
There are no guarantee that no changes around moving. The description just says
so.

When we use two steps moves, changes are
Step 1
 - One line change of SelectMisspellingAsync()
 - One line change of ContextMenuClient::ShowContextMenu()
 - One line change of SpellChecker.h

Step 2
 - Move IsWhiteSpaceOrPunctuation()
 - Move SelectMisspellingAsync()

So, combining step 1 and step 2 differs just three lines.
This is why I think combining step 1 and step 2 is OK.

[rlanday, 2017-07-24 18:04:33]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[rlanday, 2017-07-24 18:04:42]

The CQ bit was unchecked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-07-24 18:04:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rlanday, 2017-07-24 18:04:45]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-07-24 18:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2966223003/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-07-24 18:04:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 18:14:33]

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

[commit-bot: I haz the power, 2017-07-24 18:14:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rlanday, 2017-07-24 18:29:36]

rlanday@chromium.org changed reviewers:
+ tkent@chromium.org

[rlanday, 2017-07-24 18:29:43]

Adding tkent@ for OWNER review of
third_party/WebKit/Source/core/page/ContextMenuClient.cpp

[tkent, 2017-07-24 23:14:59]

lgtm

[rlanday, 2017-07-24 23:15:34]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-07-24 23:15:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 23:19:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1500938134586050,
"parent_rev": "ef6bcaea5200c2221a93f375a4d66973b0b73c52", "commit_rev":
"91df98d36219b24cb8897d1c8aaf31503f8400be"}

[commit-bot: I haz the power, 2017-07-24 23:20:01]

Description was changed from

==========
Move SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

This change improves encapsulation of the spell check code and helps hide
implementation details from ContextMenuClient.cpp.

BUG=
==========

to

==========
Move SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

This change improves encapsulation of the spell check code and helps hide
implementation details from ContextMenuClient.cpp.

BUG=

Review-Url: https://codereview.chromium.org/2966223003
Cr-Commit-Position: refs/heads/master@{#489134}
Committed:
https://chromium.googlesource.com/chromium/src/+/91df98d36219b24cb8897d1c8aaf...
==========

[commit-bot: I haz the power, 2017-07-24 23:20:02]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/91df98d36219b24cb8897d1c8aaf...