| 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: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 19264 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Hajime Morrita
2011-06-09 23:10:14 PDT
Created attachment 96899 [details]
Patch
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.
Created attachment 96901 [details]
Patch.
Fix style :)
Comment on attachment 96901 [details] Patch. Attachment 96901 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8835058 Created attachment 96903 [details]
Patch.
Fix Chromium build.
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 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
(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 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. 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? (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. Created attachment 96951 [details] Patch. Address comments #8, #9 and #10. (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. I'm not really sure who has worked in this code. 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. Created attachment 97099 [details] Patch. Additional changes from comment #15. 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 Created attachment 97103 [details]
Patch.
Update test_expectations.txt
Comment on attachment 97103 [details] Patch. Clearing flags on attachment: 97103 Committed r88793: <http://trac.webkit.org/changeset/88793> All reviewed patches have been landed. Closing bug. |