ASSERTION FAILED with -webkit-appearance:progress-bar for non-progress bar elements

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 57851 [details]
patch v0

[Early Warning System Bot]

Attachment 57851 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3090020

[WebKit Review Bot]

Attachment 57851 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3138007

[Hajime Morrita]

Created attachment 57855 [details]
patch v1; attempt to chromium build fix

[Hajime Morrita]

Comment on attachment 57855 [details]
patch v1; attempt to chromium build fix

I missed Qt build fix. canceling.

[Hajime Morrita]

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...

[WebKit Review Bot]

Attachment 57851 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3133018

[Kent Tamura]

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.

[Hajime Morrita]

Created attachment 57859 [details]
patch v2

[WebKit Review Bot]

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.

[Hajime Morrita]

Created attachment 57862 [details]
patch v3; rebased

[Hajime Morrita]

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.

[WebKit Review Bot]

Attachment 57859 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3107026

[Kent Tamura]

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.

[Hajime Morrita]

Created attachment 57988 [details]
patch v4

[Hajime Morrita]

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.

[Kent Tamura]

Comment on attachment 57988 [details]
patch v4

OK

[Hajime Morrita]

Committed r60767: <http://trac.webkit.org/changeset/60767>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60767 might have broken Chromium Win Release

[Hajime Morrita]

Comment on attachment 57988 [details]
patch v4

clearing r+