[Webkit-unassigned] [Bug 24760] Clean up Position.h

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28856|review?                     |review-
               Flag|                            |




------- Comment #8 from darin at apple.com  2009-03-23 11:41 PDT -------
(From update of attachment 28856)
> -    RenderBlock* container = renderer->containingBlock();
> +    RenderBlock* m_container = renderer->containingBlock();
>      RenderObject* next = renderer;
> -    while ((next = next->nextInPreOrder(container))) {
> +    while ((next = next->nextInPreOrder(m_container))) {

Oops! Please don't make this change.

> +    
> +    // This constructor should be private
> +    Position(PassRefPtr<Node> c, int o)
> +        : m_container(c)
> +        , m_offset(o)
> +    {}

We put these on separate subsequent lines. I suggest naming the arguments
container and offset.

> -    Node* node() const { return container.get(); }
> +    Node* node() const { return m_container.get(); }

I kinda hate this function name. Because, what node? I guess that's the core of
what you're going to be fixing.

> +    RefPtr<Node> m_container;
> +public:
> +    int m_offset; // FIXME: This should be made private.

I suggest you add the offset() function in this patch even if you don't adopt
it yet. Or don’t put that FIXME in.

> -    m_position.container = container;
> -    m_position.m_offset = offset;
> +    m_position = Position(container, offset);

This is more expensive than the old implementation. It churns the reference
count of container one extra time. You should consider having a function that
mutates the Position instead of assignment since it can be more efficient.

> -    m_position.container = child->parentNode();
>      m_childBefore = child->previousSibling();
> -    m_position.m_offset = m_childBefore ? invalidOffset : 0;
> +    m_position = Position(child->parentNode(), m_childBefore ? invalidOffset : 0);

Ditto.

>  inline void RangeBoundaryPoint::setToStart(PassRefPtr<Node> container)
>  {
>      ASSERT(container);
> -    m_position.container = container;
> -    m_position.m_offset = 0;
> +    m_position = firstDeepEditingPositionForNode(container);
>      m_childBefore = 0;
>  }

It looks to me like you’re sneaking in a behavior change here by calling
firstDeepEditingPositionForNode. How about putting that in a separate patch and
documenting what bug it fixes?

>  inline void RangeBoundaryPoint::setToEnd(PassRefPtr<Node> container)
>  {
>      ASSERT(container);
> -    m_position.container = container;
> -    if (m_position.container->offsetInCharacters()) {
> -        m_position.m_offset = m_position.container->maxCharacterOffset();
> +    if (container->offsetInCharacters()) {
> +        m_position = Position(container, container->maxCharacterOffset());
>          m_childBefore = 0;
>      } else {
> -        m_childBefore = m_position.container->lastChild();
> -        m_position.m_offset = m_childBefore ? invalidOffset : 0;
> +        m_childBefore = container->lastChild();
> +        m_position = Position(container, m_childBefore ? invalidOffset : 0);
>      }
>  }

Due to the use of invalidOffset in RangeBoundaryPoint, Position can’t really
start treating things in an abstract way until it takes over the “childBefore”
optimization. I’m also concerned that in the case of RangeBoundaryPoint, we
have special requirements. It’s not OK to automatically translate a Position
between multiple forms. Only time will tell, but this seems an easy area to get
wrong.

review- for now because of some of the mistakes above.


-- 
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.