[MarkersIntersectingRange #4] Refactor ContextMenuClient::SelectMisspellingAsync()

Refactor ContextMenuClient::SelectMisspellingAsync()

Currently, ContextMenuClient::SelectMisspellingAsync() duplicates logic that we
also implement in SpellChecker::GetSpellCheckMarkerUnderSelection(). This CL
implements SelectMisspellingAsync() on top of
GetSpellCheckMarkerUnderSelection() to reduce code duplication.

BUG=736181
Review-Url: https://codereview.chromium.org/2959553002
Cr-Commit-Position: refs/heads/master@{#489417}
Committed: https://chromium.googlesource.com/chromium/src/+/76cf4f1fd0db79aeb3c6c08d92afcae00b0cd08e

[rlanday, 2017-06-23 23:06:10]

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

[rlanday, 2017-06-23 23:08:29]

See
https://docs.google.com/document/d/1znHQwY6KUongMRwytBh9RvLJENjz_HnUfunx_uFt5QI
for an overview of the MarkersIntersectingRange series of patches

https://codereview.chromium.org/2959553002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/ContextMenuClient.cpp (right):

https://codereview.chromium.org/2959553002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/ContextMenuClient.cpp:132: if
(marker_range->GetText().StripWhiteSpace(&IsWhiteSpaceOrPunctuation) !=
I'm not sure what's going on here (referring to comparing the text in the
selection to the text in the marker, not to the code review tool duplicating the
two lines and making it look like something's being compared to itself), it
would be nice if we could avoid getting the selection outside of
GetSpellCheckMarkerUnderSelection().

[rlanday, 2017-06-26 21:07:47]

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

[commit-bot: I haz the power, 2017-06-26 21:08:09]

Dry run: CQ is trying da patch.

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

[Xiaocheng, 2017-06-26 22:51:29]

Code change seems good.

I'll wait for yosin's review of the earlier patches in this series before moving
on.

[commit-bot: I haz the power, 2017-06-26 23:25:19]

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

[commit-bot: I haz the power, 2017-06-26 23:25:20]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-06-27 01:23:53]

Could you move SelectMisspellingAsync() to SpellChecker?
We would like to put all spell checker functionality in SpellChecker class to
hide implementation
details.

Let's move spellchecker changes for context menu into another series of patch
and isolate from
Text Suggestions integration.

[rlanday, 2017-07-05 19:06:40]

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:07:01]

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:21:16]

On 2017/06/27 at 01:23:53, yosin wrote:
> Could you move SelectMisspellingAsync() to SpellChecker?
> We would like to put all spell checker functionality in SpellChecker class to
hide implementation
> details.
> 
> Let's move spellchecker changes for context menu into another series of patch
and isolate from
> Text Suggestions integration.

Ok, I've done this here:
https://codereview.chromium.org/2966223003

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

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

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

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 19:55:06]

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

[commit-bot: I haz the power, 2017-07-11 19:55:54]

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-11 20:56:37]

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

[commit-bot: I haz the power, 2017-07-11 20:56:38]

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-13 00:00:04]

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

[commit-bot: I haz the power, 2017-07-13 00:00:22]

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-13 05:29:32]

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

[commit-bot: I haz the power, 2017-07-13 05:29:33]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-07-20 01:19:07]

Is this patch obsolete?
It seems the patch[1] supersedes this patch.

[1] http://crrev.com//2966223003 [SelectMisspellingAsync #1] Move
SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

https://codereview.chromium.org/2959553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ContextMenuClient.cpp (right):

https://codereview.chromium.org/2959553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ContextMenuClient.cpp:114: if
(!node_and_marker)
How about using |pair.first| which is Node*, as no spelling marker?
If so, we could write:

if (!node_and_mark.first)
  return;

https://codereview.chromium.org/2959553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ContextMenuClient.cpp:122:
Range::Create(*selected_frame->GetDocument(), marker_node,
Could you avoid to use temporary Range make this code Oilpan friendly?
We can use EpehemralRange and PlainText() to work as Range::GetTExt()

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

[rlanday, 2017-07-21 00:14:35]

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:14:53]

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-21 00:15:18]

On 2017/07/20 at 01:19:07, yosin wrote:
> Is this patch obsolete?
> It seems the patch[1] supersedes this patch.
> 
> [1] http://crrev.com//2966223003 [SelectMisspellingAsync #1] Move
SelectMisspellingAsync() from ContextMenuClient.cpp to SpellChecker

This patch refactors the method, that one moves it to another class.

I've updated this CL, taking into account the feedback from that other CL.

[rlanday, 2017-07-21 00:16:04]

https://codereview.chromium.org/2959553002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ContextMenuClient.cpp (right):

https://codereview.chromium.org/2959553002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ContextMenuClient.cpp:114: if
(!node_and_marker)
On 2017/07/20 at 01:19:07, yosin_UTC9 wrote:
> How about using |pair.first| which is Node*, as no spelling marker?
> If so, we could write:
> 
> if (!node_and_mark.first)
>   return;

I put up a separate CL for this:
https://chromium-review.googlesource.com/c/580688/

[yosin_UTC9, 2017-07-21 01:03:48]

lgtm w/ nit

https://codereview.chromium.org/2959553002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/ContextMenuClient.cpp (right):

https://codereview.chromium.org/2959553002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/ContextMenuClient.cpp:121: VisibleSelection
selection =
nit: s/VisibleSelection/const VisibleSelection/

[commit-bot: I haz the power, 2017-07-21 01:22:26]

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

[commit-bot: I haz the power, 2017-07-21 01:22:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rlanday, 2017-07-25 17:58:08]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-07-25 17:58:09]

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/2959553002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-07-25 17:58:23]

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-25 20:41:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501005488927800,
"parent_rev": "c94bfd85af86cc4483b9d9d659a488f5e911e4c0", "commit_rev":
"76cf4f1fd0db79aeb3c6c08d92afcae00b0cd08e"}

[commit-bot: I haz the power, 2017-07-25 20:41:58]

Description was changed from

==========
Refactor ContextMenuClient::SelectMisspellingAsync()

Currently, ContextMenuClient::SelectMisspellingAsync() duplicates logic that we
also implement in SpellChecker::GetSpellCheckMarkerUnderSelection(). This CL
implements SelectMisspellingAsync() on top of
GetSpellCheckMarkerUnderSelection() to reduce code duplication.

BUG=736181
==========

to

==========
Refactor ContextMenuClient::SelectMisspellingAsync()

Currently, ContextMenuClient::SelectMisspellingAsync() duplicates logic that we
also implement in SpellChecker::GetSpellCheckMarkerUnderSelection(). This CL
implements SelectMisspellingAsync() on top of
GetSpellCheckMarkerUnderSelection() to reduce code duplication.

BUG=736181

Review-Url: https://codereview.chromium.org/2959553002
Cr-Commit-Position: refs/heads/master@{#489417}
Committed:
https://chromium.googlesource.com/chromium/src/+/76cf4f1fd0db79aeb3c6c08d92af...
==========

[commit-bot: I haz the power, 2017-07-25 20:42:00]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/76cf4f1fd0db79aeb3c6c08d92af...