Bug 82572 - Add TextFieldDecorationElement::decorate()
Summary: Add TextFieldDecorationElement::decorate()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 82143
  Show dependency treegraph
 
Reported: 2012-03-28 21:43 PDT by Kent Tamura
Modified: 2012-03-28 23:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.65 KB, patch)
2012-03-28 21:48 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (4.52 KB, patch)
2012-03-28 22:17 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.