Bug 63889 – HTMLProgressElement implementation should be simplified

• Component: DOM
• Status: RESOLVED FIXED

[Dominic Cooney]

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.

[Dominic Cooney]

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]

Created attachment 99624 [details]
Patch

[Kent Tamura]

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

Created attachment 99668 [details]
Removes InvalidPosition

[Dominic Cooney]

(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 on attachment 99668 [details]
Removes InvalidPosition

Initial InvalidPosition is still needed for starting indeterminate state animation.

[WebKit Review Bot]

Comment on attachment 99624 [details]
Patch

Clearing flags on attachment: 99624

Committed r90384: <http://trac.webkit.org/changeset/90384>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.