Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886093002:
Add class level comments to ComputedStyleBase. (Closed)
DescriptionAdd class level comments to ComputedStyleBase.
This patch adds class level comments to ComputedStyleBase.
Review-Url: https://codereview.chromium.org/2886093002
Cr-Commit-Position: refs/heads/master@{#474945}
Committed: https://chromium.googlesource.com/chromium/src/+/b6226310dcb07800431471cdd6ffb36fa1c21925
Patch Set 1 #
Total comments: 19
Patch Set 2 : Rebase #
Total comments: 4
Patch Set 3 : Address comments #
Total comments: 1
Patch Set 4 : Rebase #Messages
Total messages: 25 (17 generated)
nainar@chromium.org changed reviewers: + nainar@chromium.org, suzyh@chromium.org
https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:50: // ComputedStyleBase is a generated class that implements the storage aspect of 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."? https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:55: // However, storing every data member directly in this class is wasteful, since 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. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:60: // StyleBoxData, so different multiple ComputedStyleBases can save memory by "different multiple" is redundant - pick just one word. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:61: // sharing the same instance of StyleBoxData. Furthermore, these nested classes 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. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:66: // ComputedStyleBase 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. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:84: // should never refer to these groups, since they may changed if we find a 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. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:85: // better layout structure. There's no need to specify a reason why they may have changed. Code changes; it is in the nature of an active project! https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:89: // different accessors and helpers. For example, a field with the 'keyword' 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 ..." https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: // ComputedStyle class and no other class is allowed. 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 ...
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Suzy for the very detailed feedback. PTAL again? https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:50: // ComputedStyleBase is a generated class that implements the storage aspect of On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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."? Done. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:55: // However, storing every data member directly in this class is wasteful, since On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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. Rewrote the paragraph. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:60: // StyleBoxData, so different multiple ComputedStyleBases can save memory by On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > "different multiple" is redundant - pick just one word. Done. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:61: // sharing the same instance of StyleBoxData. Furthermore, these nested classes On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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. Omitted discussion of nested classes, since it's an implementation detail. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:66: // ComputedStyleBase On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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. Done. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:84: // should never refer to these groups, since they may changed if we find a On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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. Removed this sentence as it is not worth the confusion. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:85: // better layout structure. On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > There's no need to specify a reason why they may have changed. Code changes; it is in the nature of an active project! Done. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:89: // different accessors and helpers. For example, a field with the 'keyword' On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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 ..." Done. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: // ComputedStyle class and no other class is allowed. On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > 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 ... Added TODO, but no bug as it is a very small change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one required clarification. https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:97: // ComputedStyle class and no other class is allowed. On 2017/05/17 at 23:30:00, shend wrote: > On 2017/05/17 at 09:01:27, suzyh_UTC10 wrote: > > 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 ... > > Added TODO, but no bug as it is a very small change. 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. https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:57: // stores a set of fields and a set of pointers to children nodes (called Nit: I think the usual phrasing would be "child nodes". https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:69: // This design saves memory by allowing multiple groups to share the same Is the sharing by multiple groups within one ComputedStyleBase, or by multiple ComputedStyleBases? Reading this sentence, I thought the former; reading the example, I thought the latter. Please clarify.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:57: // stores a set of fields and a set of pointers to children nodes (called On 2017/05/19 at 02:33:59, suzyh_UTC10 wrote: > Nit: I think the usual phrasing would be "child nodes". Done. https://codereview.chromium.org/2886093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:69: // This design saves memory by allowing multiple groups to share the same On 2017/05/19 at 02:33:59, suzyh_UTC10 wrote: > Is the sharing by multiple groups within one ComputedStyleBase, or by multiple ComputedStyleBases? Reading this sentence, I thought the former; reading the example, I thought the latter. Please clarify. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit https://codereview.chromium.org/2886093002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl (right): https://codereview.chromium.org/2886093002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/build/scripts/templates/ComputedStyleBase.h.tmpl:88: // TODO(nainar): Enforce this in code. This is done now.
The CQ bit was checked by shend@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shend@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org, nainar@chromium.org Link to the patchset: https://codereview.chromium.org/2886093002/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495780544614630,
"parent_rev": "e96260c1487284932117eb66862f7b47efb61ef5", "commit_rev":
"b6226310dcb07800431471cdd6ffb36fa1c21925"}
Message was sent while issue was closed.
Description was changed from ========== Add class level comments to ComputedStyleBase. This patch adds class level comments to ComputedStyleBase. ========== to ========== Add class level comments to ComputedStyleBase. This patch adds class level comments to ComputedStyleBase. Review-Url: https://codereview.chromium.org/2886093002 Cr-Commit-Position: refs/heads/master@{#474945} Committed: https://chromium.googlesource.com/chromium/src/+/b6226310dcb07800431471cdd6ff... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b6226310dcb07800431471cdd6ff... |
