Rename variables in ContainerNode::checkForSiblingStyleChanges() for clarity

Rename variables in ContainerNode::checkForSiblingStyleChanges() for clarity

Rename variables in ContainerNode::checkForSiblingStyleChanges() for clarity
and also update comments.

Variables renaming:
- beforeChange / afterChange to nodeBeforeChange / nodeAfterChange to be clear
  about the type (e.g. not necessary Element nodes)
- newFirstChild / newLastChild to firstChildElement / lastChildElement to be
  clear that those are the first / last child _elements_.
- lastElementBeforeInsertion / firstElementAfterInsertion to
  elementBeforeChange / elementAfterChange as the change is not necessarily an
  insertion (e.g. could be a removal). The naming was confusing.

Also add an ASSERT(changeType != FinishedParsingChildren) assertion to enforce
the precondition explained in the comment for the corresponding scope.

There is no functional change in this CL to make it more easily reviewable.
This CL is purely about improving code readability / understandability.

R=esprehn@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178055

[Inactive, 2014-07-10 01:05:11]

There is no functional change in this CL to make it more easily reviewable. 
This CL is purely about improving code readability / understandability.

I have however made several comments on the CL regarding possible improvement
ideas I thought of while reviewing this code.

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1154: void
ContainerNode::checkForSiblingStyleChanges(SiblingCheckType changeType, Node*
nodeBeforeChange, Node* nodeAfterChange)
Possible improvement idea: it seems like we are calling this method for any kind
of child change (Element insertion / removal, non-Element insertion / removal,
Text change). Based on the types of invalidations, it seems to me that we only
need to cause invalidation in case of _Element_ insertion removal?

For e.g., if childrenAffectedByForwardPositionalRules() is true and this is a
Text child insertion, I believe we are unnecessarily calling
setNeedsStyleRecalc(SubtreeStyleChange) on the parent / container node.

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1170: if
(((childrenAffectedByForwardPositionalRules() ||
childrenAffectedByIndirectAdjacentRules()) && nodeAfterChange)
Possible improvement idea: it seems like we may be causing too much invalidation
here if there are only non-Element nodes after the change.

Ideally, this would be an "&& elementAfterChange" check.

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1171: ||
(childrenAffectedByBackwardPositionalRules() && nodeBeforeChange)) {
Possible improvement idea: it seems like we may be causing too much invalidation
here if there are only non-Element nodes before the change.

Ideally, this would be an "&& elementBeforeChange" check.

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1194: if (changeType == SiblingRemoved &&
firstChildElement == elementAfterChange && firstChildElement &&
(!firstChildElementStyle || !firstChildElementStyle->firstChildState()))
Possible improvement idea: Seems like this could be an "else if".

https://codereview.chromium.org/380073002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:1215: if ((changeType == SiblingRemoved ||
changeType == FinishedParsingChildren) && lastChildElement ==
elementBeforeChange && lastChildElement && (!lastChildElementStyle ||
!lastChildElementStyle->lastChildState()))
Possible improvement idea: Seems like this could be an "else if".

[esprehn, 2014-07-10 01:40:37]

This patch doesn't seem like it's improving anything, you just made it all more
verbose. I don't see how nodeAfterChange is better than afterChange...

[Inactive, 2014-07-10 02:01:55]

On 2014/07/10 01:40:37, esprehn wrote:
> This patch doesn't seem like it's improving anything, you just made it all
more
> verbose. I don't see how nodeAfterChange is better than afterChange...

Hmm, this is the part of the change I feel the less strongly about. But since we
are dealing with both Element and Node types in this method, I just thought it
would be nice to differentiate.

What triggered this CL was really the firstElementAfterInsertion /
lastElementBeforeInsertion variables naming which is very confusing IMHO as the
change is not necessarily an insertion and could very well be a removal.

I'd love to hear your opinion regarding my possible improvement ideas (in
comments) when you get a chance.

[esprehn, 2014-07-14 08:45:20]

On 2014/07/10 at 02:01:55, ch.dumez wrote:
> On 2014/07/10 01:40:37, esprehn wrote:
> > This patch doesn't seem like it's improving anything, you just made it all
more
> > verbose. I don't see how nodeAfterChange is better than afterChange...
> 
> Hmm, this is the part of the change I feel the less strongly about. But since
we are dealing with both Element and Node types in this method, I just thought
it would be nice to differentiate.
> 
> What triggered this CL was really the firstElementAfterInsertion /
lastElementBeforeInsertion variables naming which is very confusing IMHO as the
change is not necessarily an insertion and could very well be a removal.

Yeah the AfterChange renaming seems fine, lgtm to land as is. I don't want to
fight over the bike yet, especially not if you're going to fix the bug where we
call into this for non-Element changes. :)

[Inactive, 2014-07-14 13:23:46]

On 2014/07/14 08:45:20, esprehn wrote:
> On 2014/07/10 at 02:01:55, ch.dumez wrote:
> > On 2014/07/10 01:40:37, esprehn wrote:
> > > This patch doesn't seem like it's improving anything, you just made it all
> more
> > > verbose. I don't see how nodeAfterChange is better than afterChange...
> > 
> > Hmm, this is the part of the change I feel the less strongly about. But
since
> we are dealing with both Element and Node types in this method, I just thought
> it would be nice to differentiate.
> > 
> > What triggered this CL was really the firstElementAfterInsertion /
> lastElementBeforeInsertion variables naming which is very confusing IMHO as
the
> change is not necessarily an insertion and could very well be a removal.
> 
> Yeah the AfterChange renaming seems fine, lgtm to land as is. I don't want to
> fight over the bike yet, especially not if you're going to fix the bug where
we
> call into this for non-Element changes. :)

Ok thanks. Yes, this is the plan.

[Inactive, 2014-07-14 13:23:52]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-07-14 13:24:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/380073002/1

[commit-bot: I haz the power, 2014-07-14 14:00:46]

Change committed as 178055