[webkit-reviews] review denied: [Bug 40217] [Mac] <meter> elements should be rendered as level indicators. : [Attachment 57997] patch v1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 40217: [Mac] <meter> elements should be rendered as level indicators.
https://bugs.webkit.org/show_bug.cgi?id=40217

Attachment 57997: patch v1
https://bugs.webkit.org/attachment.cgi?id=57997&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
Please add expectations for
 * fast/dom/HTMLMeterElement/meter-element.html
 * fast/dom/HTMLMeterElement/set-meter-properties.html
They are in platform/mac/Skipped for now.

If the height of a meter is larger than its width, WebKit with this patch draws
ugly NSLevelIndicator, right?


LayoutTests/ChangeLog:1
 +  2010-06-06	MORITA Hajime  <morrita at google.com>
This diff format will make webkit-patch work incorrectly.
We had better change the date.


LayoutTests/ChangeLog:5
 +	    https://bugs.webkit.org/show_bug.cgi?id=40217
 +	    [Mac] <meter> elements should be rendered as level indicators.
We usually write the 1-line description first, the bug URL then.


WebCore/rendering/RenderThemeMac.h:210
 +	NSLevelIndicatorStyle levelIndicatorStyleFor(ControlPart part) const;
The argument name "part" is not needed.


WebCore/rendering/RenderThemeMac.h:211
 +	NSLevelIndicatorCell* levelFor(const RenderMeter*) const;
nit: To call NSLevelIndicator(Cell) "level" is not so descriptive.  I prefer
"levelIndicatorFor".


WebCore/rendering/RenderThemeMac.h:220
 +	mutable RetainPtr<NSLevelIndicatorCell> m_level;
ditto.	I prefer m_levelIndicator.


WebCore/rendering/RenderThemeMac.mm:865
 +	// we explictly control the color intead giving low and high value to
NSLevelIndicatorCell as is.
typo: instead


WebCore/rendering/RenderThemeMac.mm:869
 +	    [cell setWarningValue:value+1];
Add spaces around "+".