[Webkit-unassigned] [Bug 27859] [chromium] Implement media slider for chromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27859


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33897|review?                     |review-
               Flag|                            |




--- Comment #8 from David Levin <levin at chromium.org>  2009-08-03 13:56:03 PDT ---
(From update of attachment 33897)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.