Cleanup CXFA_Fill

This CL cleans up the CXFA_Fill internal getters to handle nullptr
returns correctly and moves some of the logic to the specific XFA
classes.

Change-Id: Icac487105a026a25cc9981d00fbc152e459ad0b8
Reviewed-on: https://pdfium-review.googlesource.com/22770
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Commit-Queue: dsinclair <dsinclair@chromium.org>

12 files changed, 143 insertions, 72 deletions

diff --git a/xfa/fxfa/parser/cxfa_color.cpp b/xfa/fxfa/parser/cxfa_color.cpp
index a072d0302b..95c0ad345b 100644
--- a/xfa/fxfa/parser/cxfa_color.cpp
+++ b/xfa/fxfa/parser/cxfa_color.cpp
@@ -37,3 +37,23 @@ CXFA_Color::CXFA_Color(CXFA_Document* doc, XFA_PacketType packet)
                 pdfium::MakeUnique<CJX_Color>(this)) {}
 
 CXFA_Color::~CXFA_Color() {}
+
+FX_ARGB CXFA_Color::GetValue() {
+  Optional<WideString> val = JSObject()->TryCData(XFA_Attribute::Value, false);
+  return val ? StringToFXARGB(val->AsStringView()) : 0xFF000000;
+}
+
+FX_ARGB CXFA_Color::GetValueOrDefault(FX_ARGB defaultValue) {
+  Optional<WideString> val = JSObject()->TryCData(XFA_Attribute::Value, false);
+  return val ? StringToFXARGB(val->AsStringView()) : defaultValue;
+}
+
+void CXFA_Color::SetValue(FX_ARGB color) {
+  int a;
+  int r;
+  int g;
+  int b;
+  std::tie(a, r, g, b) = ArgbDecode(color);
+  JSObject()->SetCData(XFA_Attribute::Value,
+                       WideString::Format(L"%d,%d,%d", r, g, b), false, false);
+}
diff --git a/xfa/fxfa/parser/cxfa_color.h b/xfa/fxfa/parser/cxfa_color.h
index 590bcc61da..b15c9d8806 100644
--- a/xfa/fxfa/parser/cxfa_color.h
+++ b/xfa/fxfa/parser/cxfa_color.h
@@ -11,8 +11,14 @@
 
 class CXFA_Color : public CXFA_Node {
  public:
+  static constexpr FX_ARGB kBlackColor = 0xFF000000;
+
   CXFA_Color(CXFA_Document* doc, XFA_PacketType packet);
   ~CXFA_Color() override;
+
+  FX_ARGB GetValue();
+  FX_ARGB GetValueOrDefault(FX_ARGB defaultValue);
+  void SetValue(FX_ARGB color);
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_COLOR_H_
diff --git a/xfa/fxfa/parser/cxfa_fill.cpp b/xfa/fxfa/parser/cxfa_fill.cpp
index c62006754f..e1e077b5bb 100644
--- a/xfa/fxfa/parser/cxfa_fill.cpp
+++ b/xfa/fxfa/parser/cxfa_fill.cpp
@@ -60,31 +60,19 @@ bool CXFA_Fill::IsVisible() {
 }
 
 void CXFA_Fill::SetColor(FX_ARGB color) {
-  CXFA_Color* pNode =
+  CXFA_Color* pColor =
       JSObject()->GetOrCreateProperty<CXFA_Color>(0, XFA_Element::Color);
-  if (!pNode)
+  if (!pColor)
     return;
 
-  int a;
-  int r;
-  int g;
-  int b;
-  std::tie(a, r, g, b) = ArgbDecode(color);
-  pNode->JSObject()->SetCData(XFA_Attribute::Value,
-                              WideString::Format(L"%d,%d,%d", r, g, b), false,
-                              false);
+  pColor->SetValue(color);
 }
 
 FX_ARGB CXFA_Fill::GetColor(bool bText) {
-  if (CXFA_Color* pNode = GetChild<CXFA_Color>(0, XFA_Element::Color, false)) {
-    Optional<WideString> wsColor =
-        pNode->JSObject()->TryCData(XFA_Attribute::Value, false);
-    if (wsColor)
-      return StringToFXARGB(wsColor->AsStringView());
-  }
-  if (bText)
-    return 0xFF000000;
-  return 0xFFFFFFFF;
+  CXFA_Color* pColor = GetChild<CXFA_Color>(0, XFA_Element::Color, false);
+  if (!pColor)
+    return bText ? 0xFF000000 : 0xFFFFFFFF;
+  return pColor->GetValueOrDefault(bText ? 0xFF000000 : 0xFFFFFFFF);
 }
 
 XFA_Element CXFA_Fill::GetFillType() const {
@@ -100,86 +88,75 @@ XFA_Element CXFA_Fill::GetFillType() const {
 }
 
 XFA_AttributeEnum CXFA_Fill::GetPatternType() {
-  return GetPattern()->JSObject()->GetEnum(XFA_Attribute::Type);
+  CXFA_Pattern* pattern = GetPatternIfExists();
+  return pattern ? pattern->GetType() : CXFA_Pattern::kDefaultType;
 }
 
 FX_ARGB CXFA_Fill::GetPatternColor() {
-  if (CXFA_Color* pColor =
-          GetPattern()->GetChild<CXFA_Color>(0, XFA_Element::Color, false)) {
-    Optional<WideString> wsColor =
-        pColor->JSObject()->TryCData(XFA_Attribute::Value, false);
-    if (wsColor)
-      return StringToFXARGB(wsColor->AsStringView());
-  }
-  return 0xFF000000;
+  CXFA_Pattern* pattern = GetPatternIfExists();
+  if (!pattern)
+    return CXFA_Color::kBlackColor;
+
+  CXFA_Color* pColor = pattern->GetColorIfExists();
+  return pColor ? pColor->GetValue() : CXFA_Color::kBlackColor;
 }
 
 int32_t CXFA_Fill::GetStippleRate() {
-  return GetStipple()
-      ->JSObject()
-      ->TryInteger(XFA_Attribute::Rate, true)
-      .value_or(50);
+  CXFA_Stipple* stipple = GetStippleIfExists();
+  if (!stipple)
+    return CXFA_Stipple::GetDefaultRate();
+  return stipple->GetRate();
 }
 
 FX_ARGB CXFA_Fill::GetStippleColor() {
-  if (CXFA_Color* pColor =
-          GetStipple()->GetChild<CXFA_Color>(0, XFA_Element::Color, false)) {
-    Optional<WideString> wsColor =
-        pColor->JSObject()->TryCData(XFA_Attribute::Value, false);
-    if (wsColor)
-      return StringToFXARGB(wsColor->AsStringView());
-  }
-  return 0xFF000000;
+  CXFA_Stipple* stipple = GetStippleIfExists();
+  if (!stipple)
+    return CXFA_Color::kBlackColor;
+
+  CXFA_Color* pColor = stipple->GetColorIfExists();
+  return pColor ? pColor->GetValue() : CXFA_Color::kBlackColor;
 }
 
 XFA_AttributeEnum CXFA_Fill::GetLinearType() {
-  return GetLinear()
-      ->JSObject()
-      ->TryEnum(XFA_Attribute::Type, true)
-      .value_or(XFA_AttributeEnum::ToRight);
+  CXFA_Linear* linear = GetLinearIfExists();
+  return linear ? linear->GetType() : CXFA_Linear::kDefaultType;
 }
 
 FX_ARGB CXFA_Fill::GetLinearColor() {
-  if (CXFA_Color* pColor =
-          GetLinear()->GetChild<CXFA_Color>(0, XFA_Element::Color, false)) {
-    Optional<WideString> wsColor =
-        pColor->JSObject()->TryCData(XFA_Attribute::Value, false);
-    if (wsColor)
-      return StringToFXARGB(wsColor->AsStringView());
-  }
-  return 0xFF000000;
+  CXFA_Linear* linear = GetLinearIfExists();
+  if (!linear)
+    return CXFA_Color::kBlackColor;
+
+  CXFA_Color* pColor = linear->GetColorIfExists();
+  return pColor ? pColor->GetValue() : CXFA_Color::kBlackColor;
 }
 
 bool CXFA_Fill::IsRadialToEdge() {
-  return GetRadial()
-             ->JSObject()
-             ->TryEnum(XFA_Attribute::Type, true)
-             .value_or(XFA_AttributeEnum::ToEdge) == XFA_AttributeEnum::ToEdge;
+  CXFA_Radial* radial = GetRadialIfExists();
+  return radial ? radial->IsToEdge() : false;
 }
 
 FX_ARGB CXFA_Fill::GetRadialColor() {
-  if (CXFA_Color* pColor =
-          GetRadial()->GetChild<CXFA_Color>(0, XFA_Element::Color, false)) {
-    Optional<WideString> wsColor =
-        pColor->JSObject()->TryCData(XFA_Attribute::Value, false);
-    if (wsColor)
-      return StringToFXARGB(wsColor->AsStringView());
-  }
-  return 0xFF000000;
+  CXFA_Radial* radial = GetRadialIfExists();
+  if (!radial)
+    return CXFA_Color::kBlackColor;
+
+  CXFA_Color* pColor = radial->GetColorIfExists();
+  return pColor ? pColor->GetValue() : CXFA_Color::kBlackColor;
 }
 
-CXFA_Stipple* CXFA_Fill::GetStipple() {
+CXFA_Stipple* CXFA_Fill::GetStippleIfExists() {
   return JSObject()->GetOrCreateProperty<CXFA_Stipple>(0, XFA_Element::Stipple);
 }
 
-CXFA_Radial* CXFA_Fill::GetRadial() {
+CXFA_Radial* CXFA_Fill::GetRadialIfExists() {
   return JSObject()->GetOrCreateProperty<CXFA_Radial>(0, XFA_Element::Radial);
 }
 
-CXFA_Linear* CXFA_Fill::GetLinear() {
+CXFA_Linear* CXFA_Fill::GetLinearIfExists() {
   return JSObject()->GetOrCreateProperty<CXFA_Linear>(0, XFA_Element::Linear);
 }
 
-CXFA_Pattern* CXFA_Fill::GetPattern() {
+CXFA_Pattern* CXFA_Fill::GetPatternIfExists() {
   return JSObject()->GetOrCreateProperty<CXFA_Pattern>(0, XFA_Element::Pattern);
 }
diff --git a/xfa/fxfa/parser/cxfa_fill.h b/xfa/fxfa/parser/cxfa_fill.h
index a713047c5d..dc733f5a24 100644
--- a/xfa/fxfa/parser/cxfa_fill.h
+++ b/xfa/fxfa/parser/cxfa_fill.h
@@ -40,10 +40,10 @@ class CXFA_Fill : public CXFA_Node {
   FX_ARGB GetRadialColor();
 
  private:
-  CXFA_Stipple* GetStipple();
-  CXFA_Radial* GetRadial();
-  CXFA_Linear* GetLinear();
-  CXFA_Pattern* GetPattern();
+  CXFA_Stipple* GetStippleIfExists();
+  CXFA_Radial* GetRadialIfExists();
+  CXFA_Linear* GetLinearIfExists();
+  CXFA_Pattern* GetPatternIfExists();
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_FILL_H_
diff --git a/xfa/fxfa/parser/cxfa_linear.cpp b/xfa/fxfa/parser/cxfa_linear.cpp
index 5ca27916ae..f5b605dcb8 100644
--- a/xfa/fxfa/parser/cxfa_linear.cpp
+++ b/xfa/fxfa/parser/cxfa_linear.cpp
@@ -8,6 +8,7 @@
 
 #include "fxjs/xfa/cjx_linear.h"
 #include "third_party/base/ptr_util.h"
+#include "xfa/fxfa/parser/cxfa_color.h"
 
 namespace {
 
@@ -38,3 +39,13 @@ CXFA_Linear::CXFA_Linear(CXFA_Document* doc, XFA_PacketType packet)
                 pdfium::MakeUnique<CJX_Linear>(this)) {}
 
 CXFA_Linear::~CXFA_Linear() {}
+
+XFA_AttributeEnum CXFA_Linear::GetType() {
+  return JSObject()
+      ->TryEnum(XFA_Attribute::Type, true)
+      .value_or(XFA_AttributeEnum::ToRight);
+}
+
+CXFA_Color* CXFA_Linear::GetColorIfExists() {
+  return GetChild<CXFA_Color>(0, XFA_Element::Color, false);
+}
diff --git a/xfa/fxfa/parser/cxfa_linear.h b/xfa/fxfa/parser/cxfa_linear.h
index af1b8e8a28..d17f7ffcb2 100644
--- a/xfa/fxfa/parser/cxfa_linear.h
+++ b/xfa/fxfa/parser/cxfa_linear.h
@@ -9,10 +9,17 @@
 
 #include "xfa/fxfa/parser/cxfa_node.h"
 
+class CXFA_Color;
+
 class CXFA_Linear : public CXFA_Node {
  public:
+  static constexpr XFA_AttributeEnum kDefaultType = XFA_AttributeEnum::ToRight;
+
   CXFA_Linear(CXFA_Document* doc, XFA_PacketType packet);
   ~CXFA_Linear() override;
+
+  XFA_AttributeEnum GetType();
+  CXFA_Color* GetColorIfExists();
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_LINEAR_H_
diff --git a/xfa/fxfa/parser/cxfa_pattern.cpp b/xfa/fxfa/parser/cxfa_pattern.cpp
index 7328fd254c..3102f77beb 100644
--- a/xfa/fxfa/parser/cxfa_pattern.cpp
+++ b/xfa/fxfa/parser/cxfa_pattern.cpp
@@ -8,6 +8,7 @@
 
 #include "fxjs/xfa/cjx_pattern.h"
 #include "third_party/base/ptr_util.h"
+#include "xfa/fxfa/parser/cxfa_color.h"
 
 namespace {
 
@@ -38,3 +39,11 @@ CXFA_Pattern::CXFA_Pattern(CXFA_Document* doc, XFA_PacketType packet)
                 pdfium::MakeUnique<CJX_Pattern>(this)) {}
 
 CXFA_Pattern::~CXFA_Pattern() {}
+
+CXFA_Color* CXFA_Pattern::GetColorIfExists() {
+  return GetChild<CXFA_Color>(0, XFA_Element::Color, false);
+}
+
+XFA_AttributeEnum CXFA_Pattern::GetType() {
+  return JSObject()->GetEnum(XFA_Attribute::Type);
+}
diff --git a/xfa/fxfa/parser/cxfa_pattern.h b/xfa/fxfa/parser/cxfa_pattern.h
index 44fb0af2b7..7e115f2d9d 100644
--- a/xfa/fxfa/parser/cxfa_pattern.h
+++ b/xfa/fxfa/parser/cxfa_pattern.h
@@ -9,10 +9,17 @@
 
 #include "xfa/fxfa/parser/cxfa_node.h"
 
+class CXFA_Color;
+
 class CXFA_Pattern : public CXFA_Node {
  public:
+  static constexpr XFA_AttributeEnum kDefaultType = XFA_AttributeEnum::Unknown;
+
   CXFA_Pattern(CXFA_Document* doc, XFA_PacketType packet);
   ~CXFA_Pattern() override;
+
+  XFA_AttributeEnum GetType();
+  CXFA_Color* GetColorIfExists();
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_PATTERN_H_
diff --git a/xfa/fxfa/parser/cxfa_radial.cpp b/xfa/fxfa/parser/cxfa_radial.cpp
index 5630943c58..80753b3803 100644
--- a/xfa/fxfa/parser/cxfa_radial.cpp
+++ b/xfa/fxfa/parser/cxfa_radial.cpp
@@ -8,6 +8,7 @@
 
 #include "fxjs/xfa/cjx_radial.h"
 #include "third_party/base/ptr_util.h"
+#include "xfa/fxfa/parser/cxfa_color.h"
 
 namespace {
 
@@ -38,3 +39,13 @@ CXFA_Radial::CXFA_Radial(CXFA_Document* doc, XFA_PacketType packet)
                 pdfium::MakeUnique<CJX_Radial>(this)) {}
 
 CXFA_Radial::~CXFA_Radial() {}
+
+bool CXFA_Radial::IsToEdge() {
+  return JSObject()
+             ->TryEnum(XFA_Attribute::Type, true)
+             .value_or(XFA_AttributeEnum::ToEdge) == XFA_AttributeEnum::ToEdge;
+}
+
+CXFA_Color* CXFA_Radial::GetColorIfExists() {
+  return GetChild<CXFA_Color>(0, XFA_Element::Color, false);
+}
diff --git a/xfa/fxfa/parser/cxfa_radial.h b/xfa/fxfa/parser/cxfa_radial.h
index 2316c459c7..e3a3645e0c 100644
--- a/xfa/fxfa/parser/cxfa_radial.h
+++ b/xfa/fxfa/parser/cxfa_radial.h
@@ -9,10 +9,15 @@
 
 #include "xfa/fxfa/parser/cxfa_node.h"
 
+class CXFA_Color;
+
 class CXFA_Radial : public CXFA_Node {
  public:
   CXFA_Radial(CXFA_Document* doc, XFA_PacketType packet);
   ~CXFA_Radial() override;
+
+  bool IsToEdge();
+  CXFA_Color* GetColorIfExists();
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_RADIAL_H_
diff --git a/xfa/fxfa/parser/cxfa_stipple.cpp b/xfa/fxfa/parser/cxfa_stipple.cpp
index 17b64fa80b..24715171b8 100644
--- a/xfa/fxfa/parser/cxfa_stipple.cpp
+++ b/xfa/fxfa/parser/cxfa_stipple.cpp
@@ -8,6 +8,7 @@
 
 #include "fxjs/xfa/cjx_stipple.h"
 #include "third_party/base/ptr_util.h"
+#include "xfa/fxfa/parser/cxfa_color.h"
 
 namespace {
 
@@ -37,3 +38,13 @@ CXFA_Stipple::CXFA_Stipple(CXFA_Document* doc, XFA_PacketType packet)
                 pdfium::MakeUnique<CJX_Stipple>(this)) {}
 
 CXFA_Stipple::~CXFA_Stipple() {}
+
+CXFA_Color* CXFA_Stipple::GetColorIfExists() {
+  return GetChild<CXFA_Color>(0, XFA_Element::Color, false);
+}
+
+int32_t CXFA_Stipple::GetRate() {
+  return JSObject()
+      ->TryInteger(XFA_Attribute::Rate, true)
+      .value_or(GetDefaultRate());
+}
diff --git a/xfa/fxfa/parser/cxfa_stipple.h b/xfa/fxfa/parser/cxfa_stipple.h
index 2dddf4494d..3211e24d9e 100644
--- a/xfa/fxfa/parser/cxfa_stipple.h
+++ b/xfa/fxfa/parser/cxfa_stipple.h
@@ -9,10 +9,17 @@
 
 #include "xfa/fxfa/parser/cxfa_node.h"
 
+class CXFA_Color;
+
 class CXFA_Stipple : public CXFA_Node {
  public:
+  static int32_t GetDefaultRate() { return 50; }
+
   CXFA_Stipple(CXFA_Document* doc, XFA_PacketType packet);
   ~CXFA_Stipple() override;
+
+  CXFA_Color* GetColorIfExists();
+  int32_t GetRate();
 };
 
 #endif  // XFA_FXFA_PARSER_CXFA_STIPPLE_H_