Refactoring: Replace boolean parameters with enum classes (issue 1159033006 by hajimehoshi@chromium.org)

[hajim...@chromium.org]

Reviewers: Yosi_UTC9,

Message:
PTAL

Description:
Refactoring: Replace boolean parameters with enum classes

This CL replaces boolean parameters with enum classes newly introduced.
Additionaly, this CL replaces an existing enum with an enum class.

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

Please review this at https://codereview.chromium.org/1159033006/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+41, -26 lines):
M Source/core/editing/MarkupAccumulator.h
M Source/core/editing/MarkupAccumulator.cpp
M Source/core/editing/StyledMarkupAccumulator.h
M Source/core/editing/StyledMarkupAccumulator.cpp
M Source/core/editing/StyledMarkupSerializer.h
M Source/core/editing/StyledMarkupSerializer.cpp
M Source/core/xml/XMLSerializer.cpp


Index: Source/core/editing/MarkupAccumulator.cpp
diff --git a/Source/core/editing/MarkupAccumulator.cpp
b/Source/core/editing/MarkupAccumulator.cpp
index
09769d4fbe54a054446f9464043b08e080de6e55..dc08c42de75994243bea07922ae34bdc044a2fd5
100644
--- a/Source/core/editing/MarkupAccumulator.cpp
+++ b/Source/core/editing/MarkupAccumulator.cpp
@@ -514,7 +514,7 @@ bool MarkupAccumulator::shouldSelfClose(const Element&
element) const

bool MarkupAccumulator::serializeAsHTMLDocument(const Node& node) const
{
- if (m_serializationType == ForcedXML)
+ if (m_serializationType == SerializationType::ForcedXML)
return false;
return node.document().isHTMLDocument();
}
Index: Source/core/editing/MarkupAccumulator.h
diff --git a/Source/core/editing/MarkupAccumulator.h
b/Source/core/editing/MarkupAccumulator.h
index
0131ad4c6cd7cc5898a8656f26115f0adadafc27..11630f843065f146eb2b0c6f16d5c128348894b5
100644
--- a/Source/core/editing/MarkupAccumulator.h
+++ b/Source/core/editing/MarkupAccumulator.h
@@ -57,7 +57,7 @@ enum EntityMask {
EntityMaskInHTMLAttributeValue = EntityAmp | EntityQuot | EntityNbsp,
};

