[webkit-reviews] review denied: [Bug 51071] HTML5 <details> and <summary>: rendering : [Attachment 84073] patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Luiz Agostini <luiz at webkit.org>'s
request for review:
Bug 51071: HTML5 <details> and <summary>: rendering
https://bugs.webkit.org/show_bug.cgi?id=51071

Attachment 84073: patch
https://bugs.webkit.org/attachment.cgi?id=84073&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84073&action=review

Comments below;

> Source/WebCore/html/HTMLDetailsElement.cpp:59
> +    for (Node* child = firstChild(); !m_mainSummary && child; child =
child->nextSibling()) {
> +	   if (child->hasTagName(summaryTag))
> +	       m_mainSummary = child;
> +    }

Kind of a funny way to write this.  I'd just do a break rather than testing
!m_mainSummary in the condition.

> Source/WebCore/html/HTMLDetailsElement.cpp:78
> +    if (!changedByParser) {
> +	   Node* oldSummary = m_mainSummary;
> +	   findMainSummary();
> +
> +	   if (oldSummary != m_mainSummary && !m_isOpen && attached()) {
> +	       if (oldSummary && oldSummary->attached())
> +		   oldSummary->detach();
> +	       if (m_mainSummary && childCountDelta < 0 &&
!m_mainSummary->renderer()) {
> +		   m_mainSummary->detach();
> +		   m_mainSummary->attach();
> +	       }
> +	   }
> +    }
> +}

Can you explain this case?  I'm having trouble understanding what's going on
here.	In particular childCountDelta < 0?  I get why you have to detach the
old summary since it's now inside the unrevealed content, but I don't
understand the mainSummary case.

> Source/WebCore/html/HTMLDetailsElement.cpp:87
> +    if (attached() && m_mainSummary && !m_mainSummary->renderer()) {
> +	   m_mainSummary->detach();
> +	   m_mainSummary->attach();
> +    }

It feels like this has some overlap with the code in childrenChanged... maybe
you should incorporate the attach/detach updating into findMainSummary?

> Source/WebCore/html/HTMLDetailsElement.cpp:122
> +    FloatPoint absolutePosition =
renderDetails->absoluteToLocal(FloatPoint(mouseEvent->pageX() * factor,
mouseEvent->pageY() * factor));

You should rename this.  The point is a local position not an absolute
position..