Bug 82572

Summary: Add TextFieldDecorationElement::decorate()
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, morrita, webkit.review.bot
Priority: P2    
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.