Add class level comments to ComputedStyleBase.

third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl

 {% from 'macros.tmpl' import license, print_if %}
 {% from 'fields/field.tmpl' import encode, getter_expression, setter_expression, declare_storage, fieldwise_compare, fieldwise_copy, fieldwise_diff %}
 {% from 'fields/group.tmpl' import define_field_group_class %}
 {{license()}}
 
 #ifndef ComputedStyleBase_h
 #define ComputedStyleBase_h
 
 #include "core/style/ComputedStyleConstants.h"
 #include "core/CoreExport.h"
@@ skipping 28 matching lines @@
   void* dataRefs[{{computed_style.subgroups|length}}];
   {% endif %}
   {% for field in computed_style.fields|rejectattr("is_bit_field") %}
   {{field.type_name}} {{field.name}};
   {% endfor %}
   unsigned m_bit_fields[{{computed_style.num_32_bit_words_for_bit_fields}}];
 };
 
 // The generated portion of ComputedStyle. For more info, see the header comment
 // in ComputedStyle.h.
-
-// ComputedStyleBase is a templated class to allow it to use functions
-// on ComputedStyle. This allows ComputedStyleBase to use hand written 
-// functions it would otherwise not know about.
-// It should only be templated with the ComputedStyle class and no other class
-// is allowed.
+//
+// ComputedStyleBase is a generated class that implements the storage aspect of

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

I find this paragraph unclear, I think particularly "implements the storage
aspect of ComputedStyle". Do you need this sentence, or could you start with
simply "ComputedStyleBase is a generated class that stores a set of data members
or 'fields' that are used in ComputedStyle."?

[shend, 2017/05/17 23:30:00]

Done.

+// ComputedStyle. Conceptually, it stores a set of data members or 'fields'
+// that are used in ComputedStyle. These fields can represent CSS properties or
+// internal style information.
+//
+// However, storing every data member directly in this class is wasteful, since

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

On a first read through, it's not clear why this is wasteful.

- You say "not every data member is used frequently", but I don't see why groups
are any more efficient, since presumably they are not going to be used any more
frequently than their components.
- You also say it allows sharing of an instance of a group.

I think these may be part of the same reason, but it's not obvious to me.

Also, in what way is it wasteful? Memory? Compute time?

I think the right thing to do here is to rewrite this paragraph and compare the
memory/time/whatever cost of this approach with the obvious alternatives, in
very general terms. If the explanation gets too long or involved, though, put it
in a separate doc and link to the doc from here instead.

[shend, 2017/05/17 23:30:00]

Rewrote the paragraph.

+// not every data member is used frequently. Thus, we group fields into
+// nested classes inside ComputedStyleBase and store pointers to
+// these groups instead. For example, 'width' and 'height' are stored in a group
+// called StyleBoxData. ComputedStyleBase stores a pointer to an instance of
+// StyleBoxData, so different multiple ComputedStyleBases can save memory by

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

"different multiple" is redundant - pick just one word.

[shend, 2017/05/17 23:30:00]

Done.

+// sharing the same instance of StyleBoxData. Furthermore, these nested classes

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

I think put a paragraph break here and have the nested classes point as its own
paragraph. It depends how you rewrite the earlier part of the paragraph, though.

[shend, 2017/05/17 23:30:00]

Omitted discussion of nested classes, since it's an implementation detail.

+// can themselves contain nested classes and so on, meaning we can view the
+// storage of ComputedStyleBase as a tree structure of groups with
+// ComputedStyleBase at the root:
+//
+// ComputedStyleBase

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

I wouldn't bother listing all of these groups, especially since you say below
that they may have changed. List one or two at each level, with ellipses to show
that the lists are incomplete.

[shend, 2017/05/17 23:30:00]

Done.

+//  |- StyleSurroundData
+//  |- StyleBackgroundData
+//  |- StyleVisualData
+//  |- StyleBoxData
+//  |- StyleInheritedData
+//  |- StyleRareInheritedData
+//  |- StyleRareNonInheritedData
+//      |- StyleDeprecatedFlexibleBoxData
+//      |- StyleFlexibleBoxData
+//      |- StyleMultiColData
+//      |- StyleTransformData
+//      |- StyleWillChangeData
+//      |- StyleGridData
+//      |- StyleGridItemData
+//      |- StyleScrollSnapData
+//
+// where every field belongs to exactly one of these groups. ComputedStyle
+// should never refer to these groups, since they may changed if we find a

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

I misinterpreted this sentence the first time through. I thought you were saying
that ComputedStyleBASE should never refer to these groups, and was going to
suggest adding "by name".

Now that I realise that you are talking about the plain ComputedStyle not
referring to these groups, I'm not sure if you mean that ComputedStyle should
not refer to these groups by name (but is aware that the groups exist) or that
ComputedStyle should be oblivious to the existence of the groups at all.

Can you clarify what you mean here?

I am also probably not the only person who will gloss over that you're talking
about the subclass here, since (aside from the opening sentences), this text is
entirely about ComputedStyleBase. Please draw a little more attention to the
change.

[shend, 2017/05/17 23:30:00]

Removed this sentence as it is not worth the confusion.

+// better layout structure.

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

There's no need to specify a reason why they may have changed. Code changes; it
is in the nature of an active project!

[shend, 2017/05/17 23:30:00]

Done.

+//
+// Not every field has the same interface. The interface for a field is
+// determined by its 'template'. Different field templates result in slightly
+// different accessors and helpers. For example, a field with the 'keyword'

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

I don't think it's necessary to specify "Different field templates result in
slightly different accessors and helpers."

If you're concerned about using the term "interface" but including private
helper functions in the discussion, how about something like:
"The functions generated for a given field depend on its 'template'. For example
... A list of the available templates ..."

[shend, 2017/05/17 23:30:00]

Done.

+// template has only one setter, whereas an 'external' field has an extra setter
+// that takes an rvalue reference. A list of the available templates can be
+// found in CSSProperties.json5.
+//
+// ComputedStyleBase is a template class to allow it to use functions on
+// ComputedStyle. This allows ComputedStyleBase to use hand written functions it
+// would otherwise not know about.  It should only be templated with the
+// ComputedStyle class and no other class is allowed.

[suzyh_UTC10 (ex-contributor), 2017/05/17 09:01:27]

Consider adding a TODO (ideally with bug reference*) to capture what you were
saying the other day about ideally enforcing this in code (by an assert,
presubmit check, test, or what have you).


(*) That is,
// TODO(crbug.com/XXXXX): Enforce ...

[shend, 2017/05/17 23:30:00]

Added TODO, but no bug as it is a very small change.

[suzyh_UTC10 (ex-contributor), 2017/05/19 02:33:59]

The main reason to add a bug is not because a TODO is a large change, but to
make sure that we don't lose track of the fact that we need to do it, can
consider it during quarterly planning, etc. It's not essential, however.

 template <class ComputedStyleFinal>
 class CORE_EXPORT ComputedStyleBase {
  public:
   inline bool IndependentInheritedEqual(const ComputedStyleBase& o) const {
     return (
         {{fieldwise_compare(computed_style, computed_style.all_fields
             |selectattr("is_property")
             |selectattr("is_inherited")
             |selectattr("is_independent")
             |list
@@ skipping 121 matching lines @@
 
  private:
   {% for field in computed_style.fields %}
   {{declare_storage(field)}}
   {% endfor %}
 };
 
 } // namespace blink
 
 #endif // ComputedStyleBase_h