Introduce StyledMarkupTraverser to StyledMarkupSerializer

Introduce StyledMarkupTraverser to StyledMarkupSerializer

We are now going to have StyledMarkupSerializer work on the Composed
tree. We found just templatizing  StyledMarkupSerializer was not enough.
As for traversing a node tree, even though you start to traverse a tree
in a composed-tree strategy, you would need to 'switch' the strategy to
a dom-tree strategy when you find a closed-shadow host. For example, a
<select> element is now a closed shadow host, but traversing <select>
element as a composed tree doesn't make sense (a <div> or something
would be in your clipboard). This means we need to call the DOM version
of traverseNodesForSerialization in the Composed tree version of that.

As a preparation, this CL introduces StyledMarkupTraverser which
implements traverseNodesForSerialization so that we can call
traverseNodesForSerialization in traverseNodesForSerialization
with a different editing strategy. I am going to fix
traverseNodesForSerialization later to do so.

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/1188063002.

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

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

[hajimehoshi, 2015-06-17 10:51:20]

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

[hajimehoshi, 2015-06-17 10:51:39]

PTAL

[yosin_UTC9, 2015-06-17 13:07:58]

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

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:71:
Traverser(StyledMarkupAccumulator* accumulator, Node* lastClosed)
How about moving this ctor before L244, I hope code review tool make small diff
with old L203-L204 and new L76.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:72: : m_accumulator(accumulator)
ASSERT(m_lastClosed || !m_accumlator)?

I think when m_accumulator is nullptr, m_lastClosed is nullptr too.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:73: , m_lastClosed(lastClosed)
It is better to initialize |m_wrappingStyle| to nullptr.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:75: if (m_accumulator &&
m_lastClosed && Strategy::parent(*m_lastClosed))
Let's use early return pattern.

[yosin_UTC9, 2015-06-17 13:31:02]

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

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:76: m_wrappingStyle =
EditingStyle::wrappingStyleForSerialization(Strategy::parent(*m_lastClosed),
m_accumulator->shouldAnnotate());
Since, we use |m_accumulator->shouldAnnotate()| four times, let's make
|Traverser| has a member function |shouldAnnoate()|. Also, diff size becomes
smaller. ;-)

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:366: if (text.parentElement() &&
text.parentElement()->tagQName() == textareaTag) {
isHTMLTextAreaElement(text.parentElement())

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:379:
m_accumulator->appendTextWithInlineStyle(text, inlineStyle);
Note: For supporting composed tree, we should have entry mask for parent here.
ALL-IN-ONE does so.

When |!shouldAnnotate()| in |appendTextWithInlineStyle|, we need to calculate
entry mask from Strategy::parent from text node, we should calculate entry mask
here and pass to |appendText()|.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:401: if (m_accumulator &&
node.isElementNode())
Let's use early return pattern.

[yosin_UTC9, 2015-06-17 14:06:28]

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

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:353: if
(m_accumulator->convertBlocksToInlines() && isBlock(&node))
We have two |m_accumulator->convertBlocksToInlines()|, so it may be better to
have |Tarverser::convertBlocksToInlines()| for reducing diff count.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:402:
m_accumulator->appendEndTag(toElement(node));
We need to check whether we use empty tag or not, it depends on document ,
|Strategy::hasChildren()|, and |elemtnCannotHaveEndTag()|. See ALL-IN-ONE.

[yosin_UTC9, 2015-06-17 14:33:07]

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

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:337: if
(shouldApplyWrappingStyle(element) || needsInlineStyle(element))
Note: in ALL-IN-ONE, we check |shouldApplyWrappingStyle(element)| and
|needInlineStyle(element)| before creating style to avoid useless style creation
for reducing memory and save execution time. So, we can exclude this
optimization.

[hajimehoshi, 2015-06-18 02:04:56]

Thank you!

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

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:71:
Traverser(StyledMarkupAccumulator* accumulator, Node* lastClosed)
On 2015/06/17 13:07:57, Yosi_UTC9 wrote:
> How about moving this ctor before L244, I hope code review tool make small
diff
> with old L203-L204 and new L76.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:72: : m_accumulator(accumulator)
On 2015/06/17 13:07:57, Yosi_UTC9 wrote:
> ASSERT(m_lastClosed || !m_accumlator)?
> 
> I think when m_accumulator is nullptr, m_lastClosed is nullptr too.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:73: , m_lastClosed(lastClosed)
On 2015/06/17 13:07:58, Yosi_UTC9 wrote:
> It is better to initialize |m_wrappingStyle| to nullptr.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:75: if (m_accumulator &&
m_lastClosed && Strategy::parent(*m_lastClosed))
On 2015/06/17 13:07:58, Yosi_UTC9 wrote:
> Let's use early return pattern.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:76: m_wrappingStyle =
EditingStyle::wrappingStyleForSerialization(Strategy::parent(*m_lastClosed),
m_accumulator->shouldAnnotate());
On 2015/06/17 13:31:02, Yosi_UTC9 wrote:
> Since, we use |m_accumulator->shouldAnnotate()| four times, let's make
> |Traverser| has a member function |shouldAnnoate()|. Also, diff size becomes
> smaller. ;-)

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:337: if
(shouldApplyWrappingStyle(element) || needsInlineStyle(element))
On 2015/06/17 14:33:06, Yosi_UTC9 wrote:
> Note: in ALL-IN-ONE, we check |shouldApplyWrappingStyle(element)| and
> |needInlineStyle(element)| before creating style to avoid useless style
creation
> for reducing memory and save execution time. So, we can exclude this
> optimization.

