Bug 40158 - ASSERTION FAILED with -webkit-appearance:progress-bar for non-progress bar elements
Summary: ASSERTION FAILED with -webkit-appearance:progress-bar for non-progress bar el...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-03 23:31 PDT by Hajime Morrita
Modified: 2010-09-08 20:48 PDT (History)
6 users (show)

See Also:


Attachments
reproduce (127 bytes, text/html)
2010-06-03 23:31 PDT, Hajime Morrita
no flags Details
patch v0 (16.05 KB, patch)
2010-06-04 01:34 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1; attempt to chromium build fix (16.05 KB, patch)
2010-06-04 02:01 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v2 (7.88 KB, patch)
2010-06-04 03:09 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v3; rebased (8.22 KB, patch)
2010-06-04 03:30 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v4 (8.22 KB, patch)
2010-06-06 19:07 PDT, Hajime Morrita
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 2010-06-03 23:31:38 PDT
Created attachment 57847 [details]
reproduce

ASSERTION FAILED occurs when "-webkit-appearance:progress-bar" is given for non-progress element like
<input style="-webkit-appearance: progress-bar;" />.
<meter> should have same problem, but it is not enabled yet.
Comment 1 Hajime Morrita 2010-06-04 01:34:34 PDT
Created attachment 57851 [details]
patch v0
Comment 2 Early Warning System Bot 2010-06-04 01:41:45 PDT
Attachment 57851 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3090020
Comment 3 WebKit Review Bot 2010-06-04 01:46:42 PDT
Attachment 57851 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3138007
Comment 4 Hajime Morrita 2010-06-04 02:01:33 PDT
Created attachment 57855 [details]
patch v1; attempt to chromium build fix
Comment 5 Hajime Morrita 2010-06-04 02:02:59 PDT
Comment on attachment 57855 [details]
patch v1; attempt to chromium build fix

I missed Qt build fix. canceling.
Comment 6 Hajime Morrita 2010-06-04 02:04:25 PDT
Comment on attachment 57855 [details]
patch v1; attempt to chromium build fix

Oops. Qt build failure has same cause as Chromium's one. re-enable this patch...
Comment 7 WebKit Review Bot 2010-06-04 02:13:57 PDT
Attachment 57851 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3133018
Comment 8 Kent Tamura 2010-06-04 02:32:20 PDT
Comment on attachment 57855 [details]
patch v1; attempt to chromium build fix

LayoutTests/fast/css/script-tests/invalid-appearance-progress-bar-meter.js:3
 +  var body = document.getElementsByTagName("body")[0];
document.body is simpler.


WebCore/rendering/RenderTheme.cpp:47
 +  #import "RenderProgress.h"
Why #import instead of #include ?


WebCore/rendering/RenderTheme.h:245
 +      virtual bool paintMeter(RenderMeter*, const RenderObject::PaintInfo&, const IntRect&);
For consistency with other paint*() member functions, I prefer not changing the parameter types, and check isMeter() and isProgress() in their paint*() functions.
Comment 9 Hajime Morrita 2010-06-04 03:09:07 PDT
Created attachment 57859 [details]
patch v2
Comment 10 WebKit Review Bot 2010-06-04 03:11:56 PDT
Attachment 57859 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/RenderThemeChromiumSkia.cpp:805:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Hajime Morrita 2010-06-04 03:30:56 PDT
Created attachment 57862 [details]
patch v3; rebased
Comment 12 Hajime Morrita 2010-06-04 03:33:23 PDT
Hi Kent-san, Thank you for your quick review!

> (From update of attachment 57855 [details])
> LayoutTests/fast/css/script-tests/invalid-appearance-progress-bar-meter.js:3
>  +  var body = document.getElementsByTagName("body")[0];
> document.body is simpler.
Done.

> 
> 
> WebCore/rendering/RenderTheme.cpp:47
>  +  #import "RenderProgress.h"
> Why #import instead of #include ?
Just copied from .mm file and didn't noticed. I'm sorry for confusion.

> WebCore/rendering/RenderTheme.h:245
>  +      virtual bool paintMeter(RenderMeter*, const RenderObject::PaintInfo&, const IntRect&);
> For consistency with other paint*() member functions, I prefer not changing the parameter types, and check isMeter() and isProgress() in their paint*() functions.
Done.
Comment 13 WebKit Review Bot 2010-06-04 03:54:05 PDT
Attachment 57859 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3107026
Comment 14 Kent Tamura 2010-06-04 04:29:14 PDT
Comment on attachment 57862 [details]
patch v3; rebased

WebCore/platform/qt/RenderThemeQt.cpp:731
 +          return false;
paint*() function should return true if it doesn't paint something.
Comment 15 Hajime Morrita 2010-06-06 19:07:51 PDT
Created attachment 57988 [details]
patch v4
Comment 16 Hajime Morrita 2010-06-06 19:09:00 PDT
Thank you for reviewing!

> WebCore/platform/qt/RenderThemeQt.cpp:731
>  +          return false;
> paint*() function should return true if it doesn't paint something.
Fixed to return true. I also fixed other similar parts as well.
Comment 17 Kent Tamura 2010-06-06 19:22:58 PDT
Comment on attachment 57988 [details]
patch v4

OK
Comment 18 Hajime Morrita 2010-06-06 20:08:39 PDT
Committed r60767: <http://trac.webkit.org/changeset/60767>
Comment 19 WebKit Review Bot 2010-06-06 20:22:32 PDT
http://trac.webkit.org/changeset/60767 might have broken Chromium Win Release
Comment 20 Hajime Morrita 2010-09-08 20:48:49 PDT
Comment on attachment 57988 [details]
patch v4

clearing r+