[webkit-reviews] review denied: [Bug 24760] Clean up Position.h : [Attachment 28856] Rename Position::container to m_container and make it private

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 24760: Clean up Position.h
https://bugs.webkit.org/show_bug.cgi?id=24760

Attachment 28856: Rename Position::container to m_container and make it private
https://bugs.webkit.org/attachment.cgi?id=28856&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    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.