-enum SerializationType {
+enum class SerializationType {
AsOwnerDocument,
ForcedXML
};
@@ -70,7 +70,7 @@ public:
static void appendCharactersReplacingEntities(StringBuilder&, const
String&, unsigned, unsigned, EntityMask);
static size_t totalLength(const Vector<String>&);

- MarkupAccumulator(EAbsoluteURLs, SerializationType = AsOwnerDocument);
+ MarkupAccumulator(EAbsoluteURLs, SerializationType =
SerializationType::AsOwnerDocument);
virtual ~MarkupAccumulator();

void appendString(const String&);
Index: Source/core/editing/StyledMarkupAccumulator.cpp
diff --git a/Source/core/editing/StyledMarkupAccumulator.cpp
b/Source/core/editing/StyledMarkupAccumulator.cpp
index
a4496f6efe65f489376710645441bc4c0fea2117..8f66b08c94346b763256cd64738b280f0d9dcf76
100644
--- a/Source/core/editing/StyledMarkupAccumulator.cpp
+++ b/Source/core/editing/StyledMarkupAccumulator.cpp
@@ -143,15 +143,15 @@ void
StyledMarkupAccumulator::appendText(StringBuilder& out, Text& text)

void StyledMarkupAccumulator::appendElement(StringBuilder& out, Element&
element, Namespaces*)
{
- appendElement(out, element, false, DoesFullySelectNode);
+ appendElement(out, element, AddDisplayInline::DoesNotAddDisplayInline,
RangeFullySelectsNode::DoesFullySelectNode);
}

-void StyledMarkupAccumulator::appendElement(StringBuilder& out, Element&
element, bool addDisplayInline,
StyledMarkupAccumulator::RangeFullySelectsNode rangeFullySelectsNode)
+void StyledMarkupAccumulator::appendElement(StringBuilder& out, Element&
element, AddDisplayInline addDisplayInline, RangeFullySelectsNode
rangeFullySelectsNode)
{
const bool documentIsHTML = element.document().isHTMLDocument();
m_accumulator.appendOpenTag(out, element, 0);

- const bool shouldAnnotateOrForceInline = element.isHTMLElement() &&
(shouldAnnotate() || addDisplayInline);
+ const bool shouldAnnotateOrForceInline = element.isHTMLElement() &&
(shouldAnnotate() || addDisplayInline ==
AddDisplayInline::DoesAddDisplayInline);
const bool shouldOverrideStyleAttr = shouldAnnotateOrForceInline ||
shouldApplyWrappingStyle(element);

AttributeCollection attributes = element.attributes();
@@ -180,12 +180,12 @@ void
StyledMarkupAccumulator::appendElement(StringBuilder& out, Element& element
if (shouldAnnotate())

newInlineStyle->mergeStyleFromRulesForSerialization(&toHTMLElement(element));

- if (addDisplayInline)
+ if (addDisplayInline == AddDisplayInline::DoesAddDisplayInline)
newInlineStyle->forceInline();

// If the node is not fully selected by the range, then we
don't want to keep styles that affect its relationship to the nodes around
it
// only the ones that affect it and the nodes within it.
- if (rangeFullySelectsNode == DoesNotFullySelectNode &&
newInlineStyle->style())
+ if (rangeFullySelectsNode ==
RangeFullySelectsNode::DoesNotFullySelectNode && newInlineStyle->style())
newInlineStyle->style()->removeProperty(CSSPropertyFloat);
}

@@ -199,11 +199,11 @@ void
StyledMarkupAccumulator::appendElement(StringBuilder& out, Element& element
m_accumulator.appendCloseTag(out, element);
}

-void StyledMarkupAccumulator::appendStyleNodeOpenTag(StringBuilder& out,
StylePropertySet* style, bool isBlock)
+void StyledMarkupAccumulator::appendStyleNodeOpenTag(StringBuilder& out,
StylePropertySet* style, IsBlock isBlock)
{
// wrappingStyleForSerialization should have removed
-webkit-text-decorations-in-effect
ASSERT(propertyMissingOrEqualToNone(style,
CSSPropertyWebkitTextDecorationsInEffect));
- if (isBlock)
+ if (isBlock == IsBlock::Block)
out.appendLiteral("<div style=\"");
else
out.appendLiteral("<span style=\"");
Index: Source/core/editing/StyledMarkupAccumulator.h
diff --git a/Source/core/editing/StyledMarkupAccumulator.h
b/Source/core/editing/StyledMarkupAccumulator.h
index
c2555acb08196f748f49b0e35e93e9565556726c..cb7c50ba2053fe9673d3d3a8a8de06f2e9c4c4f2
100644
--- a/Source/core/editing/StyledMarkupAccumulator.h
+++ b/Source/core/editing/StyledMarkupAccumulator.h
@@ -40,12 +40,25 @@ class Document;
class StylePropertySet;
class Text;

+enum class RangeFullySelectsNode {
+ DoesFullySelectNode,
+ DoesNotFullySelectNode
+};
+
+enum class IsBlock {
+ Block,
+ NotBlock,
+};
+
+enum class AddDisplayInline {
+ DoesAddDisplayInline,
+ DoesNotAddDisplayInline,
+};
+
class StyledMarkupAccumulator final {
WTF_MAKE_NONCOPYABLE(StyledMarkupAccumulator);
STACK_ALLOCATED();
public:
- enum RangeFullySelectsNode { DoesFullySelectNode,
DoesNotFullySelectNode };
-
StyledMarkupAccumulator(EAbsoluteURLs, const TextOffset& start, const
TextOffset& end, const PassRefPtrWillBeRawPtr<Document>,
EAnnotateForInterchange, Node*);

void appendString(const String&);
@@ -55,8 +68,8 @@ public:
void appendEndMarkup(StringBuilder&, const Element&);

void appendElement(StringBuilder&, Element&, Namespaces*);
- void appendElement(StringBuilder&, Element&, bool,
RangeFullySelectsNode);
- void appendStyleNodeOpenTag(StringBuilder&, StylePropertySet*, bool
isBlock = false);
+ void appendElement(StringBuilder&, Element&, AddDisplayInline,
RangeFullySelectsNode);
+ void appendStyleNodeOpenTag(StringBuilder&, StylePropertySet*, IsBlock
= IsBlock::NotBlock);

// TODO(hajimehoshi): These functions are called from the serializer,
but
// should not.
Index: Source/core/editing/StyledMarkupSerializer.cpp
diff --git a/Source/core/editing/StyledMarkupSerializer.cpp
b/Source/core/editing/StyledMarkupSerializer.cpp
index
79b958b7b3271f10bf5d2267eae84bfd7f04a10e..2818a496ac1e33845e1b9ae4ccaa7ed771f1eb49
100644
--- a/Source/core/editing/StyledMarkupSerializer.cpp
+++ b/Source/core/editing/StyledMarkupSerializer.cpp
@@ -47,11 +47,11 @@ namespace blink {

namespace {

-const String& styleNodeCloseTag(bool isBlock)
+const String& styleNodeCloseTag(IsBlock isBlock)
{
DEFINE_STATIC_LOCAL(const String, divClose, ("</div>"));
DEFINE_STATIC_LOCAL(const String, styleSpanClose, ("</span>"));
- return isBlock ? divClose : styleSpanClose;
+ return isBlock == IsBlock::Block ? divClose : styleSpanClose;
}

template<typename PositionType>
@@ -176,12 +176,12 @@ String
StyledMarkupSerializer<Strategy>::createMarkup()

fullySelectedRootStyle->style()->setProperty(CSSPropertyTextDecoration,
CSSValueNone);
if
(!propertyMissingOrEqualToNone(fullySelectedRootStyle->style(),
CSSPropertyWebkitTextDecorationsInEffect))

fullySelectedRootStyle->style()->setProperty(CSSPropertyWebkitTextDecorationsInEffect,
CSSValueNone);
- wrapWithStyleNode(fullySelectedRootStyle->style(),
true);
+ wrapWithStyleNode(fullySelectedRootStyle->style(),
IsBlock::Block);
}
} else {
// Since this node and all the other ancestors are not in
the selection we want to set RangeFullySelectsNode to DoesNotFullySelectNode
// so that styles that affect the exterior of the node are
not included.
- wrapWithNode(*ancestor,
StyledMarkupAccumulator::DoesNotFullySelectNode);
+ wrapWithNode(*ancestor,
RangeFullySelectsNode::DoesNotFullySelectNode);
}

if (ancestor == m_highestNodeToBeSerialized)
@@ -197,20 +197,22 @@ String
StyledMarkupSerializer<Strategy>::createMarkup()
}

template<typename Strategy>
-void StyledMarkupSerializer<Strategy>::wrapWithNode(ContainerNode& node,
typename StyledMarkupAccumulator::RangeFullySelectsNode
rangeFullySelectsNode)
+void StyledMarkupSerializer<Strategy>::wrapWithNode(ContainerNode& node,
RangeFullySelectsNode rangeFullySelectsNode)
{
StringBuilder markup;
- if (node.isElementNode())
- m_markupAccumulator.appendElement(markup, toElement(node),
convertBlocksToInlines() && isBlock(&node), rangeFullySelectsNode);
- else
+ if (node.isElementNode()) {
+ AddDisplayInline addDisplayInline = convertBlocksToInlines() &&
isBlock(&node) ? AddDisplayInline::DoesAddDisplayInline :
AddDisplayInline::DoesNotAddDisplayInline;
+ m_markupAccumulator.appendElement(markup, toElement(node),
addDisplayInline, rangeFullySelectsNode);
+ } else {
m_markupAccumulator.appendStartMarkup(markup, node, 0);
+ }
m_reversedPrecedingMarkup.append(markup.toString());
if (node.isElementNode())
m_markupAccumulator.appendEndTag(toElement(node));
}

template<typename Strategy>
-void StyledMarkupSerializer<Strategy>::wrapWithStyleNode(StylePropertySet*
style, bool isBlock)
+void StyledMarkupSerializer<Strategy>::wrapWithStyleNode(StylePropertySet*
style, IsBlock isBlock)
{
StringBuilder openTag;
m_markupAccumulator.appendStyleNodeOpenTag(openTag, style, isBlock);
Index: Source/core/editing/StyledMarkupSerializer.h
diff --git a/Source/core/editing/StyledMarkupSerializer.h
b/Source/core/editing/StyledMarkupSerializer.h
index
9b7cbc06cecb41ab2bd89b0131ffd303ed8c9e3d..77a0b72491cb3bcfaca2976957a8ad380f0e6007
100644
--- a/Source/core/editing/StyledMarkupSerializer.h
+++ b/Source/core/editing/StyledMarkupSerializer.h
@@ -58,8 +58,8 @@ private:
Node* traverseNodesForSerialization(Node* startNode, Node* pastEnd,
NodeTraversalMode);

// TODO(hajimehoshi): These functions should be at the accumulator.
- void wrapWithNode(ContainerNode&,
StyledMarkupAccumulator::RangeFullySelectsNode =
StyledMarkupAccumulator::DoesFullySelectNode);
- void wrapWithStyleNode(StylePropertySet*, bool isBlock = false);
+ void wrapWithNode(ContainerNode&, RangeFullySelectsNode =
RangeFullySelectsNode::DoesFullySelectNode);
+ void wrapWithStyleNode(StylePropertySet*, IsBlock = IsBlock::NotBlock);

bool convertBlocksToInlines() const { return m_convertBlocksToInlines
== ConvertBlocksToInlines::Convert; }

Index: Source/core/xml/XMLSerializer.cpp
diff --git a/Source/core/xml/XMLSerializer.cpp
b/Source/core/xml/XMLSerializer.cpp
index
52d47621d28da8f9cfc9eeafb232942ce4c3de4f..c3b8d4d8a11a20181bb8e7f2a488d13fd1c54da9
100644
--- a/Source/core/xml/XMLSerializer.cpp
+++ b/Source/core/xml/XMLSerializer.cpp
@@ -29,7 +29,7 @@ namespace blink {
String XMLSerializer::serializeToString(Node* root)
{
ASSERT(root);
- MarkupAccumulator accumulator(DoNotResolveURLs, ForcedXML);
+ MarkupAccumulator accumulator(DoNotResolveURLs,
SerializationType::ForcedXML);
return serializeNodes<EditingStrategy>(accumulator, *root,
IncludeNode);
}

[hajim...@chromium.org]

+tkent for core/xml

https://codereview.chromium.org/1159033006/

[yo...@chromium.org]

Could you split this patch into two patches?
1 SerializationType
2 RangeFullySelectsNode, IsBlock, AddDisplayInline

How about member of enum class to use Yes No? Just asking not suggestion.

https://codereview.chromium.org/1159033006/

[hajim...@chromium.org]

On 2015/06/02 03:47:04, Yosi_UTC9 wrote:
> Could you split this patch into two patches?
> 1 SerializationType
> 2 RangeFullySelectsNode, IsBlock, AddDisplayInline

Sure.

> How about member of enum class to use Yes No? Just asking not suggestion.

I prefer your suggestion but I'm worried that there is not such usages in
Blink.



https://codereview.chromium.org/1159033006/

[hajim...@chromium.org]

On 2015/06/02 03:51:34, hajimehoshi wrote:
> On 2015/06/02 03:47:04, Yosi_UTC9 wrote:
> > Could you split this patch into two patches?
> > 1 SerializationType
> > 2 RangeFullySelectsNode, IsBlock, AddDisplayInline

> Sure.

Created the patch for SerializationType
(https://codereview.chromium.org/1157363007/). Now I'm fixing this patch.

https://codereview.chromium.org/1159033006/

[hajim...@chromium.org]

-tkent

https://codereview.chromium.org/1159033006/

[hajim...@chromium.org]

This CL now includes changes only for RangeFullySelectsNode, IsBlock,
AddDisplayInline. PTAL

https://codereview.chromium.org/1159033006/