Move StyledMarkupAccumulator::shouldApplyWrappingStyle to its serializer

Move StyledMarkupAccumulator::shouldApplyWrappingStyle to its serializer

Now a node's parent is taken with Node::parent. To have the
accumulator work on the composed tree, we need to use
EditingInComposedTree::parent instead. This CL moves the code using
parentElement for appendText to StyledMarkupSerializer, that is already
templatized to switch the editing strategies.

This patch is result of collaboration work with yosin@chromium.org
for selection of web components. We've already prepared the new
node-traversal usages at crrev.com/1134473004.

BUG=275851
TEST=n/a; no behavior change

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197246

[hajimehoshi, 2015-06-12 10:52:26]

hajimehoshi@chromium.org changed reviewers:
+ tkent@chromium.org, yosin@chromium.org

[yosin_UTC9, 2015-06-12 12:09:55]

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
File Source/core/editing/StyledMarkupAccumulator.h (right):

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupAccumulator.h:52: void
appendTextWithWrappingStyle(Text&);
how about to call this function |appendTextWithInlineStyle|? Since, we have
|appendElementWithInlineStyle()| at L67.

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupSerializer.cpp:346: ContainerNode* parent =
Strategy::parent(text);
For checking text in TEXTAREA, we should take parent in DOM tree even
if we traverse in composed tree. In composed tree, text nodes are inside
editable element in shadow tree.

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupSerializer.cpp:351:
accumulator.appendTextWithWrappingStyle(text);
I think we may want to calculate inline style here and pass to
|appendTextWithWrappingStyle()|

[hajimehoshi, 2015-06-16 07:03:27]

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
File Source/core/editing/StyledMarkupAccumulator.h (right):

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupAccumulator.h:52: void
appendTextWithWrappingStyle(Text&);
On 2015/06/12 12:09:55, Yosi_UTC9 wrote:
> how about to call this function |appendTextWithInlineStyle|? Since, we have
> |appendElementWithInlineStyle()| at L67.

Hmm, in the case of text nodes, a new <span> element is created to wrap the text
and the style is applied. That's the difference.

[hajimehoshi, 2015-06-16 08:15:18]

Thank you!

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
File Source/core/editing/StyledMarkupAccumulator.h (right):

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupAccumulator.h:52: void
appendTextWithWrappingStyle(Text&);
On 2015/06/16 07:03:27, hajimehoshi wrote:
> On 2015/06/12 12:09:55, Yosi_UTC9 wrote:
> > how about to call this function |appendTextWithInlineStyle|? Since, we have
> > |appendElementWithInlineStyle()| at L67.
> 
> Hmm, in the case of text nodes, a new <span> element is created to wrap the
text
> and the style is applied. That's the difference.

Done.

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupSerializer.cpp:346: ContainerNode* parent =
Strategy::parent(text);
On 2015/06/12 12:09:55, Yosi_UTC9 wrote:
> For checking text in TEXTAREA, we should take parent in DOM tree even
> if we traverse in composed tree. In composed tree, text nodes are inside
> editable element in shadow tree.

Done.

https://codereview.chromium.org/1177323005/diff/1/Source/core/editing/StyledM...
Source/core/editing/StyledMarkupSerializer.cpp:351:
accumulator.appendTextWithWrappingStyle(text);
On 2015/06/12 12:09:55, Yosi_UTC9 wrote:
> I think we may want to calculate inline style here and pass to
> |appendTextWithWrappingStyle()|

Done.

