[webkit-reviews] review granted: [Bug 47424] unlink removes inline style of anchor elements : [Attachment 70826] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47424: unlink removes inline style of anchor elements
https://bugs.webkit.org/show_bug.cgi?id=47424

Attachment 70826: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=70826&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=70826&action=review

> WebCore/editing/ApplyStyleCommand.cpp:1229
>      if (m_styledInlineElement &&
element->hasTagName(m_styledInlineElement->tagQName())) {
> -	   if (mode != RemoveNone) {
> -	       if (extractedStyle && element->inlineStyleDecl())
> -		   extractedStyle->merge(element->inlineStyleDecl());
> -	       removeNodePreservingChildren(element);
> -	   }
> +	   if (mode == RemoveNone)
> +	       return true;
> +	   ASSERT(extractedStyle);
> +	   if (element->inlineStyleDecl())
> +	       extractedStyle->merge(element->inlineStyleDecl());
> +	   removeNodePreservingChildren(element);
>	   return true;
>      }

This seems to be entirely refactoring and not part of the bug fix. Might be
nice to land it separately.

> WebCore/editing/ApplyStyleCommand.cpp:1592
> +	       Node* childNode = 0;

It seems dangerous that this is a raw pointer. Can’t
removeInlineStyleFromElement result in some kind of mutation?

> WebCore/editing/ApplyStyleCommand.cpp:1598
> +	       removeInlineStyleFromElement(style.get(), elem,	RemoveIfNeeded,
styleToPushDown.get());

Two spaces here after one of the commas.

> WebCore/editing/ApplyStyleCommand.cpp:1616
> +		   for (;childNode; childNode = childNode->nextSibling())

Please put a space after that first semicolon.

> WebCore/editing/ApplyStyleCommand.cpp:1880
> +    Position positionToComputeStyle;

Not quite a grammatical name. Maybe positionForStyleComputation instead?