[webkit-reviews] review denied: [Bug 53031] isContentEditable is not working properly with document.designMode : [Attachment 88632] fix patch: new patch after dependencies have been resolved.

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 53031: isContentEditable is not working properly with document.designMode
https://bugs.webkit.org/show_bug.cgi?id=53031

Attachment 88632: fix patch: new patch after dependencies have been resolved.
https://bugs.webkit.org/attachment.cgi?id=88632&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88632&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1229
> +	       if (style->userModify() == INHERIT)
> +		   return
primitiveValueCache->createValue(node->document()->inDesignMode() ? READ_WRITE
: READ_ONLY);

I don't think this is what you want.  As Darin suggested you should set the
value in CSSStyleSelector::styleForDocument and things should work just as
expected.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:1824
> +	   case INHERIT:
> +	       m_value.ident = CSSValueInherit;
> +	       break;

I don't think this is right.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:1831
> +    if (m_value.ident == CSSValueInherit)
> +	   return INHERIT;

Ditto.

> Source/WebCore/dom/Node.cpp:720
> -    if (document()->inDesignMode() || (document()->frame() &&
document()->frame()->page() && document()->frame()->page()->isEditable()))
> +    if (document()->frame() && document()->frame()->page() &&
document()->frame()->page()->isEditable())

Removing this call is good because if you modify
CSSStyleSelector::styleForDocument, then the value will automatically be
inherited.

> Source/WebCore/dom/Node.cpp:737
> +	       case INHERIT:
> +		   return document()->inDesignMode();

Then you don't need this work-around.