Avoid style clobbering in setCompositionFromExistingText. (2nd land)

Avoid style clobbering in setCompositionFromExistingText. (2nd land)

This patch extends compositions to be able to span multiple nodes.  This involves
two main changes: first, compositions are switched to being stored as a Range, not
a Node plus two offsets.  Second, the underline painting now uses DocumentMarkerController
(which is already multi-node) instead of a custom composition-specific path.

As a result, we can remove the hack in setCompositionFromExistingText that clobbered
the text to shoehorn it into a single node, which would cause style loss.  This
is now able to span the existing text regardless of how complicated it is.

TEST=webkit_unit_tests --gtest_filter=DocumentMarkerControllerTest.UpdateRenderedRectsForComposition
TEST=webkit_unit_tests --gtest_filter=WebViewTest.SetCompositionFromExistingTextInRichText
BUG=488628
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201994

Committed: https://crrev.com/81808f5a2947e0a2c6d895fe59438ac7e3af04c8
git-svn-id: svn://svn.chromium.org/blink/trunk@202084 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[aelias_OOO_until_Jul13, 2015-08-29 03:07:27]

aelias@chromium.org changed reviewers:
+ yosin@chromium.org

[aelias_OOO_until_Jul13, 2015-08-29 03:07:27]

Hi yosin@, PTAL.  This is the patch that was posted on the bug with a few
improvements of my own.

What do you think?  I'm most concerned about the underline logic.  I observe
that the current patch sometimes fails to clear old underlines when the
composition moves to another word.  Any thoughts on why that happens and how
this ought to be written?

[yosin_UTC9, 2015-08-31 01:35:41]

I'm concern about speed of text painting. Before this patch,
text painter decides whether painting underline or not by
one compare instruction but this patch makes tree look up,
std::map, although it should be O(1) when using HashMap.

I suggest to have both m_compositinNode and m_compositionRange
and introduce m_compositionRange as special case with keeping 
current single node code path.

As we discussed before we don't need to paint underline for
multiple node case at this time. We'll do it by unification
of composition underline and document maker. We don't want
to have yet another implementation of DocumentMarker.

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:65: , m_isExistingText(true)
Can we use inverse logic? It is unclear why just initialized controller has
exiting text.

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
File Source/core/editing/InputMethodController.h (right):

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.h:81: // TODO: This method is called
only by WebViewImpl::compositionUnderlines().
nit: s/TODO/TODO(aelias)/

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.h:114: typedef std::map<const Node*,
std::pair<RefPtrWillBeMember<Node>, Vector<CompositionUnderline>>>
NodeUnderlinesMap;
nit: We should use WTF::HashMap. Both, key(const Node*) and value 
|std::pair<...>| should be collectable.

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.h:114: typedef std::map<const Node*,
std::pair<RefPtrWillBeMember<Node>, Vector<CompositionUnderline>>>
NodeUnderlinesMap;
I think we should have struct with DEFINE_TRACE() instead of std::pair<>

https://codereview.chromium.org/1325563002/diff/40001/Source/core/paint/Inlin...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1325563002/diff/40001/Source/core/paint/Inlin...
Source/core/paint/InlineTextBoxPainter.cpp:82: bool containsComposition =
m_inlineTextBox.layoutObject().node() &&
m_inlineTextBox.layoutObject().frame()->inputMethodController().isCompositionNode(m_inlineTextBox.layoutObject().node());
Note: |isCompositionNode(Node*)| should be fast, since it is called for every
|Text| nodes.

[aelias_OOO_until_Jul13, 2015-09-04 04:03:24]

Hi yosin@, PTAL.  I went ahead and rewrote the underline code to use
DocumentMarkers as you suggested.

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/40001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:65: , m_isExistingText(true)
On 2015/08/31 at 01:35:40, Yosi_UTC9 wrote:
> Can we use inverse logic? It is unclear why just initialized controller has
> exiting text.

Done.

[yosin_UTC9, 2015-09-04 06:20:43]

Awesome!

Could you split this patch into three patches?
1. DocumentMarker changes with a test for addTextCompositionMarker() in
DOcumentMarkerControllerTest
2. Use DoucumentMarker in *current* InputMethodControl with TextBoxPainter.cpp
changes
3. Change InputMethodController to use |Range|.

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:296: m_compositionRange =
Range::create(baseNode->document(), baseNode, baseOffset, baseNode,
extentOffset);
nit: Please create |Range| object at construction of |InputMethdoController|
rather than creating for each text composition for GC.

