Bug 83664 – [Refactoring] Move initial style setting for ProgressValueElement from attach method to createShadowSubtree method in HTMLProgressElement.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Takashi Sakamoto]

HTMLProgressElement initializes ProgressValueElement's width property in attach method (via didElementStateChange). However it is better to initialize the property just after creating shadow subtree, which contains ProgressValueElement and ProgressBarElement.

[Takashi Sakamoto]

Created attachment 136812 [details]
Patch

[Hajime Morrita]

Comment on attachment 136812 [details]
Patch

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

> Source/WebCore/html/HTMLProgressElement.cpp:91
>  }

I'm sorry for the slow response.
Could you call updateFromElement() at HTMLProgressElement::createRenderer() or some existing place?
Then we can eliminate attach() at all.

[Hajime Morrita]

Also, please CC possible reviews in your mind.
Then they can (hopefully) notice that the patch is waiting for a review.

[Takashi Sakamoto]

(In reply to comment #2)
> (From update of attachment 136812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136812&action=review
> 
> > Source/WebCore/html/HTMLProgressElement.cpp:91
> >  }
> 
> I'm sorry for the slow response.
> Could you call updateFromElement() at HTMLProgressElement::createRenderer() or some existing place?

I tried moving updateFromElement() to HTMLProgressElement::createRenderer(), i.e.
--- --- ---
{
  RenderObject* renderer = new (arena) RenderProgress(this);
  renderer->updateFromElement();
  return renderer;
}
--- --- ---

However, this change caused a problem:
----
[21190:21190:5720078002:ERROR:process_util_posix.cc(143)] Received signal 11
        base::debug::StackTrace::StackTrace() [0x864a2a]
        base::(anonymous namespace)::StackDumpSignalHandler() [0x82568d]
        0x7f0714b15af0
        WTF::RefPtr<>::get() [0x4b098c]
        WebCore::DataRef<>::get() [0x4b097e]
        WebCore::DataRef<>::operator->() [0x4aedba]
        WebCore::RenderStyle::appearance() [0x6cc6ae]
        WebCore::RenderStyle::hasAppearance() [0x6cc68a]
        WebCore::RenderProgress::updateAnimationState() [0x19cdc4a]
        WebCore::RenderProgress::updateFromElement() [0x19cda6e]
        WebCore::HTMLProgressElement::createRenderer() [0x26682fb]
        WebCore::NodeRendererFactory::createRenderer() [0x775b0d]
        WebCore::NodeRendererFactory::createRendererIfNeeded() [0x775e6e]
        WebCore::Node::createRendererIfNeeded() [0x757687]
        WebCore::Element::attach() [0x73052b]
        WebCore::HTMLProgressElement::attach() [0x2668434]
        WebCore::ContainerNode::attachChildren() [0x6cbca5]
        WebCore::ContainerNode::attach() [0x6c9b0e]
        WebCore::Element::attach() [0x7305a4]
        WebCore::updateTreeAfterInsertion() [0x6cb623]
        WebCore::ContainerNode::appendChild() [0x6c95a5]
----
As m_style is initialized to be NULL (in HTMLProgressElement::createRenderer), style()->hasAppearance() causes signal 11.
I'm not sure whether there exists some method which is called after finishing creating renderer or not.
I would like to ask your idea.

[Takashi Sakamoto]

Created attachment 171179 [details]
Patch

[Takashi Sakamoto]

*** Bug 100507 has been marked as a duplicate of this bug. ***

[Andrey Khalyavin]

Sorry, that I submit an example with error - <progress> html element doesn't have min property. So we should remove it from test.

[Takashi Sakamoto]

Created attachment 171195 [details]
Patch

[Takashi Sakamoto]

(In reply to comment #7)
> Sorry, that I submit an example with error - <progress> html element doesn't have min property. So we should remove it from test.

I see. I have just removed.

Best regards,
Takashi Sakamoto

[Hajime Morrita]

Comment on attachment 171195 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        This patch fixes bug 100507. The new layout test is used to see whether

You don't need to mention this since the bug is marked as a dup of this bug.

[Takashi Sakamoto]

Created attachment 171198 [details]
Patch

[Takashi Sakamoto]

Comment on attachment 171195 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:16
>> +        This patch fixes bug 100507. The new layout test is used to see whether
> 
> You don't need to mention this since the bug is marked as a dup of this bug.

I see. Done.

[WebKit Review Bot]

Comment on attachment 171198 [details]
Patch

Clearing flags on attachment: 171198

Committed r133124: <http://trac.webkit.org/changeset/133124>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Can we add assertions to detect such mistakes?