[webkit-reviews] review granted: [Bug 19644] Text copied with Select All pastes with a indent but shouldn't : [Attachment 29641] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Justin Garcia
<justin.garcia at apple.com>'s request for review:
Bug 19644: Text copied with Select All pastes with a indent but shouldn't
https://bugs.webkit.org/show_bug.cgi?id=19644

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

------- Additional Comments from Darin Adler <darin at apple.com>
Ideally I'd also like to see a check if the destination already has the
properties in question. For example, the same background color or same image.

> +	   if
(fullySelectedRootStyle->getPropertyCSSValue(CSSPropertyBackgroundImage) ||
> +	      
fullySelectedRootStyle->getPropertyCSSValue(CSSPropertyBackgroundColor) ||
> +	      
static_cast<Element*>(fullySelectedRoot)->hasAttribute(backgroundAttr))

I find this if statement really difficult to read. I would prefer that you put
it in a separate function or format it differently.

I don't understand why the cast to Element* here is safe. What guarantees that
fullySelectedRoot is an Element, not another kind of Node?

Given that we're mutating the DOM here, maybe fullySelectedRoot should be a
RefPtr<Node>, not just a Node*.

r=me