Bug 82572 – Add TextFieldDecorationElement::decorate()

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Add TextFieldDecorationElement::decorate()

[Kent Tamura]

Created attachment 134495 [details]
Patch

[Hajime Morrita]

Comment on attachment 134495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134495&action=review

> Source/WebCore/dom/ShadowRoot.cpp:-116
> -    ASSERT(purpose != CreatingUserAgentShadowRoot || !element->hasShadowRoot());

Looks like this assertion should be corrected instead of removed.
ASSERT(purpose == CreatingUserAgentShadowRoot || !element->hasShadowRoot()) will be an expected form.

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:80
> +    ASSERT(existingRoot->firstChild() == existingRoot->lastChild());

You mean existingRoot->childNodeCount() == 1 ?

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:82
> +    static_cast<HTMLElement*>(existingRoot->firstChild())->setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 1.0, CSSPrimitiveValue::CSS_NUMBER);

toHTMLElement() has captured this assert-and-cast pattern.

[Kent Tamura]

Comment on attachment 134495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134495&action=review

>> Source/WebCore/dom/ShadowRoot.cpp:-116
>> -    ASSERT(purpose != CreatingUserAgentShadowRoot || !element->hasShadowRoot());
> 
> Looks like this assertion should be corrected instead of removed.
> ASSERT(purpose == CreatingUserAgentShadowRoot || !element->hasShadowRoot()) will be an expected form.

Really?
Your expected form means that one can't add multiple author shadow roots to an element.

>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:80
>> +    ASSERT(existingRoot->firstChild() == existingRoot->lastChild());
> 
> You mean existingRoot->childNodeCount() == 1 ?

Yes.  I thought I should have avoided childNodeCount because it's O(N), but it's acceptable because the child node count should be 1.  I'll change it to childNodeCount()==1.

>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:82
>> +    static_cast<HTMLElement*>(existingRoot->firstChild())->setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 1.0, CSSPrimitiveValue::CSS_NUMBER);
> 
> toHTMLElement() has captured this assert-and-cast pattern.

will do.

[Kent Tamura]

Created attachment 134499 [details]
Patch 2

[Hajime Morrita]

(In reply to comment #3)
> (From update of attachment 134495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134495&action=review
> 
> >> Source/WebCore/dom/ShadowRoot.cpp:-116
> >> -    ASSERT(purpose != CreatingUserAgentShadowRoot || !element->hasShadowRoot());
> > 
> > Looks like this assertion should be corrected instead of removed.
> > ASSERT(purpose == CreatingUserAgentShadowRoot || !element->hasShadowRoot()) will be an expected form.
> 
> Really?
>> Your expected form means that one can't add multiple author shadow roots to an element.
You're right. I was confused.

> 
> >> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:80
> >> +    ASSERT(existingRoot->firstChild() == existingRoot->lastChild());
> > 
> > You mean existingRoot->childNodeCount() == 1 ?
> 
> Yes.  I thought I should have avoided childNodeCount because it's O(N), but it's acceptable because the child node count should be 1.  I'll change it to childNodeCount()==1.
> 
> >> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:82
> >> +    static_cast<HTMLElement*>(existingRoot->firstChild())->setInlineStyleProperty(CSSPropertyWebkitBoxFlex, 1.0, CSSPrimitiveValue::CSS_NUMBER);
> > 
> > toHTMLElement() has captured this assert-and-cast pattern.
> 
> will do.

[WebKit Review Bot]

Comment on attachment 134499 [details]
Patch 2

Clearing flags on attachment: 134499

Committed r112503: <http://trac.webkit.org/changeset/112503>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.