Bug 27859 - [chromium] Implement media slider for chromium
Summary: [chromium] Implement media slider for chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-30 15:25 PDT by Hin-Chung Lam
Modified: 2009-08-03 17:44 PDT (History)
4 users (show)

See Also:


Attachments
sample image (84.26 KB, image/png)
2009-07-30 15:25 PDT, Hin-Chung Lam
no flags Details
patch (11.61 KB, patch)
2009-07-30 15:43 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
sample image (83.49 KB, image/png)
2009-07-31 13:19 PDT, Hin-Chung Lam
no flags Details
patch (12.24 KB, patch)
2009-07-31 13:20 PDT, Hin-Chung Lam
levin: review-
Details | Formatted Diff | Diff
patch (13.92 KB, patch)
2009-08-03 16:30 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
patch (14.07 KB, patch)
2009-08-03 16:59 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2009-07-30 15:25:01 PDT
Need to proper implement the media slider for chromium port.
Comment 1 Hin-Chung Lam 2009-07-30 15:25:30 PDT
Created attachment 33844 [details]
sample image

The media slider bar should look like this.
Comment 2 Hin-Chung Lam 2009-07-30 15:43:23 PDT
Created attachment 33846 [details]
patch
Comment 3 Hin-Chung Lam 2009-07-30 15:46:52 PDT
As you can see in the sample image the progress bar is in solid color, would it look better if it has a gradient?
Comment 4 Hin-Chung Lam 2009-07-31 13:19:19 PDT
Created attachment 33896 [details]
sample image
Comment 5 Hin-Chung Lam 2009-07-31 13:20:11 PDT
Created attachment 33897 [details]
patch
Comment 6 Hin-Chung Lam 2009-07-31 13:20:40 PDT
I have updated the code and sample image, please take a look.
Comment 7 Hin-Chung Lam 2009-07-31 13:52:32 PDT
ok, the latest implementation is good for review now.
Comment 8 David Levin 2009-08-03 13:56:03 PDT
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.
Comment 9 Hin-Chung Lam 2009-08-03 16:30:30 PDT
Created attachment 34016 [details]
patch
Comment 10 Hin-Chung Lam 2009-08-03 16:35:09 PDT
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.
Comment 11 Hin-Chung Lam 2009-08-03 16:59:30 PDT
Created attachment 34023 [details]
patch
Comment 12 David Levin 2009-08-03 17:04:10 PDT
Comment on attachment 34023 [details]
patch

r+   since this passed a review within the chromium video folks and passed a sanity check by me.
Comment 13 Adam Barth 2009-08-03 17:31:15 PDT
Will land.
Comment 14 Adam Barth 2009-08-03 17:44:00 PDT
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
Comment 15 Adam Barth 2009-08-03 17:44:04 PDT
All reviewed patches have been landed.  Closing bug.