Move border-*-width out of BorderValue and store on SurroundData in ComputedStyle instead

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

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2003, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
  * rights reserved.
  * Copyright (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 276 matching lines @@
   void SetStyleType(PseudoId style_type) { SetStyleTypeInternal(style_type); }
 
   ComputedStyle* GetCachedPseudoStyle(PseudoId) const;
   ComputedStyle* AddCachedPseudoStyle(PassRefPtr<ComputedStyle>);
   void RemoveCachedPseudoStyle(PseudoId);
 
   const PseudoStyleCache* CachedPseudoStyles() const {
     return cached_pseudo_styles_.get();
   }
 
+  bool BorderWidthEquals(float border_width_first,
+                         float border_width_second) const {
+    return WidthToFixedPoint(border_width_first) ==
+           WidthToFixedPoint(border_width_second);
+  }
+
   /**
      * ComputedStyle properties
      *
      * Each property stored in ComputedStyle is made up of fields. Fields have
      * initial value functions, getters and setters. A field is preferably a
      * basic data type or enum, but can be any type. A set of fields should be
      * preceded by the property the field is stored for.
      *
      * Field method naming should be done like so:
      *   // name-of-property
@@ skipping 188 matching lines @@
   const BorderImageLengthBox& BorderImageOutset() const {
     return surround_data_->border_.GetImage().Outset();
   }
   void SetBorderImageOutset(const BorderImageLengthBox&);
 
   // Border width properties.
   static float InitialBorderWidth() { return 3; }
 
   // border-top-width
   float BorderTopWidth() const {
-    return surround_data_->border_.BorderTopWidth();
+    if (surround_data_->border_.top_.Style() == kBorderStyleNone ||
+        surround_data_->border_.top_.Style() == kBorderStyleHidden)
+      return 0;
+    return static_cast<float>(BorderTopWidthInternal()) /
+           kBorderWidthDenominator;
   }
   void SetBorderTopWidth(float v) {
-    SET_BORDER_WIDTH(surround_data_, border_.top_, v);
+    surround_data_.Access()->border_top_width_ = WidthToFixedPoint(v);
   }
 
   // border-bottom-width
   float BorderBottomWidth() const {
-    return surround_data_->border_.BorderBottomWidth();
+    if (surround_data_->border_.bottom_.Style() == kBorderStyleNone ||
+        surround_data_->border_.bottom_.Style() == kBorderStyleHidden)
+      return 0;
+    return static_cast<float>(BorderBottomWidthInternal()) /
+           kBorderWidthDenominator;
   }
   void SetBorderBottomWidth(float v) {
-    SET_BORDER_WIDTH(surround_data_, border_.bottom_, v);
+    surround_data_.Access()->border_bottom_width_ = WidthToFixedPoint(v);
   }
 
   // border-left-width
   float BorderLeftWidth() const {
-    return surround_data_->border_.BorderLeftWidth();
+    if (surround_data_->border_.left_.Style() == kBorderStyleNone ||
+        surround_data_->border_.left_.Style() == kBorderStyleHidden)
+      return 0;
+    return static_cast<float>(BorderLeftWidthInternal()) /
+           kBorderWidthDenominator;
   }
   void SetBorderLeftWidth(float v) {
-    SET_BORDER_WIDTH(surround_data_, border_.left_, v);
+    surround_data_.Access()->border_left_width_ = WidthToFixedPoint(v);
   }
 
   // border-right-width
   float BorderRightWidth() const {
-    return surround_data_->border_.BorderRightWidth();
+    if (surround_data_->border_.right_.Style() == kBorderStyleNone ||
+        surround_data_->border_.right_.Style() == kBorderStyleHidden)
+      return 0;
+    return static_cast<float>(BorderRightWidthInternal()) /

[shend, 2017/05/07 23:08:31]

The fixed point stuff feels like it should be a class. Maybe as future work, we
could make a FixedPoint class that handles this stuff in a generic way?

[nainar, 2017/05/08 00:59:57]

Yes. Added a todo here on me to take a look at this next.

+           kBorderWidthDenominator;
   }
   void SetBorderRightWidth(float v) {
-    SET_BORDER_WIDTH(surround_data_, border_.right_, v);
+    surround_data_.Access()->border_right_width_ = WidthToFixedPoint(v);
   }
 
   // Border style properties.
   static EBorderStyle InitialBorderStyle() { return kBorderStyleNone; }
 
   // border-top-style
   EBorderStyle BorderTopStyle() const {
     return surround_data_->border_.Top().Style();
   }
   void SetBorderTopStyle(EBorderStyle v) {
@@ skipping 2303 matching lines @@
     return BorderImage().HasImage() && BorderImage().Outset().NonZero();
   }
   LayoutRectOutsets BorderImageOutsets() const {
     return ImageOutsets(BorderImage());
   }
   bool BorderImageSlicesFill() const { return Border().GetImage().Fill(); }
 
   void SetBorderImageSlicesFill(bool);
   const BorderData& Border() const { return surround_data_->border_; }
   const BorderValue& BorderLeft() const {
-    return surround_data_->border_.Left();
+    BorderValue* border_value =

[shend, 2017/05/07 23:08:30]

Have we measured the perf impact of this dynamic allocation?
Also, I'm not familiar with Blink GC magic, but isn't this a memory leak?
Could we just do:

BorderValue BorderLeft() const {
  return BorderValue(surround_data_->border_.Left(), BorderLeftWidth());
}

[nainar, 2017/05/08 00:59:57]

Done.

+        new BorderValue(surround_data_->border_.Left(), BorderLeftWidth());
+    return *border_value;
   }
   const BorderValue& BorderRight() const {
-    return surround_data_->border_.Right();
+    BorderValue* border_value =
+        new BorderValue(surround_data_->border_.Right(), BorderRightWidth());
+    return *border_value;
   }
-  const BorderValue& BorderTop() const { return surround_data_->border_.Top(); }
+  const BorderValue& BorderTop() const {
+    BorderValue* border_value =
+        new BorderValue(surround_data_->border_.Top(), BorderTopWidth());
+    return *border_value;
+  }
   const BorderValue& BorderBottom() const {
-    return surround_data_->border_.Bottom();
+    BorderValue* border_value =
+        new BorderValue(surround_data_->border_.Bottom(), BorderBottomWidth());
+    return *border_value;
+  }
+  bool BorderSizeEquals(const ComputedStyle& o) const {
+    return BorderWidthEquals(BorderLeftWidth(), o.BorderLeftWidth()) &&
+           BorderWidthEquals(BorderTopWidth(), o.BorderTopWidth()) &&
+           BorderWidthEquals(BorderRightWidth(), o.BorderRightWidth()) &&
+           BorderWidthEquals(BorderBottomWidth(), o.BorderBottomWidth());
   }
   const BorderValue& BorderBefore() const;
   const BorderValue& BorderAfter() const;
   const BorderValue& BorderStart() const;
   const BorderValue& BorderEnd() const;
   float BorderAfterWidth() const;
   float BorderBeforeWidth() const;
   float BorderEndWidth() const;
   float BorderStartWidth() const;
   float BorderOverWidth() const;
   float BorderUnderWidth() const;
 
   bool HasBorderFill() const { return Border().HasBorderFill(); }
-  bool HasBorder() const { return Border().HasBorder(); }
+  bool HasBorder() const {
+    return Border().HasBorder() || BorderLeftWidth() || BorderRightWidth() ||
+           BorderTopWidth() || BorderBottomWidth();
+  }
   bool HasBorderDecoration() const { return HasBorder() || HasBorderFill(); }
   bool HasBorderRadius() const {
     if (!BorderTopLeftRadius().Width().IsZero())
       return true;
     if (!BorderTopRightRadius().Width().IsZero())
       return true;
     if (!BorderBottomLeftRadius().Width().IsZero())
       return true;
     if (!BorderBottomRightRadius().Width().IsZero())
       return true;
@@ skipping 15 matching lines @@
     ResetBorderTop();
     ResetBorderRight();
     ResetBorderBottom();
     ResetBorderLeft();
     ResetBorderTopLeftRadius();
     ResetBorderTopRightRadius();
     ResetBorderBottomLeftRadius();
     ResetBorderBottomRightRadius();
   }
   void ResetBorderTop() {
-    SET_VAR(surround_data_, border_.top_, BorderValue());
+    SET_VAR(surround_data_, border_.top_, BorderColorAndStyle());

[nainar, 2017/05/05 07:16:50]

hmm on second thought this should probably also reset BorderTopWidth.

Best to do it in this function or in all callsites?

[shend, 2017/05/07 23:08:31]

Hmm, good question. I would do it here, since BorderLeft() returns a BorderValue
(color + style + width), I would expect ResetBorderLeft() to reset all three
things.

[nainar, 2017/05/08 00:59:57]

Yup. Agreed. Done.

   }
   void ResetBorderRight() {
-    SET_VAR(surround_data_, border_.right_, BorderValue());
+    SET_VAR(surround_data_, border_.right_, BorderColorAndStyle());
   }
   void ResetBorderBottom() {
-    SET_VAR(surround_data_, border_.bottom_, BorderValue());
+    SET_VAR(surround_data_, border_.bottom_, BorderColorAndStyle());
   }
   void ResetBorderLeft() {
-    SET_VAR(surround_data_, border_.left_, BorderValue());
+    SET_VAR(surround_data_, border_.left_, BorderColorAndStyle());
   }
   void ResetBorderImage() {
     SET_VAR(surround_data_, border_.image_, NinePieceImage());
   }
 
   void SetBorderRadius(const LengthSize& s) {
     SetBorderTopLeftRadius(s);
     SetBorderTopRightRadius(s);
     SetBorderBottomLeftRadius(s);
     SetBorderBottomRightRadius(s);
@@ skipping 751 matching lines @@
       PseudoBitsInternal() | 1 << (pseudo - kFirstPublicPseudoId)));
 }
 
 inline bool ComputedStyle::HasPseudoElementStyle() const {
   return PseudoBitsInternal() & kElementPseudoIdMask;
 }
 
 }  // namespace blink
 
 #endif  // ComputedStyle_h