RESOLVED FIXED 63889
HTMLProgressElement implementation should be simplified
https://bugs.webkit.org/show_bug.cgi?id=63889
Summary HTMLProgressElement implementation should be simplified
Dominic Cooney
Reported 2011-07-03 22:46:40 PDT
Per the thread on bug 63861, progress element should be indeterminate when the value attribute is missing, per the spec here: <http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-progress-element> As is, HTMLProgressElement::isDeterminate tests whether the value is specified, and also < max.
Attachments
Patch (2.68 KB, patch)
2011-07-04 06:29 PDT, Dominic Cooney
no flags
Removes InvalidPosition (4.46 KB, patch)
2011-07-04 20:32 PDT, Dominic Cooney
no flags
Dominic Cooney
Comment 1 2011-07-04 06:24:42 PDT
It duplicates the logic for isDeterminate, uses slower attribute accessors than is necessary, and checks an impossible case (InvalidPosition) only used in its RenderProgress. I originally thought that this was buggy, however I don’t believe it is—just slightly obfuscated. Should HTMLProgressElement::InvalidPosition be deported to RenderProgress? That is the only place it is used, as a kind of flag to test whether the render object has been updated from the element for the first time or not.
Dominic Cooney
Comment 2 2011-07-04 06:29:50 PDT
Kent Tamura
Comment 3 2011-07-04 17:27:53 PDT
(In reply to comment #1) > It duplicates the logic for isDeterminate, uses slower attribute accessors than is necessary, and checks an impossible case (InvalidPosition) only used in its RenderProgress. ok, it would be reasonable. > Should HTMLProgressElement::InvalidPosition be deported to RenderProgress? That is the only place it is used, as a kind of flag to test whether the render object has been updated from the element for the first time or not. I think InvalidPosition is not needed. we can initialize RenderProgress::m_position with element->position(). Morita-san, what do you think?
Hajime Morrita
Comment 4 2011-07-04 20:28:01 PDT
Comment on attachment 99624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99624&action=review Looks good in general. have one question: > Source/WebCore/html/HTMLProgressElement.cpp:-140 > - return (HTMLProgressElement::IndeterminatePosition != currentPosition && HTMLProgressElement::InvalidPosition != currentPosition); Do we need InvalidPosition yet ?
Dominic Cooney
Comment 5 2011-07-04 20:32:42 PDT
Created attachment 99668 [details] Removes InvalidPosition
Dominic Cooney
Comment 6 2011-07-04 20:34:26 PDT
(In reply to comment #4) > Do we need InvalidPosition yet ? InvalidPosition was only used by RenderProgress; it initializes its position to InvalidPosition and then gets updated from the element. I have uploaded a patch that removes InvalidPosition entirely by initializing the position in the RenderProgress straight away… WDYT?
Dominic Cooney
Comment 7 2011-07-04 20:38:30 PDT
Comment on attachment 99668 [details] Removes InvalidPosition Initial InvalidPosition is still needed for starting indeterminate state animation.
WebKit Review Bot
Comment 8 2011-07-04 21:21:38 PDT
Comment on attachment 99624 [details] Patch Clearing flags on attachment: 99624 Committed r90384: <http://trac.webkit.org/changeset/90384>
WebKit Review Bot
Comment 9 2011-07-04 21:21:42 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.