Bug 63889 - HTMLProgressElement implementation should be simplified
Summary: HTMLProgressElement implementation should be simplified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Cooney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-03 22:46 PDT by Dominic Cooney
Modified: 2011-07-04 21:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.68 KB, patch)
2011-07-04 06:29 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Removes InvalidPosition (4.46 KB, patch)
2011-07-04 20:32 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 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.
Comment 1 Dominic Cooney 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.
Comment 2 Dominic Cooney 2011-07-04 06:29:50 PDT
Created attachment 99624 [details]
Patch
Comment 3 Kent Tamura 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?
Comment 4 Hajime Morrita 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 ?
Comment 5 Dominic Cooney 2011-07-04 20:32:42 PDT
Created attachment 99668 [details]
Removes InvalidPosition
Comment 6 Dominic Cooney 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?
Comment 7 Dominic Cooney 2011-07-04 20:38:30 PDT
Comment on attachment 99668 [details]
Removes InvalidPosition

Initial InvalidPosition is still needed for starting indeterminate state animation.
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2011-07-04 21:21:42 PDT
All reviewed patches have been landed.  Closing bug.