[webkit-reviews] review denied: [Bug 27859] [chromium] Implement media slider for chromium : [Attachment 33897] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 3 13:56:03 PDT 2009


David Levin <levin at chromium.org> has denied Hin-Chung Lam <hclam at google.com>'s
request for review:
Bug 27859: [chromium] Implement media slider for chromium
https://bugs.webkit.org/show_bug.cgi?id=27859

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

------- Additional Comments from David Levin <levin at chromium.org>
Here's quick review.  Some potential bugs, but mostly style issues.


> Index: WebCore/rendering/RenderThemeChromiumSkia.cpp

> +bool RenderThemeChromiumSkia::paintMediaControlsBackground(RenderObject* o,
const RenderObject::PaintInfo& paintInfo, const IntRect& rect)

o -> object (Use whole words, same comment throughout)

> +{
> +#if ENABLE(VIDEO)
>
> +    // Draws the left border, it is always 1px wide.
> +    paint.setColor(o->style()->borderLeftColor().rgb());
> +    canvas->drawLine(rect.x() + 1,
> +			rect.y(),
> +			rect.x() + 1,
> +			rect.y() + rect.height(),
> +			paint);
> +
> +    // Draws the right border, it is always 1px wide.
> +    paint.setColor(o->style()->borderRightColor().rgb());
> +    canvas->drawLine(rect.x() + rect.width() - 1,
> +			rect.y(),
> +			rect.x() + rect.width() - 1,
> +			rect.y() + rect.height(),
> +			paint);

Does the rect have to be at least one pixel wide?  If not, it looks like this
could do weird things.

> +    return true;
> +#else

I think this may get unused variable warnings here.  So you should probably use
the UNUSED_PARAM macro (from wtf, same comment throughout.


> +    return false;
> +#endif
> +}
> +
> +bool RenderThemeChromiumSkia::paintMediaSliderTrack(RenderObject* o, const
RenderObject::PaintInfo& paintInfo, const IntRect& rect)
> +{

> +
> +    // Draw the buffered ranges.
> +    // FIXME: Draw multiple ranges if there are multiple buffered ranges.
> +    SkRect bufferedRect;
> +    bufferedRect.set(backgroundRect.fLeft + 2,
> +			backgroundRect.fTop + 2,
> +			backgroundRect.fRight - 2,
> +			backgroundRect.fBottom - 2);

Is backgroundRect at least four pixels wide/heigh?


> +    int width = static_cast<int>(bufferedRect.width() *
> +				    mediaElement->percentLoaded());
> +    bufferedRect.fRight = bufferedRect.fLeft + width;
> +
> +    SkPoint points[2] = { {0, bufferedRect.fTop}, {0, bufferedRect.fBottom}
};

I think there is usually a space inside the { (but this is inconsistent with
itself).


> +    SkColor startColor = o->style()->color().rgb();
> +    SkColor endColor = SkColorSetRGB(SkColorGetR(startColor) / 2,
> +					SkColorGetG(startColor) / 2,
> +					SkColorGetB(startColor) / 2);
> +    SkColor colors[2] = { startColor, endColor};

Space before }.

> +    SkShader* gradient = SkGradientShader::CreateLinear(
> +	   points, colors, NULL, 2, SkShader::kMirror_TileMode, NULL);

Use 0 instead of NULL.

Why is there a "2" in here? (Is it the arraysize of colors? If so, there are
macro for this.)

Lastly, this wrapping looks like something done to meet 80 columns (which isn't
necessary in WebKit code).  I would just unwrap this line.


> +
> +    paint.reset();
> +    paint.setShader(gradient);
> +    paint.setAntiAlias(true);
> +    canvas->drawRoundRect(bufferedRect, borderRadius.width(),
> +			     borderRadius.height(), paint);

There is no need to wrap at 80 columns in webkit code, so do whatever looks
best.

> +    gradient->unref();

Are there any smart points for these skia types? (like RefCounted?)




>  bool RenderThemeChromiumSkia::paintMediaPlayButton(RenderObject* o, const
RenderObject::PaintInfo& paintInfo, const IntRect& rect)
> Index: WebCore/rendering/RenderThemeChromiumSkia.h
> ===================================================================>	
> +	   virtual bool paintMediaControlsBackground(RenderObject*, const
RenderObject::PaintInfo&, const IntRect&);
> +	   virtual bool paintMediaSliderTrack(RenderObject*, const
RenderObject::PaintInfo&, const IntRect&);
> +	   virtual void adjustSliderThumbSize(RenderObject* o) const;

Don't include param names when they don't add information. "o"

> Index: WebCore/rendering/RenderThemeChromiumWin.cpp
> ===================================================================

>	   o->style()->setHeight(Length(sliderThumbAlongAxis, Fixed));
> +    } else {
> +	   RenderThemeChromiumSkia::adjustSliderThumbSize(o);
>      }

Don't use {} for single line statements.


More information about the webkit-reviews mailing list