Bug 62430 - <progress> should support :indeterminate pseudo-class
Summary: <progress> should support :indeterminate pseudo-class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: HTML5Forms
  Show dependency treegraph
 
Reported: 2011-06-09 23:10 PDT by Hajime Morrita
Modified: 2011-06-14 06:22 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2011-06-09 23:10:14 PDT
As Mozilla does. https://developer.mozilla.org/en/HTML/element/progress
Comment 1 Yael 2011-06-12 18:19:20 PDT
Created attachment 96899 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Yael 2011-06-12 18:22:57 PDT
Created attachment 96901 [details]
Patch.

Fix style :)
Comment 4 WebKit Review Bot 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
Comment 5 Yael 2011-06-12 18:38:05 PDT
Created attachment 96903 [details]
Patch.

Fix Chromium build.
Comment 6 WebKit Review Bot 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
Comment 7 WebKit Review Bot 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
Comment 8 Kent Tamura 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
Comment 9 Kent Tamura 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.
Comment 10 Dominic Cooney 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?
Comment 11 Yael 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.
Comment 12 Yael 2011-06-13 06:34:42 PDT
Created attachment 96951 [details]
Patch.

Address comments #8, #9 and #10.
Comment 13 Yael 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.
Comment 14 Eric Seidel (no email) 2011-06-13 15:04:06 PDT
I'm not really sure who has worked in this code.
Comment 15 Kent Tamura 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.
Comment 16 Yael 2011-06-14 05:26:15 PDT
Created attachment 97099 [details]
Patch.

Additional changes from comment #15.
Comment 17 WebKit Review Bot 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
Comment 18 Yael 2011-06-14 05:41:07 PDT
Created attachment 97103 [details]
Patch.

Update test_expectations.txt
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2011-06-14 06:22:40 PDT
All reviewed patches have been landed.  Closing bug.