Bug 53583

Created attachment 80908 [details] Patch

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

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.

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()?

Created attachment 81027 [details] Patch

Oops. Updated patch is not ready....

Created attachment 81031 [details] Patch

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.

Comment on attachment 81031 [details] Patch ok.

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

(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.

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