[webkit-reviews] review granted: [Bug 63384] Stop instantiating Position with PositionIsOffsetInAnchor in various files : [Attachment 98844] Added containerText
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 28 09:49:38 PDT 2011
Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 63384: Stop instantiating Position with PositionIsOffsetInAnchor in various
files
https://bugs.webkit.org/show_bug.cgi?id=63384
Attachment 98844: Added containerText
https://bugs.webkit.org/attachment.cgi?id=98844&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=98844&action=review
> Source/WebCore/dom/Position.cpp:95
> + ASSERT(!((anchorType == PositionIsBeforeChildren || anchorType ==
PositionIsAfterChildren)
> + && (m_anchorNode->isTextNode() ||
editingIgnoresContent(m_anchorNode.get()))));
Normally we’d not line up the && with the parenthesis, and just indent it 4
characters. Or we’d leave this all in one big if statement. Or even make a
helper so this assertion is easier to read and shorter.
> Source/WebCore/dom/Position.cpp:152
> +Text* Position::containerText() const
I’m a little confused about the rules for when this function returns 0 and when
it’s illegal to call the function. It seems that for PositionIsBeforeChildren
and PositionIsAfterChildren it’s illegal to even call the function. Why is
that? And why not the same rule for the other anchor types.
I’m guessing some of this strangeness might be shared by the containerNode
function. I’d like to understand the rationale for when this returns 0 and when
it asserts.
> Source/WebCore/dom/Position.cpp:155
> + if (!m_anchorNode || !m_anchorNode->isTextNode())
> + return 0;
Strange to not check the anchor type when the anchor node happens to not be a
text node. It would be more natural to have the isTextNode check be inside the
switch statement just before the return statement.
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:193
> + splitTextNode(start.containerText(), startOffset);
For uses like this it might be good to have a containerText function that
asserts it’s non-zero rather than one that returns 0 if it’s not text or a null
position or whatever. I think it makes sense to have a function that is only
callable when you know the container is a text node.
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:194
> + start = firstPositionInNode(start.containerNode());
It seems a little strange to get the container again right after getting it on
the previous line. This was less strange when this was a free getter, but now
that it does some additional checking it’s a little irritating that it does it
twice.
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:197
> + end = Position(start.containerText(),
end.offsetInContainerNode() - startOffset);
Not twice, three times!
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:201
> + m_endOfLastParagraph = Position(start.containerText(),
m_endOfLastParagraph.offsetInContainerNode() - startOffset);
Four times!
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:250
> + if (containerNode.get() == start.containerNode() &&
containerNode->previousSibling() &&
containerNode->previousSibling()->isTextNode()) {
Should not need the get() here.
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:254
> + if (containerNode.get() == end.containerNode() &&
containerNode->previousSibling() &&
containerNode->previousSibling()->isTextNode()) {
Should not need the get() here.
> Source/WebCore/editing/ApplyBlockElementCommand.cpp:258
> + if (containerNode.get() == m_endOfLastParagraph.containerNode()) {
Should not need the get() here.
> Source/WebCore/editing/ApplyStyleCommand.cpp:1130
> + updateStartEnd(firstPositionInNode(start.containerNode()), newEnd);
A general pattern here seems to be re-fetching the same node over and over
again. Can we maybe use RefPtr<Text> a bit to avoid that. Not sure what the
best style is.
More information about the webkit-reviews
mailing list