Add DocumentMarker::kActiveSuggestion MarkerType

Add DocumentMarker::kActiveSuggestion MarkerType

This is a marker type I need to implement the Android spellcheck/suggestion
menu. When a misspelling or suggestion region is tapped, I want to highlight
the text the spellcheck or suggestion menu applies to. This is very similar in
functionality to composition markers, but I can't reuse that marker type because
we can have a composition underline active at the same time as a highlighted
suggestion region (and then we'd have no way of clearing one but not the other).

I'm refactoring most of CompositionMarker's functionality into a base class,
MarkerWithFormatting, that ActiveSuggestionMarker also inherits from.

BUG=672259, 715365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[rlanday, 2017-06-02 22:36:00]

Description was changed from

==========
Add DocumentMarker::kActiveSuggestion MarkerType

This is a marker type I need to implement the Android spellcheck/suggestion
menu. When a misspelling or suggestion region is tapped, I want to highlight
the text the spellcheck or suggestion menu applies to. This is very similar in
functionality to composition markers, but I can't reuse that marker type because
we can have a composition underline active at the same time as a highlighted
suggestion region (and then we'd have no way of clearing one but not the other).

BUG=672259,715365
==========

to

==========
Add DocumentMarker::kActiveSuggestion MarkerType

This is a marker type I need to implement the Android spellcheck/suggestion
menu. When a misspelling or suggestion region is tapped, I want to highlight
the text the spellcheck or suggestion menu applies to. This is very similar in
functionality to composition markers, but I can't reuse that marker type because
we can have a composition underline active at the same time as a highlighted
suggestion region (and then we'd have no way of clearing one but not the other).

BUG=672259,715365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rlanday, 2017-06-02 22:36:05]

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

[commit-bot: I haz the power, 2017-06-02 22:36:43]

Dry run: CQ is trying da patch.

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

[rlanday, 2017-06-02 22:36:48]

Description was changed from

==========
Add DocumentMarker::kActiveSuggestion MarkerType

This is a marker type I need to implement the Android spellcheck/suggestion
menu. When a misspelling or suggestion region is tapped, I want to highlight
the text the spellcheck or suggestion menu applies to. This is very similar in
functionality to composition markers, but I can't reuse that marker type because
we can have a composition underline active at the same time as a highlighted
suggestion region (and then we'd have no way of clearing one but not the other).

BUG=672259,715365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add DocumentMarker::kActiveSuggestion MarkerType

This is a marker type I need to implement the Android spellcheck/suggestion
menu. When a misspelling or suggestion region is tapped, I want to highlight
the text the spellcheck or suggestion menu applies to. This is very similar in
functionality to composition markers, but I can't reuse that marker type because
we can have a composition underline active at the same time as a highlighted
suggestion region (and then we'd have no way of clearing one but not the other).

I'm refactoring most of CompositionMarker's functionality into a base class,
MarkerWithFormatting, that ActiveSuggestionMarker also inherits from.

BUG=672259,715365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rlanday, 2017-06-02 22:39:21]

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

[rlanday, 2017-06-02 22:42:24]

One potential issue with this CL is that it's not really self-contained; adding
this MarkerType by itself doesn't really do anything until I build the feature
on top. But if I combine all the code necessary for the feature in one CL, that
CL will be very large.

I later plan to add SuggestionMarker as another subclass of
MarkerWithFormatting; however, we don't need that for the spellcheck menu (only
SuggestionSpan support), so I need this MarkerType first.

https://codereview.chromium.org/2919173002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2919173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:682:
CompositionUnderline underline(
We should probably rename CompositionUnderline and all the methods like
PaintSingleCompositionBackgroundRun() since they're now also being used for
ActiveSuggestion markers. We may want to do this in (a) separate CL(s) since
this CL is already somewhat large.

[commit-bot: I haz the power, 2017-06-02 22:46:45]

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

[commit-bot: I haz the power, 2017-06-02 22:46:47]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rlanday, 2017-06-02 22:57:08]

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

[commit-bot: I haz the power, 2017-06-02 22:57:40]

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-06-02 23:07:22]

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

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

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

[rlanday, 2017-06-02 23:28:39]

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

[commit-bot: I haz the power, 2017-06-02 23:29:06]

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-06-03 00:22:06]

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

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

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_...)

[Xiaocheng, 2017-06-03 02:48:19]

No more comments other than the name of the base class.

"Format" reminds me of document format. I think "style" is better.

And other markers also have their own styles. The difference is whether the
style can be customized.

So how about "StyleableMarker"?
(Hope it doesn't let people think that the marker can be customized with CSS...)

https://codereview.chromium.org/2919173002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2919173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:682:
CompositionUnderline underline(
On 2017/06/02 at 22:42:24, rlanday wrote:
> We should probably rename CompositionUnderline and all the methods like
PaintSingleCompositionBackgroundRun() since they're now also being used for
ActiveSuggestion markers. We may want to do this in (a) separate CL(s) since
this CL is already somewhat large.

Agreed.

[yosin_UTC9, 2017-06-05 03:28:00]

Could you split this patch into
 - introduce MarkerWithFormatting + CompositionMarker; no behavior changes
 - introduce ActiveSuggestionMarker + ListImpl; w/ tests
 - Make painter to support; w/ tests
 - AXNodeObject; you need to discussion AX team about handling of
ActiveSuggestionMarker. I'm not sure whether returning to new marker is enough
or not.

[rlanday, 2017-06-05 03:29:52]

On 2017/06/05 at 03:28:00, yosin wrote:
> Could you split this patch into
>  - introduce MarkerWithFormatting + CompositionMarker; no behavior changes
>  - introduce ActiveSuggestionMarker + ListImpl; w/ tests
>  - Make painter to support; w/ tests
>  - AXNodeObject; you need to discussion AX team about handling of
ActiveSuggestionMarker. I'm not sure whether returning to new marker is enough
or not.

What kind of test(s) do you want for the painter? A layout test to verify the
background color and underline are painted properly?

[yosin_UTC9, 2017-06-05 03:37:43]

On 2017/06/05 at 03:29:52, rlanday wrote:
> On 2017/06/05 at 03:28:00, yosin wrote:
> > Could you split this patch into
> >  - introduce MarkerWithFormatting + CompositionMarker; no behavior changes
> >  - introduce ActiveSuggestionMarker + ListImpl; w/ tests
> >  - Make painter to support; w/ tests
> >  - AXNodeObject; you need to discussion AX team about handling of
ActiveSuggestionMarker. I'm not sure whether returning to new marker is enough
or not.
> 
> What kind of test(s) do you want for the painter? A layout test to verify the
background color and underline are painted properly?

Exactly. Just copy composition marker painting test for now.

[dmazzoni, 2017-06-06 02:51:29]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2017-06-06 02:51:30]

It sounds like this marker type could be useful for accessibility.
Could you search the codebase for kWebAXMarkerTypeSpelling
and AX_MARKER_TYPE_SPELLING and add this marker type too?
They're kept in sync with AssertMatchingEnums. Let me know if
you have any questions.

Thanks!

[rlanday, 2017-06-06 04:40:08]

I split this CL up into:

1. Introduce abstract StyleableMarker subclass of DocumentMarker
https://codereview.chromium.org/2922233002

2. Add ActiveSuggestionMarker
https://codereview.chromium.org/2923033002

3. Add painting for ActiveSuggestionMarker
https://codereview.chromium.org/2925543003