[yosin_UTC9, 2015-06-16 08:23:57]

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupAccumulator.cpp (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupAccumulator.cpp:97: void
StyledMarkupAccumulator::appendTextWithInlineStyle(Text& text,
PassRefPtrWillBeRawPtr<EditingStyle> style)
better to use |inlineStyle| rather than |style|.

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:349:
RefPtrWillBeRawPtr<EditingStyle> style = nullptr;
s/style/inlineStyle/

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:377: bool
StyledMarkupSerializer<Strategy>::shouldApplyWrappingStyle(StyledMarkupAccumulator&
accumulator, const Node& node) const
|const StyledMarkupAccumulator&|

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupSerializer.h (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.h:60: bool shouldAnnotate() const {
return m_shouldAnnotate == AnnotateForInterchange; }
Can we have another CL for introducing |shouldAnnote()|?

[hajimehoshi, 2015-06-16 08:54:25]

Patchset #3 (id:40001) has been deleted

[hajimehoshi, 2015-06-16 08:58:06]

Patchset #3 (id:60001) has been deleted

[hajimehoshi, 2015-06-16 08:58:18]

Thank you!

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupAccumulator.cpp (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupAccumulator.cpp:97: void
StyledMarkupAccumulator::appendTextWithInlineStyle(Text& text,
PassRefPtrWillBeRawPtr<EditingStyle> style)
On 2015/06/16 08:23:56, Yosi_UTC9 wrote:
> better to use |inlineStyle| rather than |style|.

Done.

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:349:
RefPtrWillBeRawPtr<EditingStyle> style = nullptr;
On 2015/06/16 08:23:56, Yosi_UTC9 wrote:
> s/style/inlineStyle/

Done.

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:377: bool
StyledMarkupSerializer<Strategy>::shouldApplyWrappingStyle(StyledMarkupAccumulator&
accumulator, const Node& node) const
On 2015/06/16 08:23:56, Yosi_UTC9 wrote:
> |const StyledMarkupAccumulator&|

Done.

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupSerializer.h (right):

https://codereview.chromium.org/1177323005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.h:60: bool shouldAnnotate() const {
return m_shouldAnnotate == AnnotateForInterchange; }
On 2015/06/16 08:23:56, Yosi_UTC9 wrote:
> Can we have another CL for introducing |shouldAnnote()|?

Done. (Now committing)

[hajimehoshi, 2015-06-16 10:07:52]

https://codereview.chromium.org/1177323005/diff/80001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupAccumulator.cpp (right):

https://codereview.chromium.org/1177323005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupAccumulator.cpp:235: bool
StyledMarkupAccumulator::shouldApplyWrappingStyle(const Node& node) const
TODO: Remove this

[hajimehoshi, 2015-06-16 10:56:47]

https://codereview.chromium.org/1177323005/diff/80001/Source/core/editing/Sty...
File Source/core/editing/StyledMarkupAccumulator.cpp (right):

https://codereview.chromium.org/1177323005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupAccumulator.cpp:235: bool
StyledMarkupAccumulator::shouldApplyWrappingStyle(const Node& node) const
On 2015/06/16 10:07:51, hajimehoshi wrote:
> TODO: Remove this

Done.

[yosin_UTC9, 2015-06-17 07:45:21]

lgtm w/ nits

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
Source/core/editing/StyledMarkupSerializer.cpp:342: return;
s/return/break/

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
Source/core/editing/StyledMarkupSerializer.cpp:355: } case Node::ELEMENT_NODE: {
nit: please insert newline between "} case"

[hajimehoshi, 2015-06-17 07:49:32]

Thank you!

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
File Source/core/editing/StyledMarkupSerializer.cpp (right):

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
Source/core/editing/StyledMarkupSerializer.cpp:342: return;
On 2015/06/17 07:45:21, Yosi_UTC9 wrote:
> s/return/break/

Done.

https://codereview.chromium.org/1177323005/diff/160001/Source/core/editing/St...
Source/core/editing/StyledMarkupSerializer.cpp:355: } case Node::ELEMENT_NODE: {
On 2015/06/17 07:45:20, Yosi_UTC9 wrote:
> nit: please insert newline between "} case"

Done.

[hajimehoshi, 2015-06-17 07:49:38]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2015-06-17 07:49:39]

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/1177323005/#ps180001
(title: "yosin's review")

[commit-bot: I haz the power, 2015-06-17 07:50:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177323005/180001

[commit-bot: I haz the power, 2015-06-17 09:18:30]

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