Bug 106929

Created attachment 182813 [details] Patch

Comment on attachment 182813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review Cool, this is a nice cleanup and lets us do more powerful traversals (like bug 106792 is trying to do). Kenneth/Julien, does this look good? > Source/WebCore/rendering/TextAutosizer.cpp:299 > + (renderer == container) ? NormalTraversal : SkipDescendantsOfContainers); Nit: WebKit doesn't often wrap long lines (except comments). Perhaps break this into two lines, where the first line sets a temporary |traversalMode| variable? > Source/WebCore/rendering/TextAutosizer.h:60 > + enum TraversalMode { Nit: perhaps TraversalFilter would be clearer? Don't mind, your call...

Comment on attachment 182813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). Please change this to e.g. "Existing tests confirm that there is no change in functionality." to confirm that you have thought about this.

Comment on attachment 182813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review I would need more context to understand how this patch helps in any ways. Adding an extra parameter doesn't seem to be much of a win compared keeping separated methods as it makes the code easier to misread or misuse but also makes the code less readable (more parameters and your traversal function becomes a swiss-knife function) >> Source/WebCore/rendering/TextAutosizer.cpp:299 >> + (renderer == container) ? NormalTraversal : SkipDescendantsOfContainers); > > Nit: WebKit doesn't often wrap long lines (except comments). Perhaps break this into two lines, where the first line sets a temporary |traversalMode| variable? This seems to be a change in behavior as you could skip descendants where you didn't before: you will only skip descendants if renderer == container && isAutosizingContainer(renderer) where it used to be only isAutosizingContainer(renderer) > Source/WebCore/rendering/TextAutosizer.cpp:-366 > - if (current == stayWithin || !isAutosizingContainer(current)) You removed the stayWithin check which means that you can potentially continue iterating if (current == stayWithin) > Source/WebCore/rendering/TextAutosizer.cpp:367 > + bool skipChildren = (mode == SkipDescendants) || (mode == SkipDescendantsOfContainers && isAutosizingContainer(current)); Per our style guide: Precede boolean values with words like "is" and "did". I think |shouldSkipChildren| would be more readable. > Source/WebCore/rendering/TextAutosizer.cpp:371 > + RenderObject* child = current->firstChild(); > + if (child) if (RenderObject* child = current->firstChild()) return child; > Source/WebCore/rendering/TextAutosizer.cpp:379 > + RenderObject* sibling = ancestor->nextSibling(); > + if (sibling) Again it should be on one line. > Source/WebCore/rendering/TextAutosizer.h:62 > + SkipDescendants, This new mode is not explained anywhere nor used. I would be more supportive to postpone adding it until it's actually used.

Comment on attachment 182813 [details] Patch Why are you doing this? I don’t agree that this is a step in the right direction. This is makes render tree traversal less like the traversal we do for the DOM tree. But instead we want it to be more like the DOM tree traversal. I really like the work Antti has done in NodeTraversal.h and I’d like to see our render tree traversal use similar patterns.

(In reply to comment #4) > I would need more context to understand how this patch helps in any ways. Adding an extra parameter doesn't seem to be much of a win compared keeping separated methods as it makes the code easier to misread or misuse but also makes the code less readable (more parameters and your traversal function becomes a swiss-knife function) I agree with this 100%.

Comment on attachment 182813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182813&action=review (In reply to comment #5) > This is makes render tree traversal less like the traversal we do for the DOM tree. But instead we want it to be more like the DOM tree traversal. I really like the work Antti has done in NodeTraversal.h and I’d like to see our render tree traversal use similar patterns. There may have been a slight mis-communication: this wasn't modifying the core render tree traversal functions defined in RenderObject.h, but merely Text Autosizing's private traversal function. But I agree that we should try to stay in sync with the core traversal functions, and avoid unnecessary duplication. (In reply to comment #4) > I would need more context to understand how this patch helps in any ways. Agree, more explanation would have been helpful. The current patch to bug 106792 uses a custom tree traversal function because it wants to traverse the descendants of a container, skipping descendants of renderers that are themselves containers (so far this is identical to nextInPreOrderSkippingDescendantsOfContainers), but it also wants to skip descendants of <a> elements. Currently nextInPreOrderSkippingDescendantsOfContainers doesn't provide any easy way of skipping arbitrary subtrees, and it seemed a feature that might be useful in other places too, so I suggested that Tim make nextInPreOrderSkippingDescendantsOfContainers slightly more flexible. In hindsight I realise that this was unnecessary, as I'd forgotten that RenderObject.h provides nextInPreOrderAfterChildren(stayWithin) which has exactly the functionality needed for this case. So this patch can probably be dropped. Instead, to reduce duplication, at some point we might want to reimplement nextInPreOrderSkippingDescendantsOfContainers as simply: RenderObject* TextAutosizer::nextInPreOrderSkippingDescendantsOfContainers(const RenderObject* current, const RenderObject* stayWithin) { if (current == stayWithin || !isAutosizingContainer(current)) return current->nextInPreOrder(stayWithin); return current->nextInPreOrderAfterChildren(stayWithin); } >> Source/WebCore/rendering/TextAutosizer.cpp:-366 >> - if (current == stayWithin || !isAutosizingContainer(current)) > > You removed the stayWithin check which means that you can potentially continue iterating if (current == stayWithin) For the record, no, this stayWithin check was because while we normally want to skip descendants of containers, we usually start a traversal by passing in a container as |current|, and in that specific case we didn't want to skip descendants of containers (because the whole point is to traverse the container). Instead Tim used NormalTraversal when starting the traversals above, so this special case was no longer necessary which is arguably cleaner (though might make it easier to misuse the traversal?). The stayWithin check in the for loop below is the one that actually enforces the stayWithin.

decided to take a different approach, closing this bug. the simplified implementation of nextInPreOrderSkippingDescendantsOfContainers (as outlined above) is now covered in https://bugs.webkit.org/show_bug.cgi?id=107446.

Comment on attachment 182813 [details] Patch Cleared review? from attachment 182813 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).