[Webkit-unassigned] [Bug 24763] Position should support neighbor-anchored positions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24763





------- Comment #8 from eric at webkit.org  2009-04-07 06:15 PDT -------
(In reply to comment #6)
> (From update of attachment 29274 [review])
> > +    switch (anchorType()) {
> > +    case PositionIsOffsetInAnchor:
> > +        return m_anchorNode.get();
> 
> I prefer indenting the cases inside the switch statements.

Me too. :)  But that's a violation of the style guide:
http://webkit.org/coding/coding-style.html

r=me to change the style guide to require indenting case: statements.

> > +    case PositionIsOffsetInAnchor:
> > +    {
> 
> Opening brace should be on same line as case.

This wasn't covered in the style guide, so I made a guess. :)

> > +// FIXME: Position should store an AnchorType up-front, instead of lazily
> > +// determining it via editingIgnoresContent(anchorNode())
> > +// That would require fixing moveToOffset to know how to recompute the AnchorType
> > +// for positions which need historical editing-compatible behavior
> > +// (and for explicitly anchored positions to ASSERT_NOT_REACHED())
> 
> Or maybe you could just compute the anchor type at construction time for now.

Yeah, done in a separate patch.  I tried to keep these small-as-possible.  This
rewrite has been a huge pain, mostly due to my initial attempts trying to
change too much at once.

> > +    // These are always DOM compliant values.  Editing positions like [img, 0] (aka [img, before])
> > +    // will return img->parentNode() and img->nodeIndex() from these functions.
> > +    Node* containerNode() const;
> > +    int computeOffsetInContainerNode() const;
> > +
> > +    // These are convenience methods which are smart about whether the position is neighbor anchored or parent anchored
> > +    Node* nodeBeforePosition() const;
> > +    Node* nodeAfterPosition() const;
> 
> I'm not sure I understand the rule for when you use "compute" and when you
> don't. It seems that nodeBeforePosition and nodeAfterPosition can both be
> expensive operations that involve calling childNode, so it seems they should
> have compute in their names unless and until you change the class's design.

Agreed!  Yes, these should definitely be named "compute" since they call
childNode() in the parent-anchor case.


I'm not sure I like computeOffsetInContainer() anymore.  I might change callers
to use a toParentAnchoredPosition() or makeParentAnchored() call instead.  Not
sure yet.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.