Bug 53583 – Refactoring: <progress> should not use ShadowElement

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

This is a part of ShadowElement removal refactoring.

[Hajime Morrita]

Created attachment 80908 [details]
Patch

[Hajime Morrita]

Hi Dimitri, could you take a look? 
This change kicks ShadowElement out from <progress>.
I'll do same for <meter> after this one.

[WebKit Review Bot]

Attachment 80908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 80908 [details]
Patch

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

Can you explain a bit about the originalVisibility? I may have known but forgotten, and now confuses me. I am easily confused. Why do we need this?

> Source/WebCore/html/HTMLProgressElement.cpp:40
> +class ProgressBarValueElement : public HTMLDivElement {

This class should live in html/shadow/ProgressBarValuePartElement

> Source/WebCore/html/HTMLProgressElement.cpp:53
> +#if 0

You didn't mean to leave this in :)

> Source/WebCore/rendering/RenderIndicator.h:37
> +    virtual bool shouldHidden() = 0;

shouldBeHidden()?

> Source/WebCore/rendering/RenderProgress.cpp:102
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling())

Why is this here? Why can't we just use RenderProgress:layout()?

[Hajime Morrita]

Created attachment 81027 [details]
Patch

[Hajime Morrita]

Oops. Updated patch is not ready....

[Hajime Morrita]

Created attachment 81031 [details]
Patch

[Hajime Morrita]

Dimitori, thank you for the review!
I updated the patch.

(In reply to comment #4)
> (From update of attachment 80908 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80908&action=review
> 
> Can you explain a bit about the originalVisibility? I may have known but forgotten, and now confuses me. I am easily confused. Why do we need this?
This is for the situation that dynamically changes the visibility. (hiding/unhiding the part element.)
Though <progress> has no such case,  <meter> has one.
I'm preparing for code sharing between <meter>and <progress>, so I'll put this here.

> 
> > Source/WebCore/html/HTMLProgressElement.cpp:40
> > +class ProgressBarValueElement : public HTMLDivElement {
> 
> This class should live in html/shadow/ProgressBarValuePartElement
Moved to there.

> 
> > Source/WebCore/html/HTMLProgressElement.cpp:53
> > +#if 0
> 
> You didn't mean to leave this in :)
>
Oops. Removed.
 
> > Source/WebCore/rendering/RenderIndicator.h:37
> > +    virtual bool shouldHidden() = 0;
> 
> shouldBeHidden()?
Sure. Renamed.

> 
> > Source/WebCore/rendering/RenderProgress.cpp:102
> > +    for (RenderObject* child = firstChild(); child; child = child->nextSibling())
> 
> Why is this here? Why can't we just use RenderProgress:layout()?
This is because RenderProgress shares layout() with RenderMeter via RenderIndicator superclass.
I'm going to migrate the layout code of both and remove layoutAsPart() at that change.

[Dimitri Glazkov (Google)]

Comment on attachment 81031 [details]
Patch

ok.

[Dominic Cooney]

It looks like patch 81031 mis-spells ProgressBarValueElement.h in GNUmakefile.am and WebCore.gypi (ProgressBarValuElement.h c.f. ProgressBarValueElement.h in WebCore.xcodeproj.)

[Hajime Morrita]

(In reply to comment #10)
> It looks like patch 81031 mis-spells ProgressBarValueElement.h in GNUmakefile.am and WebCore.gypi (ProgressBarValuElement.h c.f. ProgressBarValueElement.h in WebCore.xcodeproj.)
oh, thanks for finding this! I'll fix it before landing.

[Hajime Morrita]

Committed r77578: <http://trac.webkit.org/changeset/77578>