[Webkit-unassigned] [Bug 11031] Another crazy counters bug
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 19 10:24:17 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=11031
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #43487|review? |review-
Flag| |
--- Comment #27 from Darin Adler <darin at apple.com> 2009-11-19 10:24:16 PST ---
(From update of attachment 43487)
Patch looks good. Quite a few stylistic issues.
I don't know that all of this needs to go in one patch. I'd prefer to see one
fix at a time even though it would take a bit longer. There may be some pieces
that are interdependent, but many seem not to be. To pick just one example, the
change to isReset seems separate from the rest. Similarly, adding comments can
be done separate from behavior changes. And some of the refactoring can be done
in an initial patch before reusing the refactored code in new ways. For
example, destroyCounterNodes could be refactored in a separate patch that does
not change behavior at all.
> + while (m_lastChild != refChild) {
> + RenderCounter::destroyCounterNode(m_lastChild->renderer(), identifier);
> + }
In WebKit coding style this does not have braces.
> - bool isReset() const { return m_isReset; }
> + bool isReset() const { return m_isReset || !m_parent; }
> + bool isTypeReset() const { return m_isReset; }
I don't think these names are clear enough.
The one you call isReset could be have a name like shouldTriggerReset.
It is not good to have an m_isReset and an isReset function that do not match.
At the very least if you think these function names are great and want to keep
them you'd have to rename the data member to m_isTypeReset. But also when
naming function members remember that we want them to work as a sentence like
this:
"<object> <functionName>"
And "counter is type reset" is not clear. Thus you might end up with a name
like hasResetType since "counter has reset type" is not bad.
> + // identifier must match the identifier of this counter or
> + // the renderers affected by this removal shall not be invalidated.
> void removeChild(CounterNode*, const AtomicString& identifier);
This comment seems kind of sideways. I think the comment should say what the
function does, rather than what the function does not do. If renderers are not
invalidated, why is that correct/OK? I think I know the answer but that's what
the comment should be talking about, not just the mechanics of the logic in the
function.
We sometimes use "identifier" and other times "counterName". It would be good
to make the terminology as consistent as possible.
> +// Finds the insertion point for the counter described by counterOwner, isReset and
> +// counterName in the CounterNode tree for counterName and sets parent and
> +// previousSibling accordingly.
> +// The function returns true if the counter whose insertion point is searched is NOT
> +// the root of the tree.
> +// The root of the tree is a counter reference that is not in the scope of any other
> +// counter with the same identifier.
> +// All the counter references with the same identifier as this one that are in
> +// children or subsequent siblings of the renderer that owns the root of the tree
> +// form the rest of of the nodes of the tree.
> +// The root of the tree is always a reset type reference.
> +// A subtree rooted at any reset node in the tree is equivalent to all counter
> +// references that are in the scope of the counter or nested counter defined by that
> +// reset node.
> +// Non-reset CounterNodes cannot have descendants.
Typically we don't put line breaks after sentences in a comment like this.
> -static bool findPlaceForCounter(RenderObject* object, const AtomicString& counterName,
> - bool isReset, CounterNode*& parent, CounterNode*& previousSibling)
> +static bool findPlaceForCounter(RenderObject* counterOwner,
> + const AtomicString& counterName,
> + bool isReset, CounterNode*& parent,
> + CounterNode*& previousSibling)
This kind of formatting, where the subsequent lines are lined up with the "(",
is typically frowned upon in WebKit. Sadly the editors push you to do it this
way. One of the problems with this is that it complicates later efforts to
rename. You have to realign the function. The old version used our formatting
style.
> + // We may be at the end of our search
Comments use sentence style, so typically have a period as well as a capital
letter.
> + if (currentCounter) {
> + // We have a suitable counter on the EndSearchRenderer
> + if (previousSibling) { //But we already found another counter that we come after
We put spaces after the "//".
> + if (currentCounter->isReset()) {
> + // We found a reset counter that is on a renderer that is a sibling of ours or a parent
> + if (isReset
> + && (currentRenderer->parent() == counterOwner->parent())) {
We typically don't use parentheses when && and == are involved in a boolean
expression. Since this idiom is so common in if statements the order of
precedence is clear by convention if nothing else.
> + // We are not a reset node or the previous reset must be on an ancestor of our renderer
> + // hence we must be a child of that reset counter
Another of many examples that lack a period.
> + parent = currentCounter;
> + ASSERT(previousSibling->parent() == currentCounter);
> + return true;
> + } // currentCounter is not reset
This kind of comment on the ending brace does not seem to make the code
clearer. In fact, I find it confusing that it describes the condition on the
code above it. These are not giant if statements, so I think the code is
clearer without these back-reference comments. Maybe you meant this to be a
comment on the code after the brace, in which case it's critical to move it to
the next line.
> + if (!isReset
> + || (currentRenderer->parent() != counterOwner->parent())) {
> + // If the node we are placing is not reset or we have found a counter that is attached
> + // to an ancestor of the placed counter's renderer we know we are a sibling of that node
> + ASSERT(currentCounter->parent() == previousSibling->parent());
> + parent = currentCounter->parent();
> + return true;
> + }
In the other cases like this just above and below you indented the comment to
match the body of the if. Should do that here too.
> + if (isReset
> + && (currentRenderer->parent() == counterOwner->parent())) {
Really seems this expression would read better on one line. And similar ones
above and below. The line break makes it harder to read.
> + } // We come here if the previous sibling or parent of our renderer had no
> + // good counter, or we are a reset node and the counter on the previous sibling
> + // of our renderer was not a reset counter.
> + // Set a new goal for the end of the search.
The comment not be on the same line with the brace.
> + // We found a suitable counter ...
We definitely do not use "..." like this. People can see the structure from the
C++ code, and trying to mirror that with sentence fragments doesn't seem to
make the code clearer.
> + if ( currentCounter->isReset()) {
> + // but this one is reset.
The above is a perfect example of a comment that says exactly the same thing as
the C++ code. It's important to have comments say the things that the C++ code
does not say, rather than repeating what it does say. Comments are good for why
the code does something but should rarely be used for what the code does. There
are other ways to make clear what the code does, including making well-named
local variables and helper functions, that are far more effective.
> + for (CounterMaps::iterator it = counterMaps().begin();it != end;
> + ++it)
Need a space after the semicolon. This should be on one line.
> + if (it->first != object) {
We often find that early continue makes these things better, rather than
nesting the entire loop.
> + nodeMap = it->second;
> + if (CounterNode* node = nodeMap->get(counterName.impl())) {
Same here.
> + if (!node->parent()
> + && findPlaceForCounter(
> + const_cast<RenderObject*>(it->first),
> + counterName, true, newParent,
> + newPreviousSibling)) {
And here too. It's especially poor to write a function call vertically like
this. Makes it hard to read. It would be better to have a named variable for
it->first casted to the right type and put this on one line.
The worst argument here is the "true". That's why we no longer used booleans
for this type of thing. Instead we would have an enum with two named values.
> + newParent->insertAfter(node,
> + newPreviousSibling, counterName.impl());
This should be on one line, not two. Do we still need the impl() call here?
> - CounterMap* map = maps.get(object);
> + CounterMap* map = maps.get(renderer);
> if (!map)
> return;
> - maps.remove(object);
> -
> CounterMap::const_iterator end = map->end();
> for (CounterMap::const_iterator it = map->begin(); it != end; ++it) {
> - CounterNode* node = it->second;
> AtomicString identifier(it->first.get());
> - destroyCounterNodeChildren(identifier, node);
> - if (CounterNode* parent = node->parent())
> - parent->removeChild(node, identifier);
> - delete node;
> + destroyCounterNodeWithoutMapRemoval(identifier, it->second);
> }
Braces go now that the body has one line.
> + maps.remove(renderer);
Now that the get and remove function calls are in the same function, it's not
good to do double hash table lookup. Instead you should use a find call and
check against end to do the "get" part, and call the version of remove that
takes an iterator so we can reuse the single hash table lookup.
> + CounterMaps& maps = counterMaps();
> + CounterMap* map = maps.get(renderer);
Seems unnecessary to put counterMaps() into a local variable in a function that
uses it only one.
> + if (!map)
> + return;
> + CounterNode* node = map->get(identifier.impl());
> + if (node) {
> + destroyCounterNodeWithoutMapRemoval(identifier, node);
> + map->remove(RefPtr<AtomicStringImpl>(identifier.impl()));
I do not thing that typecast to RefPtr<AtomicStringImpl> is need. If it is, I
am quite surprised!
> +// Do not delete map even if empty as the expectation is that soon nodes will be added
> +// to the map, or otherwise destroyCounterNodes would have been called instead.
> + }
This comment is at the wrong nesting level. Also, I think "expectation" is not
right here. This comment needs to treat this as guarantee the caller makes, not
an expectation, using words like "must". It would be even better if later we
can modify the code to ensure this always happens.
> +void RenderCounter::createCounterIfNeeded(RenderObject* renderer,
> + const AtomicString& identifier)
> +{
> + makeCounterNode(renderer, identifier, false);
> +}
The line before the opening brace is indented wrong, should be only one level.
But also, This should just all go on one line.
> +void RenderCounter::updateCounters(RenderObject* renderer)
> +{
> + updateCounter(renderer);
> + for (RenderObject* child = renderer->nextInPreOrder(renderer); child; child = child->nextInPreOrder(renderer))
> + updateCounter(child);
> +}
The variable child is actually any descendant, so child is not the optimal
name. And this can be written like this:
for (RenderObject* descendant = renderer; descendant; descendant =
descendant->nextInPreOrder(renderer))
updateCounter(descendant);
No separate function call outside the loop needed.
> + const CounterDirectiveMap* pCounterDirectiveMap = object->style()->counterDirectives();
We don't use "pXXX" names like this in WebKit code. I also think that you can
just call this directiveMap since the context of the function means "counter"
can often be omitted. It's always a readability/judgement call.
> + if ((newParent == node->parent()) && (newPreviousSibling == node->previousSibling()))
> + continue;
> + if (node->parent())
> + node->parent()->removeChild(node, it->first.get());
Since node->parent() is always used, and used three times, you could consider a
local variable for it.
I suggest omitting the parentheses in the "==" and "&&" expression. We normally
do and I find it slightly harder to read the expression with the additional
punctuation.
> + static void destroyCounterNodes(RenderObject* renderer);
> + static void destroyCounterNode(RenderObject* renderer, const AtomicString& identifier);
> + static void updateCounters(RenderObject* renderer);
> + static void updateCounter(RenderObject* renderer);
> + static void createCounterIfNeeded(RenderObject* renderer, const AtomicString& identifier);
The argument name "renderer" adds nothing for a type that is RenderObject*, so
I suggest omitting it. If there was some way to make clearer the role of the
RenderObject you could include an argument name that did it, but I don't think
there is.
> + RenderCounter::updateCounters(newChild);
Is this extra work that is bad for performance?
> - updateFillImages(oldStyle ? oldStyle->backgroundLayers() : 0, m_style ? m_style->backgroundLayers() : 0);
> - updateFillImages(oldStyle ? oldStyle->maskLayers() : 0, m_style ? m_style->maskLayers() : 0);
> -
> - updateImage(oldStyle ? oldStyle->borderImage().image() : 0, m_style ? m_style->borderImage().image() : 0);
> - updateImage(oldStyle ? oldStyle->maskBoxImage().image() : 0, m_style ? m_style->maskBoxImage().image() : 0);
> + if (oldStyle) {
> + if (m_style) {
> + if (const CounterDirectiveMap* pCounterDirectiveMap = m_style->counterDirectives()) {
> + if (const CounterDirectiveMap* pOldCounterDirectiveMap = oldStyle->counterDirectives()) {
> + CounterDirectiveMap::const_iterator end = pCounterDirectiveMap->end();
> + for (CounterDirectiveMap::const_iterator it = pCounterDirectiveMap->begin(); it != end; ++it) {
> + if (pOldCounterDirectiveMap->contains(it->first)) {
> + if (pOldCounterDirectiveMap->get(it->first) != it->second)
> + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> + } else {
> + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> + RenderCounter::createCounterIfNeeded(this, AtomicString(it->first.get()));
> + }
> + }
> + end = pOldCounterDirectiveMap->end();
> + for (CounterDirectiveMap::const_iterator it = pOldCounterDirectiveMap->begin(); it != end; ++it)
> + if (!pCounterDirectiveMap->contains(it->first))
> + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> + } else {
> + RenderCounter::destroyCounterNodes(this);
> + RenderCounter::updateCounters(this);
> + }
> + } else if (m_hasCounterNodeMap)
> + RenderCounter::destroyCounterNodes(this);
>
> - // We need to ensure that view->maximalOutlineSize() is valid for any repaints that happen
> - // during styleDidChange (it's used by clippedOverflowRectForRepaint()).
> - if (m_style->outlineWidth() > 0 && m_style->outlineSize() > maximalOutlineSize(PaintPhaseOutline))
> - toRenderView(document()->renderer())->setMaximalOutlineSize(m_style->outlineSize());
> + updateFillImages(oldStyle->backgroundLayers(),
> + m_style->backgroundLayers());
> + updateFillImages(oldStyle->maskLayers(), m_style->maskLayers());
> +
> + updateImage(oldStyle->borderImage().image(),
> + m_style->borderImage().image());
> + updateImage(oldStyle->maskBoxImage().image(),
> + m_style->maskBoxImage().image());
> + // We need to ensure that view->maximalOutlineSize() is valid for
> + // any repaints that happen during styleDidChange (it's used by
> + // clippedOverflowRectForRepaint()).
> + if (m_style->outlineWidth() > 0 && m_style->outlineSize() >
> + maximalOutlineSize(PaintPhaseOutline)) {
> + toRenderView(document()->renderer())->setMaximalOutlineSize(
> + m_style->outlineSize());
> + }
> + } else {
> + if (m_hasCounterNodeMap)
> + RenderCounter::destroyCounterNodes(this);
> + updateFillImages(oldStyle->backgroundLayers(), 0);
> + updateFillImages(oldStyle->maskLayers(), 0);
> +
> + updateImage(oldStyle->borderImage().image(), 0);
> + updateImage(oldStyle->maskBoxImage().image(), 0);
> + }
> + } else if (m_style) {
> + RenderCounter::updateCounter(this);
> + updateFillImages(0, m_style->backgroundLayers());
> + updateFillImages(0, m_style->maskLayers());
> +
> + updateImage(0, m_style->borderImage().image());
> + updateImage(0, m_style->maskBoxImage().image());
> + // We need to ensure that view->maximalOutlineSize() is valid for any repaints that happen
> + // during styleDidChange (it's used by clippedOverflowRectForRepaint()).
> + if (m_style->outlineWidth() > 0 && m_style->outlineSize() > maximalOutlineSize(PaintPhaseOutline))
> + toRenderView(document()->renderer())->setMaximalOutlineSize(m_style->outlineSize());
> + }
This new code is long and hard to read and understand. What's the benefit of
putting all the cases in different branches like this? Is there a way we can
factor this so it's not so gigantic? Replacing 4 lines of code with 50 needs a
really good justification. There's also a lot of code broken into two lines
prematurely. We'd prefer to have a moderately long line rather than breaking
everything so it fits in an 80-column character.
> - virtual bool isEmpty() const { return firstChild() == 0; }
> + virtual bool isEmpty() const { return !firstChild(); }
> - SelectionState selectionState() const { return static_cast<SelectionState>(m_selectionState);; }
> + SelectionState selectionState() const { return static_cast<SelectionState>(m_selectionState); }
These changes, unrelated to the rest of the patch, should go in a separate
patch.
Given the amount of change, I'm surprised at the small number of new test
cases.
review- because I want at least some of the style issues fixed. Please consider
all my comments.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list