[ActiveSuggestionMarker #2] Add ActiveSuggestionMarker

Add ActiveSuggestionMarker

This is a new type of DocumentMarker that will be used on Android to highlight
a region of text that an open spellcheck/suggestion menu pertains to. For a
spellcheck menu, we'll want to highlight the text in red (to match the
underline), but for a suggestion menu, we'll want to highlight the text in the
color of the suggestion underline (on Android, there are a handful of different
colors used based on which flags are set on the corresponding SuggestionSpan).
So I'm making this marker inherit from the StyleableMarker subclass I
refactored CompositionMarker on top of in
https://codereview.chromium.org/2922233002.

This CL doesn't actually add code to paint ActiveSuggestionMarkers; I will add
that in a future CL (together with layout tests).

BUG=715365
Review-Url: https://codereview.chromium.org/2923033002
Cr-Commit-Position: refs/heads/master@{#477476}
Committed: https://chromium.googlesource.com/chromium/src/+/cf561e1c071256c42e108ba8fd1fb71dcd1db57b

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

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

[rlanday, 2017-06-05 19:23:38]

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

[rlanday, 2017-06-05 19:25:05]

This is the second CL to result from splitting up
https://codereview.chromium.org/2919173002

https://codereview.chromium.org/2923033002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2923033002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1417: case
DocumentMarker::kActiveSuggestion:
dmazzoni@: is there anything else I need to do to provide proper accessibility
support for my feature?

[Xiaocheng, 2017-06-05 19:30:21]

lgtm

[rlanday, 2017-06-05 20:23:38]

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

[commit-bot: I haz the power, 2017-06-05 20:24:09]

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-06 00:45:24]

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

[commit-bot: I haz the power, 2017-06-06 00:45:25]

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

[yosin_UTC9, 2017-06-06 01:27:48]

lgtm w/ nits

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/ActiveSuggestionMarker.h
(right):

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/ActiveSuggestionMarker.h:26:
MarkerType GetType() const;
nit: s/;/const final;/

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/markers/StyleableMarker.cpp (right):

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/StyleableMarker.cpp:34: type ==
DocumentMarker::kActiveSuggestion;
Could you add a test for IsStylableMaker() with ActiveSuggetionMarke?

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2923033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1417: case
DocumentMarker::kActiveSuggestion:
I think this case should be added when clients of |AXNodeObject::Markers()| to
handle it.

[rlanday, 2017-06-06 01:48:46]

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

[commit-bot: I haz the power, 2017-06-06 01:49: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-06-06 01:49:33]

Updated. Still waiting for feedback from dmazzoni@ about AXNodeObject

[commit-bot: I haz the power, 2017-06-06 03:40:45]

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

[commit-bot: I haz the power, 2017-06-06 03:40:46]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-06-06 04:39:10]

On 2017/06/06 at 01:49:33, rlanday wrote:
> Updated. Still waiting for feedback from dmazzoni@ about AXNodeObject

dmazzoni's feedback on the all-in-one CL (which I split up):
https://codereview.chromium.org/2919173002/#msg22

[dmazzoni, 2017-06-06 15:27:52]

https://codereview.chromium.org/2923033002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/2923033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1417: case
DocumentMarker::kActiveSuggestion:
Copying from the other changelist.

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.

If you'd prefer to do this in a follow-up, or you don't think
we should use this marker for accessibiltiy at this time, then
don't handle it in this switch.

If you handle the marker type in the switch without making
the rest of the changes, we might get some warnings or
errors elsewhere in the code when we see an unrecognized
enum value.

[rlanday, 2017-06-06 19:08:26]

On 2017/06/06 at 15:27:52, dmazzoni wrote:
> If you'd prefer to do this in a follow-up, or you don't think
> we should use this marker for accessibiltiy at this time, then
> don't handle it in this switch.

I'm going to take it out of the switch statement in this CL and do the
accessibility stuff in a follow-up CL for cleanliness.

The enum stuff gets a little weird because we don't currently handle Composition
markers for accessibility (so we skip over some enum values)...we can discuss it
when I put up the next CL

[rlanday, 2017-06-06 19:13:15]

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

[commit-bot: I haz the power, 2017-06-06 19:13:40]

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

I've moved kActiveSuggestion to the "not handled" section of the switch
statement in AXNodeObject::Markers() and added a TODO.

This CL still needs approval from an owner of AXNodeObject.cpp before it can be
landed.

[dmazzoni, 2017-06-06 19:44:57]

lgtm

[rlanday, 2017-06-06 19:48:43]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-06-06 19:48:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
xiaochengh@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2923033002/#ps60001
(title: "Add TODO in AXNodeObject")

[commit-bot: I haz the power, 2017-06-06 19:49:10]

CQ is trying da patch.

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

[rlanday, 2017-06-06 21:16:16]

The CQ bit was unchecked by rlanday@chromium.org

[rlanday, 2017-06-06 21:20:53]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-06-06 21:20:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, yosin@chromium.org, xiaochengh@chromium.org
Link to the patchset: https://codereview.chromium.org/2923033002/#ps80001
(title: "Update comment on ActiveSuggestionMarker.h")

[commit-bot: I haz the power, 2017-06-06 21:21:30]

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-07 00:15:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496784053317050,
"parent_rev": "981b6a8a907ce40b46fbe76881a0f0394cc00d2b", "commit_rev":
"cf561e1c071256c42e108ba8fd1fb71dcd1db57b"}

[commit-bot: I haz the power, 2017-06-07 00:15:52]

Description was changed from

==========
Add ActiveSuggestionMarker

This is a new type of DocumentMarker that will be used on Android to highlight
a region of text that an open spellcheck/suggestion menu pertains to. For a
spellcheck menu, we'll want to highlight the text in red (to match the
underline), but for a suggestion menu, we'll want to highlight the text in the
color of the suggestion underline (on Android, there are a handful of different
colors used based on which flags are set on the corresponding SuggestionSpan).
So I'm making this marker inherit from the StyleableMarker subclass I
refactored CompositionMarker on top of in
https://codereview.chromium.org/2922233002.

This CL doesn't actually add code to paint ActiveSuggestionMarkers; I will add
that in a future CL (together with layout tests).

BUG=715365
==========

to

==========
Add ActiveSuggestionMarker

This is a new type of DocumentMarker that will be used on Android to highlight
a region of text that an open spellcheck/suggestion menu pertains to. For a
spellcheck menu, we'll want to highlight the text in red (to match the
underline), but for a suggestion menu, we'll want to highlight the text in the
color of the suggestion underline (on Android, there are a handful of different
colors used based on which flags are set on the corresponding SuggestionSpan).
So I'm making this marker inherit from the StyleableMarker subclass I
refactored CompositionMarker on top of in
https://codereview.chromium.org/2922233002.

This CL doesn't actually add code to paint ActiveSuggestionMarkers; I will add
that in a future CL (together with layout tests).

BUG=715365

Review-Url: https://codereview.chromium.org/2923033002
Cr-Commit-Position: refs/heads/master@{#477476}
Committed:
https://chromium.googlesource.com/chromium/src/+/cf561e1c071256c42e108ba8fd1f...
==========

[commit-bot: I haz the power, 2017-06-07 00:15:54]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/cf561e1c071256c42e108ba8fd1f...