[webkit-reviews] review denied: [Bug 30836] Creating a link when selecting multiple nodes creates multiple links : [Attachment 65356] initial patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 30836: Creating a link when selecting multiple nodes creates multiple links
https://bugs.webkit.org/show_bug.cgi?id=30836

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

------- Additional Comments from Tony Chang <tony at chromium.org>
> Index: WebCore/editing/ApplyStyleCommand.cpp
> +    bool operator==(const StyleChange& other)
> +    {
> +	   return m_cssStyle == other.m_cssStyle

I am worried that this will regress someday when someone adds a new member, but
I don't have a good suggestion on how to avoid this.

> -	       while (sibling && sibling != pastEndNode &&
(!sibling->isElementNode() || sibling->hasTagName(brTag)) && !isBlock(sibling))
{
> +	       StyleChange startChange(style, Position(node, 0));
> +	       while (sibling && sibling != pastEndNode &&
!sibling->contains(pastEndNode)) {
> +		   if (isBlock(sibling) && !sibling->hasTagName(brTag))
> +		       break;
> +		   if (StyleChange(style, Position(sibling, 0)) != startChange)

> +		       break;

Was there a reason to move conditions into separate if statements?  Seems like
these could be merged with the while condition.

> +	   RefPtr<StyledElement> styledElement;
> +	   if (current->isStyledElement() && m_styledInlineElement &&
current->hasTagName(m_styledInlineElement->tagQName()))
> +	       styledElement = static_cast<StyledElement*>(current);

Does |styledElement| need to be a RefPtr?


> Index: WebCore/editing/ApplyStyleCommand.h
> -    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start,
Node* end);
> +    void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start,
Node* end, bool shouldAddStyledElement = true);

Can we use an enum instead of a bool?


Can you also add a test for anchors with inline styles.