As we talked, let's do such fix later.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:353: if
(m_accumulator->convertBlocksToInlines() && isBlock(&node))
On 2015/06/17 14:06:28, Yosi_UTC9 wrote:
> We have two |m_accumulator->convertBlocksToInlines()|, so it may be better to
> have |Tarverser::convertBlocksToInlines()| for reducing diff count.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:366: if (text.parentElement() &&
text.parentElement()->tagQName() == textareaTag) {
On 2015/06/17 13:31:01, Yosi_UTC9 wrote:
> isHTMLTextAreaElement(text.parentElement())

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:379:
m_accumulator->appendTextWithInlineStyle(text, inlineStyle);
On 2015/06/17 13:31:02, Yosi_UTC9 wrote:
> Note: For supporting composed tree, we should have entry mask for parent here.
> ALL-IN-ONE does so.
> 
> When |!shouldAnnotate()| in |appendTextWithInlineStyle|, we need to calculate
> entry mask from Strategy::parent from text node, we should calculate entry
mask
> here and pass to |appendText()|.

As we talked, do such fix later.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:401: if (m_accumulator &&
node.isElementNode())
On 2015/06/17 13:31:02, Yosi_UTC9 wrote:
> Let's use early return pattern.

Done.

https://codereview.chromium.org/1188693005/diff/20001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:402:
m_accumulator->appendEndTag(toElement(node));
On 2015/06/17 14:06:28, Yosi_UTC9 wrote:
> We need to check whether we use empty tag or not, it depends on document ,
> |Strategy::hasChildren()|, and |elemtnCannotHaveEndTag()|. See ALL-IN-ONE.

As we talked, do such fix later.

[yosin_UTC9, 2015-06-18 05:13:29]

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

https://codereview.chromium.org/1188693005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:182: Node* lastClosed =
traverser.traverseNodesForSerialization(firstNode, pastEnd);
s/traverseNodesForSerialization/serializeNodes/
or
s/traverseNodesForSerialization/traverse/
or
s/traverseNodesForSerialization/traverseNodes/

https://codereview.chromium.org/1188693005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:251: ASSERT(m_lastClosed ||
!m_accumulator);
How about this?

if (!m_accumulator) {
  ASSERT(!m_lastClosed);
  return;
}

if (!m_lastClosed)
  return;

ContainerNode* parent = Strategy::parent(*m_lastClosed);
if (!parent)
  return;
m_wrappingStyle = ...

To avoid complex assertion and expensive |Strategy::parent()| calll.

[hajimehoshi, 2015-06-18 05:24:55]

Thank you!

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

https://codereview.chromium.org/1188693005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:182: Node* lastClosed =
traverser.traverseNodesForSerialization(firstNode, pastEnd);
On 2015/06/18 05:13:29, Yosi_UTC9 wrote:
> s/traverseNodesForSerialization/serializeNodes/
> or
> s/traverseNodesForSerialization/traverse/
> or
> s/traverseNodesForSerialization/traverseNodes/
> 

Done.

https://codereview.chromium.org/1188693005/diff/80001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:251: ASSERT(m_lastClosed ||
!m_accumulator);
On 2015/06/18 05:13:29, Yosi_UTC9 wrote:
> How about this?
> 
> if (!m_accumulator) {
>   ASSERT(!m_lastClosed);
>   return;
> }
> 
> if (!m_lastClosed)
>   return;
> 
> ContainerNode* parent = Strategy::parent(*m_lastClosed);
> if (!parent)
>   return;
> m_wrappingStyle = ...
> 
> To avoid complex assertion and expensive |Strategy::parent()| calll.

Done.

[yosin_UTC9, 2015-06-18 06:06:57]

I'm OK with this patch.
Since, I'm co-author of this patch. So, I'm not a good for reviewer.

tkent@, PTAL

[tkent, 2015-06-18 06:10:49]

rslgtm.

[tkent, 2015-06-18 06:11:15]

lgtm.

[hajimehoshi, 2015-06-18 06:14:55]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2015-06-18 06:15:23]

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

[commit-bot: I haz the power, 2015-06-18 06:26:54]

Committed patchset #6 (id:90001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197322

[yosin_UTC9, 2015-06-18 13:28:51]

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

https://codereview.chromium.org/1188693005/diff/90001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:255: ASSERT(m_lastClosed);
|m_lastClosed| can be |nullptr| according to Patch Set 4. Is it OK to change to
ASSERT?

[hajimehoshi, 2015-06-19 04:25:08]

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

https://codereview.chromium.org/1188693005/diff/90001/Source/core/editing/Sty...
Source/core/editing/StyledMarkupSerializer.cpp:255: ASSERT(m_lastClosed);
On 2015/06/18 13:28:51, Yosi_UTC9 wrote:
> |m_lastClosed| can be |nullptr| according to Patch Set 4. Is it OK to change
to
> ASSERT?

Good point. I'll fix this.