Bug 50660

(In reply to comment #0) > <progress> is holding its shadows as a member variables of RenderProgress. > We could move them to HTMLMeterElement. No, to HTMLProgressElement.

Created attachment 80424 [details] Patch

Hi Dimitri, could you take a look? I know this is just the first step of the shadow refresh and I'll eliminate more renderer-to-dom dependency in following changes.

Comment on attachment 80424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80424&action=review Ok. You probably should've put this patch into a separate bug, since it doesn't technically fix the bug yet -- and use this bug as a meta-bug :) > Source/WebCore/html/HTMLProgressElement.cpp:40 > +class ProgressBarValueElement : public ShadowBlockElement { Does it need to be ShadowBlockElement anymore? But I guess that's what the next patch is about.

Committed r77107: <http://trac.webkit.org/changeset/77107>

HI Dimitri, Thank you for reviewing! (In reply to comment #4) > (From update of attachment 80424 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80424&action=review > > Ok. You probably should've put this patch into a separate bug, since it doesn't technically fix the bug yet -- and use this bug as a meta-bug :) Sure. I'll do so ... or introduce another bug as a tracking bug. > > > Source/WebCore/html/HTMLProgressElement.cpp:40 > > +class ProgressBarValueElement : public ShadowBlockElement { > > Does it need to be ShadowBlockElement anymore? But I guess that's what the next patch is about. I'll do this when attacking <meter>. But I have a few item on <progress> side yet.

I see some new test crashes after this commit: http://build.webkit.org/results/Leopard%20Intel%20Release%20(Tests)/r77108%20(27420)/results.html

Patch was backed out via bug 53412.

Created attachment 80629 [details] Patch

Simon, thank you for save the tree. I'm sorry for the breakage. Dimitri, could you take a look again? The difference from the last patch is ProgressBarValueElement::detach(). The last patch missed to unlink from host -> shadow reference, which caused the inconsistency. (shadow -> host reference is unlinked by ShadowElement::detach()). Thanks in advance.

Comment on attachment 80629 [details] Patch Please make sure to run tests before landing.

> Please make sure to run tests before landing. Yes, I will. I'm really sorry for that. The test was done only under dom/HTMLProgressElement/ at the last patch and I was wrong.

Committed r77211: <http://trac.webkit.org/changeset/77211>

Comment on attachment 80629 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80629&action=review > Source/WebCore/rendering/RenderProgress.cpp:105 > + if (!(valuePart()->renderer() && valuePart()->renderer()->style()->hasAppearance())) Why does this need appearance? What happens if I do? ::-webkit-progress-bar-value { -webkit-appearance: none; background-color: red; }

(In reply to comment #14) > (From update of attachment 80629 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80629&action=review > > > Source/WebCore/rendering/RenderProgress.cpp:105 > > + if (!(valuePart()->renderer() && valuePart()->renderer()->style()->hasAppearance())) > > Why does this need appearance? What happens if I do? > > ::-webkit-progress-bar-value { > -webkit-appearance: none; > background-color: red; > } Shadow rendered even if "none" is given if host's appearance is set to "none". But yes, it's confusing.... I'll revise the logic in coming patch(es).