WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Removes InvalidPosition
(4.46 KB, patch)
2011-07-04 20:32 PDT
,
Dominic Cooney
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 99624
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug