[ActiveSuggestionMarker #1] Introduce abstract StyleableMarker subclass of DocumentMarker

Introduce abstract StyleableMarker subclass of DocumentMarker

This CL refactors some code in CompositionMarker into a new abstract class
StyleableMarker, which will contain shared logic for MarkerTypes that can be
rendered with different underline color/thickness and/or background color.

This is in preparation for adding two new MarkerTypes I need to support some
features on Android (one for marking the range of text an open spellcheck/
suggestion menu pertains to, the other for marking ranges of text the user can
open a suggestion menu for by tapping on them).

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

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

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

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

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

[commit-bot: I haz the power, 2017-06-05 19:08:25]

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-05 19:09:10]

This is the first CL resulting from splitting up
https://codereview.chromium.org/2919173002

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

lgtm

https://codereview.chromium.org/2922233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/editing/markers/StyleableMarker.h (right):

https://codereview.chromium.org/2922233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/editing/markers/StyleableMarker.h:33: };
add DISALLOW_COPY_AND_ASSIGN

[rlanday, 2017-06-05 20:14:30]

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

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

On 2017/06/05 at 19:21:55, xiaochengh wrote:
> lgtm
> 
>
https://codereview.chromium.org/2922233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/editing/markers/StyleableMarker.h (right):
> 
>
https://codereview.chromium.org/2922233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/editing/markers/StyleableMarker.h:33: };
> add DISALLOW_COPY_AND_ASSIGN

Updated

[commit-bot: I haz the power, 2017-06-05 20:15:03]

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

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

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

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

lgtm w/ nit

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

https://codereview.chromium.org/2922233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/StyleableMarker.cpp:17:
thickness_(thickness) {}
Q: Do we allow |underline_color == background_color|?
Is it better to have DCHECK_NE(underline_color, background_color)?

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

https://codereview.chromium.org/2922233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/StyleableMarker.h:37: bool
CORE_EXPORT IsStyleableMarker(const DocumentMarker&);
Could you add test for |IsStyleableMarker()| in "CompositionMarkerTest.cpp"?

[rlanday, 2017-06-06 01:42:44]

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

https://codereview.chromium.org/2922233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/markers/StyleableMarker.cpp:17:
thickness_(thickness) {}
On 2017/06/06 at 01:18:15, yosin_UTC9 wrote:
> Q: Do we allow |underline_color == background_color|?
> Is it better to have DCHECK_NE(underline_color, background_color)?

I don't know that we'd ever actually need to have both colors set the same, but
I also don't see a strong reason why we should DCHECK() in this case. DCHECKing
feels like it would be surprising behavior. It also doesn't seem totally
unreasonable that we might at some point have a StyleableMarker class that a
caller might want to sometimes render with both colors set to transparent, to be
invisible.

[rlanday, 2017-06-06 01:42:58]

The CQ bit was checked by rlanday@chromium.org

[rlanday, 2017-06-06 01:42:58]

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/2922233002/#ps40001
(title: "Add test for IsStyleableMarker()")

[commit-bot: I haz the power, 2017-06-06 01:43:46]

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 01:53:04]

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

[commit-bot: I haz the power, 2017-06-06 01:53:05]

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-06-06 02:16:54]

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

[rlanday, 2017-06-06 02:17:09]

On 2017/06/06 at 01:53:05, commit-bot wrote:
> 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...)

Adding tkent@ as OWNER reviewer for
third_party/WebKit/Source/core/testing/Internals.cpp

[tkent, 2017-06-06 06:09:35]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-06-06 06:09:35]

lgtm

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

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 06:58:25]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496729375283220,
"parent_rev": "1c2c3c3b4b76e2510fb5b809e0121fe47e5b723f", "commit_rev":
"44d1353d9f4b134e7aee2b1d70eae0e0320b42c7"}

[commit-bot: I haz the power, 2017-06-06 06:58:36]

Description was changed from

==========
Introduce abstract StyleableMarker subclass of DocumentMarker

This CL refactors some code in CompositionMarker into a new abstract class
StyleableMarker, which will contain shared logic for MarkerTypes that can be
rendered with different underline color/thickness and/or background color.

This is in preparation for adding two new MarkerTypes I need to support some
features on Android (one for marking the range of text an open spellcheck/
suggestion menu pertains to, the other for marking ranges of text the user can
open a suggestion menu for by tapping on them).

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

to

==========
Introduce abstract StyleableMarker subclass of DocumentMarker

This CL refactors some code in CompositionMarker into a new abstract class
StyleableMarker, which will contain shared logic for MarkerTypes that can be
rendered with different underline color/thickness and/or background color.

This is in preparation for adding two new MarkerTypes I need to support some
features on Android (one for marking the range of text an open spellcheck/
suggestion menu pertains to, the other for marking ranges of text the user can
open a suggestion menu for by tapping on them).

BUG=715365

Review-Url: https://codereview.chromium.org/2922233002
Cr-Commit-Position: refs/heads/master@{#477221}
Committed:
https://chromium.googlesource.com/chromium/src/+/44d1353d9f4b134e7aee2b1d70ea...
==========

[commit-bot: I haz the power, 2017-06-06 06:58:38]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/44d1353d9f4b134e7aee2b1d70ea...