Bug 62430

Summary:

<progress> should support :indeterminate pseudo-class

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric.carlson, eric, macpherson, tkent, webkit.review.bot, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

19264

Attachments:

Description	Flags
Patch	none
Patch.	webkit.review.bot: commit-queue-
Patch.	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01	none
Patch.	tkent: review+
Patch.	webkit.review.bot: commit-queue-
Patch.	none

Hajime Morrita

Reported 2011-06-09 23:10:14 PDT

As Mozilla does. https://developer.mozilla.org/en/HTML/element/progress

Attachments
Patch (27.76 KB, patch) 2011-06-12 18:19 PDT, Yael	no flags	Details Formatted Diff Diff
Patch. (27.75 KB, patch) 2011-06-12 18:22 PDT, Yael	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Patch. (27.75 KB, patch) 2011-06-12 18:38 PDT, Yael	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.60 MB, application/zip) 2011-06-12 18:54 PDT, WebKit Review Bot	no flags	Details
Patch. (29.29 KB, patch) 2011-06-13 06:34 PDT, Yael	tkent: review+	Details Formatted Diff Diff
Patch. (29.24 KB, patch) 2011-06-14 05:26 PDT, Yael	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Patch. (29.35 KB, patch) 2011-06-14 05:41 PDT, Yael	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Yael

Comment 1 2011-06-12 18:19:20 PDT

Created attachment 96899 [details] Patch

WebKit Review Bot

Comment 2 2011-06-12 18:21:52 PDT

Attachment 96899 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/html/HTMLProgressElement.cpp:152: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yael

Comment 3 2011-06-12 18:22:57 PDT

Created attachment 96901 [details] Patch. Fix style :)

WebKit Review Bot

Comment 4 2011-06-12 18:27:38 PDT

Comment on attachment 96901 [details] Patch. Attachment 96901 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8835058

Yael

Comment 5 2011-06-12 18:38:05 PDT

Created attachment 96903 [details] Patch. Fix Chromium build.

WebKit Review Bot

Comment 6 2011-06-12 18:54:28 PDT

Comment on attachment 96903 [details] Patch. Attachment 96903 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8831617 New failing tests: fast/dom/HTMLProgressElement/indeterminate-progress-001.html

WebKit Review Bot

Comment 7 2011-06-12 18:54:34 PDT

Created attachment 96905 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Kent Tamura

Comment 8 2011-06-12 18:59:48 PDT

(In reply to comment #6) > (From update of attachment 96903 [details]) > Attachment 96903 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/8831617 > > New failing tests: > fast/dom/HTMLProgressElement/indeterminate-progress-001.html Would you add the following line to LayoutTests/chromium/test_expectations.txt please? BUGWK62430 : fast/dom/HTMLProgressElement/indeterminate-progress-001.html = FAIL

Kent Tamura

Comment 9 2011-06-12 19:06:14 PDT

Comment on attachment 96903 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=96903&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1078 > if (isControl) { nit: This block for isControl is large. Can we make a separated function? > Source/WebCore/html/HTMLProgressElement.cpp:140 > + return (HTMLProgressElement::IndeterminatePosition != position() && HTMLProgressElement::InvalidPosition != position()); Calling position() twice is not efficient. > LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:12 > + document.getElementById("dymanicindeterminate").removeAttribute("value"); > + document.getElementById("dymanicdeterminate").setAttribute("value", 0.5); > +} We usually use 4-space indentation in JavaScript code. > LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-002.html:10 > +window.onload = function() { > + if (window.layoutTestController) ditto.

Dominic Cooney

Comment 10 2011-06-12 19:08:54 PDT

Comment on attachment 96903 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=96903&action=review > Source/WebCore/css/CSSStyleSelector.cpp:1085 > + if ((thisProgressElement->isDeterminate() && !otherProgressElement->isDeterminate()) || (!thisProgressElement->isDeterminate() && otherProgressElement->isDeterminate())) Maybe thisProgressElement->isDeterminate() != otherProgressElement->isDeterminate() is simpler? Then since there is only one expression, you could inline the casts to shorten this a bit more. > LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:1 > +<!DOCTYPE html> Does the pixel result for this test really add anything that the textual result doesn't have?

Yael

Comment 11 2011-06-13 06:33:16 PDT

(In reply to comment #10) > (From update of attachment 96903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96903&action=review Thank you for reviewing :) > > > Source/WebCore/css/CSSStyleSelector.cpp:1085 > > + if ((thisProgressElement->isDeterminate() && !otherProgressElement->isDeterminate()) || (!thisProgressElement->isDeterminate() && otherProgressElement->isDeterminate())) > > Maybe > > thisProgressElement->isDeterminate() != otherProgressElement->isDeterminate() > > is simpler? > ok > Then since there is only one expression, you could inline the casts to shorten this a bit more. > > > LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-001.html:1 > > +<!DOCTYPE html> > > Does the pixel result for this test really add anything that the textual result doesn't have? I think so; Just like progress-bar-value-pseudo-element.html, the pixel result shows that we can see the changes to the style of the progress element.

Yael

Comment 12 2011-06-13 06:34:42 PDT

Created attachment 96951 [details] Patch. Address comments #8, #9 and #10.

Yael

Comment 13 2011-06-13 06:36:27 PDT

(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 96903 [details] [details]) > > Attachment 96903 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/8831617 > > > > New failing tests: > > fast/dom/HTMLProgressElement/indeterminate-progress-001.html > > Would you add the following line to LayoutTests/chromium/test_expectations.txt please? > > BUGWK62430 : fast/dom/HTMLProgressElement/indeterminate-progress-001.html = FAIL Thank you for your review. I added the line, but I think the failure is because of the missing expected result.

Eric Seidel (no email)

Comment 14 2011-06-13 15:04:06 PDT

I'm not really sure who has worked in this code.

Kent Tamura

Comment 15 2011-06-13 16:11:59 PDT

Comment on attachment 96951 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=96951&action=review > Source/WebCore/html/HTMLProgressElement.cpp:151 > + if ((wasIndeterminate && isDeterminate()) || (!wasIndeterminate && !isDeterminate())) We can write this as if (wasIndeterminate == isDeterminate()) Also, I think wasIndeterminate should be changed to wasDeterminate. bool wasDeterminate = render->isDeterminate(); render->updateFromEelemnt(); if (wasDetermiante != isDeterminate) setNeedsStyleRecalc(); "if (status is change) setNeedsStyleRecalc()" is a common idiom and easy to read for us.

Yael

Comment 16 2011-06-14 05:26:15 PDT

Created attachment 97099 [details] Patch. Additional changes from comment #15.

WebKit Review Bot

Comment 17 2011-06-14 05:28:07 PDT

Comment on attachment 97099 [details] Patch. Rejecting attachment 97099 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2 Last 500 characters of output: ected.txt patching file LayoutTests/fast/dom/HTMLProgressElement/indeterminate-progress-002.html patching file LayoutTests/platform/chromium/test_expectations.txt Hunk #1 FAILED at 3994. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file LayoutTests/platform/mac/fast/dom/HTMLProgressElement/indeterminate-progress-001-expected.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/8844049

Yael

Comment 18 2011-06-14 05:41:07 PDT

Created attachment 97103 [details] Patch. Update test_expectations.txt

WebKit Review Bot

Comment 19 2011-06-14 06:22:31 PDT

Comment on attachment 97103 [details] Patch. Clearing flags on attachment: 97103 Committed r88793: <http://trac.webkit.org/changeset/88793>

WebKit Review Bot

Comment 20 2011-06-14 06:22:40 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.