Once Oilpan GC is enabled, end of Q3/2015, |Range| object is lasting until
GC. So, we have unsued |Range| in |Document|.

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:327: EphemeralRange range =
PlainTextRange(compositionStart, compositionEnd).createRange(*editable);
nit: |const EphemeralRange|

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:331: Position start =
range.startPosition();
nit: |const Position|

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:335: Position end =
range.endPosition();
nit: |const Position|

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:350: m_compositionRange =
Range::create(range.document(), range.startPosition(), range.endPosition());
nit: Please reuse |Range| object.

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/Inp...
Source/core/editing/InputMethodController.cpp:357: return
EphemeralRange(m_compositionRange->startPosition(),
m_compositionRange->endPosition());
nit: |return EphemeralRange(m_compositionRange);|

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/mar...
File Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/mar...
Source/core/editing/markers/DocumentMarker.cpp:99: class
DocumentMarkerComposition final : public DocumentMarkerDetails {
|DocumentMarkerCompostion| can be misleading.
How about |DocumentMarkerDetailsForTextComposition| ,
|TextCompositionMarkerDetails| or another?

https://codereview.chromium.org/1325563002/diff/60001/Source/core/editing/mar...
Source/core/editing/markers/DocumentMarker.cpp:130: return 0;
nit: s/0/nullptr/;

https://codereview.chromium.org/1325563002/diff/60001/Source/core/paint/Inlin...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1325563002/diff/60001/Source/core/paint/Inlin...
Source/core/paint/InlineTextBoxPainter.cpp:10: #include "core/editing/Editor.h"
I think we can get rid of include Editor.h.

https://codereview.chromium.org/1325563002/diff/60001/Source/core/paint/Inlin...
Source/core/paint/InlineTextBoxPainter.cpp:11: #include
"core/editing/InputMethodController.h"
I think we can get rid of include InputMethodController.h.

[aelias_OOO_until_Jul13, 2015-09-08 03:20:04]

OK, I fixed/added tests and applied the requested changes, except for:

On 2015/09/04 at 06:20:43, yosin wrote:
> Could you split this patch into three patches?
> 1. DocumentMarker changes with a test for addTextCompositionMarker() in
DOcumentMarkerControllerTest
> 2. Use DoucumentMarker in *current* InputMethodControl with TextBoxPainter.cpp
changes
> 3. Change InputMethodController to use |Range|.

I'd like to push back on this and request to land this as a single patch.  The
reason is that this change will need to be cherry-picked to 46 branch. In my
experience, cherry-picking a chain of patches is difficult and error-prone
(particularly in Blink because it's still using SVN).

https://codereview.chromium.org/1325563002/diff/60001/Source/core/paint/Inlin...
File Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1325563002/diff/60001/Source/core/paint/Inlin...
Source/core/paint/InlineTextBoxPainter.cpp:10: #include "core/editing/Editor.h"
On 2015/09/04 at 06:20:43, Yosi_UTC9 wrote:
> I think we can get rid of include Editor.h.

No, this header is also needed for find matches.

[yosin_UTC9, 2015-09-08 06:34:04]

yosin@chromium.org changed reviewers:
+ pdr@chromium.org

[yosin_UTC9, 2015-09-08 06:34:05]

+pdr@ for core/paint chagnes.

Please add TEST= in description.

TEST=webkit_unit_tests
--gtest_filter=DocumentMarkerControllerTest.UpdateRenderedRectsForComposition
TEST=webkit_unit_tests
--gtest_filter=WebViewTest.SetCompositionFromExistingTextInRichText

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:91:
m_compositionRange->setEnd(frame().document(), 0);
nit: m_compositionRange->collapse(true);

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:163: if (start.anchorNode() &&
end.anchorNode()) {
We don't need to have this if-statement, if |!selection.isNone()|.

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:164: if
(m_compositionRange->isPointInRange(start.computeContainerNode(),
start.computeOffsetInContainerNode(), IGNORE_EXCEPTION)
How about,

selection.start().compareTo(m_compositionRange->startPosition()) >= 0
&& selection.end().compareTo(m_compositionRange->endPosition()) <= 0

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:314: if (underlines.size()) {
nit: Let's use underlines.isEmpty() and swap then and else clauses and
early return.

if (underlines.isEmpty()) {
    frame().document()->markers().addCompositionMarker(m_compositionRange->s
     tartPosition(), m_compositionRange->endPosition(), Color::black, false,
LayoutTheme::theme().platformDefaultCompositionBackgroundColor());
    return;
}

for (const auto& underline : underlines) {
  ....
}

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:315: for (size_t i = 0; i <
underlines.size(); ++i) {
nit: Let's use range-for, for (const auto& underline : underlines)

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:340: if (!start.anchorNode() ||
start.anchorNode()->rootEditableElement() != editable)
nit: editableRootForPosition(start) != editable

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:344: if (!end.anchorNode() ||
end.anchorNode()->rootEditableElement() != editable)
nit: editableRootForPosition(end) != editable

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:349: for (size_t i = 0; i <
underlines.size(); ++i) {
nit: Let's use range-for.

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/ma...
File Source/core/editing/markers/DocumentMarker.cpp (right):

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/ma...
Source/core/editing/markers/DocumentMarker.cpp:101: static
PassRefPtrWillBeRawPtr<TextCompositionMarkerDetails> instanceFor(Color
underlineColor, bool thick, Color backgroundColor);
nit: Could you use |create()| instead of |instanceFor()|?
It seems only DocumentMarker uses |instanceFor()|. 
We'll rename |instanceFor()|.

https://codereview.chromium.org/1325563002/diff/100001/Source/core/editing/ma...
Source/core/editing/markers/DocumentMarker.cpp:117: bool m_thick;
nit: Rather than having alignment padding in middle of object, it is better
to move |bool m_thick| to last of member variable declarations for
adding other |bool| in future.

[aelias_OOO_until_Jul13, 2015-09-09 02:30:53]

OK, I applied all of those nits to the latest patch set, please take another
look.

[yosin_UTC9, 2015-09-09 03:47:06]

lgtm

Thanks for using DocumentMarker for painting text
composition! You make me my life easier.

Please wait for pdr@ for core/paint/ changes.

[pdr., 2015-09-09 18:35:50]

On 2015/09/09 at 03:47:06, yosin wrote:
> lgtm
> 
> Thanks for using DocumentMarker for painting text
> composition! You make me my life easier.
> 
> Please wait for pdr@ for core/paint/ changes.

core/paint looks good, LGTM.

[aelias_OOO_until_Jul13, 2015-09-09 18:48:41]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2015-09-09 18:48:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325563002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325563002/140001

[commit-bot: I haz the power, 2015-09-09 19:44:30]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201994

[Mike West, 2015-09-10 09:01:30]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1330233003/ by mkwst@chromium.org.

The reason for reverting is: This patch caused repeatable failures on WebKit
Linux Leaks (starting at
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...).
Verified locally by adding `--enable-leak-detection` to the test runner on my
linux machine. Reverting this patch locally fixed the leaks. .

[yosin_UTC9, 2015-09-10 09:42:28]

Try to add --enable-leak-detection to content_shell.
We can see what objects are leaked.

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:161: Position start =
selection.start();
Please remove unused local variable |start|.

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:162: Position end =
selection.end();
Please remove unused local variable |end|.

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:369: PassRefPtrWillBeRawPtr<Range>
InputMethodController::compositionRange() const
It seems we should return raw pointer |Range*|.

[aelias_OOO_until_Jul13, 2015-09-10 19:59:29]

The leak was caused by the RefPtr<Document> inside the Range.  I fixed it in the
latest patchset by releasing the Range from Document::detach().

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
File Source/core/editing/InputMethodController.cpp (right):

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:161: Position start =
selection.start();
On 2015/09/10 at 09:42:28, Yosi_UTC9 wrote:
> Please remove unused local variable |start|.

Done.

https://codereview.chromium.org/1325563002/diff/140001/Source/core/editing/In...
Source/core/editing/InputMethodController.cpp:369: PassRefPtrWillBeRawPtr<Range>
InputMethodController::compositionRange() const
On 2015/09/10 at 09:42:28, Yosi_UTC9 wrote:
> It seems we should return raw pointer |Range*|.

Looks like I can't, because this is used for WebRange construction which takes
PassRefPtrWillBeRawPtr<Range>.

[aelias_OOO_until_Jul13, 2015-09-10 20:25:32]

The CQ bit was checked by aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-09-10 20:25:32]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1325563002/#ps160001
(title: "Release Range on document detach and remove selectionStart/End")

[commit-bot: I haz the power, 2015-09-10 20:25:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325563002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325563002/160001

[commit-bot: I haz the power, 2015-09-10 21:33:41]

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

[commit-bot: I haz the power, 2015-09-10 21:33:42]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aelias_OOO_until_Jul13, 2015-09-10 22:16:47]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2015-09-10 22:17:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1325563002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1325563002/160001

[commit-bot: I haz the power, 2015-09-11 01:14:15]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202084

[commit-bot: I haz the power, 2015-09-23 12:02:20]

Patchset 9 (id:??) landed as
https://crrev.com/c74bb6390ae0eae4fa55c30e40732e5f0e7035a1

[commit-bot: I haz the power, 2015-09-23 12:17:19]

Patchset 9 (id:??) landed as
https://crrev.com/81808f5a2947e0a2c6d895fe59438ac7e3af04c8