Bug 82572

Summary:

Add TextFieldDecorationElement::decorate()

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, morrita, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

82143

Attachments:

Description	Flags
Patch	none
Patch 2	none

Description Kent Tamura 2012-03-28 21:43:27 PDT

Add TextFieldDecorationElement::decorate()

Comment 1 Kent Tamura 2012-03-28 21:48:00 PDT

Created attachment 134495 [details]
Patch

Comment 2 Hajime Morrita 2012-03-28 22:00:47 PDT

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.

Comment 3 Kent Tamura 2012-03-28 22:11:13 PDT

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.

Comment 4 Kent Tamura 2012-03-28 22:17:14 PDT

Created attachment 134499 [details]
Patch 2

Comment 5 Hajime Morrita 2012-03-28 22:42:13 PDT

(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.

Comment 6 WebKit Review Bot 2012-03-28 23:00:40 PDT

Comment on attachment 134499 [details]
Patch 2

Clearing flags on attachment: 134499

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

Comment 7 WebKit Review Bot 2012-03-28 23:00:44 PDT

All reviewed patches have been landed.  Closing bug.