Bug 27859 – [chromium] Implement media slider for chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Hin-Chung Lam]

Need to proper implement the media slider for chromium port.

[Hin-Chung Lam]

Created attachment 33844 [details]
sample image

The media slider bar should look like this.

[Hin-Chung Lam]

Created attachment 33846 [details]
patch

[Hin-Chung Lam]

As you can see in the sample image the progress bar is in solid color, would it look better if it has a gradient?

[Hin-Chung Lam]

Created attachment 33896 [details]
sample image

[Hin-Chung Lam]

Created attachment 33897 [details]
patch

[Hin-Chung Lam]

I have updated the code and sample image, please take a look.

[Hin-Chung Lam]

ok, the latest implementation is good for review now.

[David Levin]

Comment on attachment 33897 [details]
patch

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.

[Hin-Chung Lam]

Created attachment 34016 [details]
patch

[Hin-Chung Lam]

I'd leave the unref() as is because SkAutoUnref is not a template type, if I use this in this code I need to have additional casting. 
As for number of points, I just leave it as is.

[Hin-Chung Lam]

Created attachment 34023 [details]
patch

[David Levin]

Comment on attachment 34023 [details]
patch

r+   since this passed a review within the chromium video folks and passed a sanity check by me.

[Adam Barth]

Will land.

[Adam Barth]

Comment on attachment 34023 [details]
patch

Clearing review flag on attachment: 34023

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/css/mediaControlsChromium.css
	M	WebCore/rendering/RenderThemeChromiumSkia.cpp
	M	WebCore/rendering/RenderThemeChromiumSkia.h
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
Committed r46740
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderThemeChromiumSkia.cpp
	M	WebCore/rendering/RenderThemeChromiumSkia.h
	M	WebCore/rendering/RenderThemeChromiumWin.cpp
	M	WebCore/css/mediaControlsChromium.css
r46740 = 3650cb3c52838303e8d91c3eaebc49b4be0da261 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46740

[Adam Barth]

All reviewed patches have been landed.  Closing bug.