Use generated getters/setters in ComputedStyle instead of directly accessing fields

third_party/WebKit/Source/core/style/ComputedStyle.cpp

 /*
  * Copyright (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 327 matching lines @@
   background_ = other.background_;
   surround_ = other.surround_;
   rare_non_inherited_data_ = other.rare_non_inherited_data_;
 
   // The flags are copied one-by-one because they contain
   // bunch of stuff other than real style data.
   // See comments for each skipped flag below.
 
   // These are not generated in ComputedStyleBase
   SetOriginalDisplay(other.OriginalDisplay());
-  SetVerticalAlign(other.VerticalAlign());
+  SetVerticalAlignInternal(other.VerticalAlignInternal());

[shend, 2017/04/19 06:17:30]

This is an interesting case. Both the previous and new code work.
I think we need to make a decision about which getter/setter to call when there
is a public getter that's the same as the internal getter.

IMO, we should only use internal getters/setters as a last resort, but I don't
mind as long as there is a consistent rule.

[nainar, 2017/04/19 06:31:14]

Replied in the next comment.Also done. 

   SetHasViewportUnits(other.HasViewportUnits());
-  has_rem_units_ = other.HasRemUnits();
+  SetHasRemUnitsInternal(other.HasRemUnits());
 
   // Correctly set during selector matching:
   // m_styleType
   // m_pseudoBits
 
   // Set correctly while computing style for children:
   // m_explicitInheritance
 
   // unique() styles are not cacheable.
   DCHECK(!other.Unique());
@@ skipping 112 matching lines @@
 
 bool ComputedStyle::LoadingCustomFontsEqual(const ComputedStyle& other) const {
   return GetFont().LoadingCustomFonts() == other.GetFont().LoadingCustomFonts();
 }
 
 bool ComputedStyle::NonInheritedEqual(const ComputedStyle& other) const {
   // compare everything except the pseudoStyle pointer
   return ComputedStyleBase::NonInheritedEqual(other) &&
          OriginalDisplay() ==
              other.OriginalDisplay() &&  // Not generated in ComputedStyleBase
-         VerticalAlign() ==
-             other.VerticalAlign() &&  // Not generated in ComputedStyleBase
+         VerticalAlignInternal() ==
+             other.VerticalAlignInternal() &&  // Not generated in
+                                               // ComputedStyleBase
          box_ == other.box_ &&
          visual_ == other.visual_ && background_ == other.background_ &&
          surround_ == other.surround_ &&
          rare_non_inherited_data_ == other.rare_non_inherited_data_ &&
          svg_style_->NonInheritedEqual(*other.svg_style_);
 }
 
 bool ComputedStyle::InheritedDataShared(const ComputedStyle& other) const {
   // This is a fast check that only looks if the data structures are shared.
   // TODO(sashab): Should ComputedStyleBase have an inheritedDataShared method?
@@ skipping 348 matching lines @@
 bool ComputedStyle::DiffNeedsFullLayout(const ComputedStyle& other) const {
   if (box_.Get() != other.box_.Get()) {
     if (box_->Width() != other.box_->Width() ||
         box_->MinWidth() != other.box_->MinWidth() ||
         box_->MaxWidth() != other.box_->MaxWidth() ||
         box_->Height() != other.box_->Height() ||
         box_->MinHeight() != other.box_->MinHeight() ||
         box_->MaxHeight() != other.box_->MaxHeight())
       return true;
 
-    if (box_->VerticalAlign() != other.box_->VerticalAlign())
+    if (box_->vertical_align_ != other.box_->vertical_align_)
       return true;
 
     if (box_->BoxSizing() != other.box_->BoxSizing())
       return true;
   }
 
-  if (VerticalAlign() != other.VerticalAlign() ||
+  if (VerticalAlignInternal() != other.VerticalAlignInternal() ||
       GetPosition() != other.GetPosition())
     return true;
 
   if (rare_non_inherited_data_.Get() != other.rare_non_inherited_data_.Get()) {
     if (rare_non_inherited_data_->align_content_ !=
             other.rare_non_inherited_data_->align_content_ ||
         rare_non_inherited_data_->align_items_ !=
             other.rare_non_inherited_data_->align_items_ ||
         rare_non_inherited_data_->align_self_ !=
             other.rare_non_inherited_data_->align_self_ ||
@@ skipping 183 matching lines @@
       diff.SetNeedsRecomputeOverflow();
   }
 
   if (!surround_->border_.VisualOverflowEqual(other.surround_->border_))
     diff.SetNeedsRecomputeOverflow();
 
   if (!diff.NeedsFullPaintInvalidation()) {
     if (style_inherited_data_->color != other.style_inherited_data_->color ||
         style_inherited_data_->visited_link_color !=
             other.style_inherited_data_->visited_link_color ||
-        has_simple_underline_ != other.has_simple_underline_ ||
+        HasSimpleUnderlineInternal() != other.HasSimpleUnderlineInternal() ||
         visual_->text_decoration != other.visual_->text_decoration) {
       diff.SetTextDecorationOrColorChanged();
     } else if (rare_non_inherited_data_.Get() !=
                    other.rare_non_inherited_data_.Get() &&
                (rare_non_inherited_data_->text_decoration_style_ !=
                     other.rare_non_inherited_data_->text_decoration_style_ ||
                 rare_non_inherited_data_->text_decoration_color_ !=
                     other.rare_non_inherited_data_->text_decoration_color_ ||
                 rare_non_inherited_data_->visited_link_text_decoration_color_ !=
                     other.rare_non_inherited_data_
@@ skipping 630 matching lines @@
   return GetFontDescription().HasSizeAdjust();
 }
 FontWeight ComputedStyle::GetFontWeight() const {
   return GetFontDescription().Weight();
 }
 FontStretch ComputedStyle::GetFontStretch() const {
   return GetFontDescription().Stretch();
 }
 
 TextDecoration ComputedStyle::TextDecorationsInEffect() const {
-  if (has_simple_underline_)
+  if (HasSimpleUnderlineInternal())
     return kTextDecorationUnderline;
   if (!rare_inherited_data_->applied_text_decorations)
     return kTextDecorationNone;
 
   int decorations = 0;
 
   const Vector<AppliedTextDecoration>& applied = AppliedTextDecorations();
 
   for (size_t i = 0; i < applied.size(); ++i)
     decorations |= applied[i].Lines();
 
   return static_cast<TextDecoration>(decorations);
 }
 
 const Vector<AppliedTextDecoration>& ComputedStyle::AppliedTextDecorations()
     const {
-  if (has_simple_underline_) {
+  if (HasSimpleUnderlineInternal()) {
     DEFINE_STATIC_LOCAL(
         Vector<AppliedTextDecoration>, underline,
         (1, AppliedTextDecoration(
                 kTextDecorationUnderline, kTextDecorationStyleSolid,
                 VisitedDependentColor(CSSPropertyTextDecorationColor))));
     // Since we only have one of these in memory, just update the color before
     // returning.
     underline.at(0).SetColor(
         VisitedDependentColor(CSSPropertyTextDecorationColor));
     return underline;
@@ skipping 266 matching lines @@
   if (!list->HasOneRef())
     list = list->Copy();
 
   for (size_t i = 0; i < list->size(); ++i)
     list->at(i).SetColor(override_color);
 }
 
 void ComputedStyle::ApplyTextDecorations(
     const Color& parent_text_decoration_color,
     bool override_existing_colors) {
-  if (GetTextDecoration() == kTextDecorationNone && !has_simple_underline_ &&
+  if (GetTextDecoration() == kTextDecorationNone &&
+      !HasSimpleUnderlineInternal() &&
       !rare_inherited_data_->applied_text_decorations)
     return;
 
   // If there are any color changes or decorations set by this element, stop
-  // using m_hasSimpleUnderline.
+  // using m_hasSimpleUnderlineInternal.
   Color current_text_decoration_color =
       VisitedDependentColor(CSSPropertyTextDecorationColor);
-  if (has_simple_underline_ &&
+  if (HasSimpleUnderlineInternal() &&
       (GetTextDecoration() != kTextDecorationNone ||
        current_text_decoration_color != parent_text_decoration_color)) {
-    has_simple_underline_ = false;
+    SetHasSimpleUnderlineInternal(false);
     AddAppliedTextDecoration(AppliedTextDecoration(
         kTextDecorationUnderline, kTextDecorationStyleSolid,
         parent_text_decoration_color));
   }
   if (override_existing_colors &&
       rare_inherited_data_->applied_text_decorations)
     OverrideTextDecorationColors(current_text_decoration_color);
   if (GetTextDecoration() == kTextDecorationNone)
     return;
-  DCHECK(!has_simple_underline_);
+  DCHECK(!HasSimpleUnderlineInternal());
   // To save memory, we don't use AppliedTextDecoration objects in the common
   // case of a single simple underline of currentColor.
   TextDecoration decoration_lines = GetTextDecoration();
   TextDecorationStyle decoration_style = GetTextDecorationStyle();
   bool is_simple_underline = decoration_lines == kTextDecorationUnderline &&
                              decoration_style == kTextDecorationStyleSolid &&
                              TextDecorationColor().IsCurrentColor();
   if (is_simple_underline && !rare_inherited_data_->applied_text_decorations) {
-    has_simple_underline_ = true;
+    SetHasSimpleUnderlineInternal(true);
     return;
   }
 
   AddAppliedTextDecoration(AppliedTextDecoration(
       decoration_lines, decoration_style, current_text_decoration_color));
 }
 
 void ComputedStyle::ClearAppliedTextDecorations() {
-  has_simple_underline_ = false;
+  SetHasSimpleUnderlineInternal(false);
 
   if (rare_inherited_data_->applied_text_decorations)
     rare_inherited_data_.Access()->applied_text_decorations = nullptr;
 }
 
 void ComputedStyle::RestoreParentTextDecorations(
     const ComputedStyle& parent_style) {
-  has_simple_underline_ = parent_style.has_simple_underline_;
+  SetHasSimpleUnderlineInternal(parent_style.HasSimpleUnderlineInternal());
   if (rare_inherited_data_->applied_text_decorations !=
       parent_style.rare_inherited_data_->applied_text_decorations)
     rare_inherited_data_.Access()->applied_text_decorations =
         parent_style.rare_inherited_data_->applied_text_decorations;
 }
 
 void ComputedStyle::ClearMultiCol() {
   rare_non_inherited_data_.Access()->multi_col_ = nullptr;
   rare_non_inherited_data_.Access()->multi_col_.Init();
 }
@@ skipping 379 matching lines @@
   edges[kBSBottom] = BorderEdge(
       BorderBottomWidth(), VisitedDependentColor(CSSPropertyBorderBottomColor),
       BorderBottomStyle(), horizontal || include_logical_right_edge);
 
   edges[kBSLeft] = BorderEdge(
       BorderLeftWidth(), VisitedDependentColor(CSSPropertyBorderLeftColor),
       BorderLeftStyle(), !horizontal || include_logical_left_edge);
 }
 
 void ComputedStyle::CopyChildDependentFlagsFrom(const ComputedStyle& other) {
-  SetEmptyState(other.EmptyState());
+  SetEmptyStateInternal(other.EmptyStateInternal());

[shend, 2017/04/19 06:17:30]

Here is the problem with using internal getters. I don't think this code does
the same thing. Previously, we would have called SetUnique from the
SetEmptyState, but now we don't. So internal setters don't maintain class
invariants, which could subtly break code.

Surprisingly, tests don't break...

[nainar, 2017/04/19 06:31:14]

Yup, I see what you mean. Using public getters is best when they are available.
Made the change

Also yeah at lack of test coverage.

   if (other.HasExplicitlyInheritedProperties())
     SetHasExplicitlyInheritedProperties();
 }
 
 bool ComputedStyle::ShadowListHasCurrentColor(const ShadowList* shadow_list) {
   if (!shadow_list)
     return false;
   for (size_t i = shadow_list->Shadows().size(); i--;) {
     if (shadow_list->Shadows()[i].GetColor().IsCurrentColor())
       return true;
@@ skipping 26 matching lines @@
     if (value < 0)
       fvalue -= 0.5f;
     else
       fvalue += 0.5f;
   }
 
   return RoundForImpreciseConversion<int>(fvalue / zoom_factor);
 }
 
 }  // namespace blink