[webkit-reviews] review denied: [Bug 27156] SplitElementCommand shouldn't be duplicating id attribute : [Attachment 62650] Fixed the switch statement

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 27156: SplitElementCommand shouldn't be duplicating id attribute
https://bugs.webkit.org/show_bug.cgi?id=27156

Attachment 62650: Fixed the switch statement
https://bugs.webkit.org/attachment.cgi?id=62650&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
It seems like you could separate the ApplyStyleCommand changes into a separate
patch from the SplitElementCommand changes.

I don't follow the logic of how we decide if we should split. Is there code
somewhere else that corresponds to this? I worry that this will fall out of
sync with the code that depends on nodes being split or not.

> +bool ApplyStyleCommand::shouldSplitElement(Element* elem,
CSSMutableStyleDeclaration* style)
> +{
> +    if (!elem || !elem->isHTMLElement() || !elem->parentElement() ||
!elem->parentElement()->isContentEditable())
> +	   return false;
> +
> +    // If the element is of styleInlineElement, then split because commands
such as createLink and unlink require this behavior
> +    if (m_styledInlineElement &&
elem->hasTagName(m_styledInlineElement->tagQName()))
> +	   return true;
> +
> +    // If the element is one of presentational elements and interferes with
the syle we're applying, then split
> +    if
(implicitlyStyledElementShouldBeRemovedWhenApplyingStyle(static_cast<HTMLElemen
t*>(elem), style))
> +	   return true;
> +
> +    // If the element is a font tag and interferes with the style we're
applying, then split
> +    if (elem->hasTagName(fontTag)) {
> +	   CSSMutableStyleDeclaration::const_iterator end = style->end();
> +	   for (CSSMutableStyleDeclaration::const_iterator it = style->begin();
it != end; ++it) {
> +	       switch ((*it).id()) {
> +	       case CSSPropertyColor:
> +		   if (elem->hasAttribute(colorAttr))
> +		       return true;
> +		   break;
> +	       case CSSPropertyFontFamily:
> +		   if (elem->hasAttribute(faceAttr))
> +		       return true;
> +		   break;
> +	       case CSSPropertyFontSize:
> +		   if (elem->hasAttribute(sizeAttr))
> +		       return true;
> +		   break;
> +	       }
> +	   }
> +    }
> +
> +    // Otherwise, compute the difference between the computed style and the
syle we're applying.
> +    // If the difference doesn't match new style, then the element
interferes, so split.
> +    RefPtr<CSSComputedStyleDeclaration> styleOfElem = computedStyle(elem);
> +    RefPtr<CSSComputedStyleDeclaration> styleOfParent =
computedStyle(elem->parentNode());
> +    RefPtr<CSSMutableStyleDeclaration> diff =
propertiesNotIn(styleOfElem.get(), styleOfParent.get());
> +    RefPtr<CSSMutableStyleDeclaration> styleToApply = style->copy();
> +
> +    bool isDiff = false;
> +    CSSMutableStyleDeclaration::const_iterator end = style->end();
> +    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it
!= end; ++it) {
> +	   const CSSProperty& property = *it;
> +	   if (diff->getPropertyCSSValue(property.id()) &&
diff->getPropertyValue(property.id()) != property.cssText())
> +	       isDiff = true;
> +    }
> +    return isDiff;
> +}


> +++ LayoutTests/ChangeLog	(working copy)
> +	   Added a test to ensure splitting element doesn't produce multiple
nodes with the same id.
> +	   The test unbolds a text inside b element with id so as to split the
element.
> +	   It traverses through all nodes in under document.body to ensure that
there is only one node with the id.

Don't need these last two sentences. It's clear from looking at the test that
it does this. The ChangeLog should just give a high-level understanding of the
changes and explain non-obvious bits.

> +++ LayoutTests/editing/style/split-element.html	(revision 0)
It might be easier to just:
document.execCommand('bold');
var test = document.getElementById('test');
test.parentNode.removeChild(test);
test = document.getElementById('test');
document.body.innerHTML = test ? 'FAIL ...' : 'PASS';

No need to keep a count of the nodes or anything.

> +<script type="text/javascript">

No need to include the type here.

> +function countNodesWithId(node, id)
> +{
> +    if (!(node instanceof Element))
> +	   return 0;
> +

No need to make sure they are elements. There won't be non-element nodes on
this page with an ID.

> +    var count = 0;
> +    if (node.id == id)
> +	   count++;
> +
> +    for (var i = 0; i < node.childNodes.length; i++)
> +	   count += countNodesWithId(node.childNodes[i], id);
> +
> +    return count;
> +}
> +
> +var range = document.createRange();
> +range.setStart(document.getElementById('test').firstChild, 1);
> +range.setEnd(document.getElementById('test').firstChild, 4);
> +window.getSelection().removeAllRanges();
> +window.getSelection().addRange(range);

var testNode = document.getElementById('test').firstChild;
window.getSelection().setBaseAndExtent(testNode, 1, testNode, 4);