Pass SkPaint instead of its alpha and mode in WebMediaPlayer::paint

media/renderers/skcanvas_video_renderer.cc

Index: media/renderers/skcanvas_video_renderer.cc
diff --git a/media/renderers/skcanvas_video_renderer.cc b/media/renderers/skcanvas_video_renderer.cc
index 8c71b3edfbdd00fd7da334e1bfbe1f838f8edcc1..ffbe7ce30dd88f134d4cb548a5948af0f23bed74 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -332,27 +332,26 @@ SkCanvasVideoRenderer::~SkCanvasVideoRenderer() {
 void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
                                   SkCanvas* canvas,
                                   const gfx::RectF& dest_rect,
-                                  uint8_t alpha,
-                                  SkXfermode::Mode mode,
+                                  const SkPaint* paint,

[Justin Novosad, 2016/08/25 19:18:18]

since the code is not mean to support nullptr for paint, you should use &
instead of *

                                   VideoRotation video_rotation,
                                   const Context3D& context_3d) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (alpha == 0) {
+  if (paint->getAlpha() == 0) {
     return;
   }
 
   SkRect dest;
   dest.set(dest_rect.x(), dest_rect.y(), dest_rect.right(), dest_rect.bottom());
 
-  SkPaint paint;
-  paint.setAlpha(alpha);
+  SkPaint freshPaint;

[Justin Novosad, 2016/08/25 19:18:19]

this is a funny name.  What do you mean by fresh?

[xidachen, 2016/08/26 02:21:39]

It is basically explaining that we are creating a new skpaint instead of using
the one that is passed in. Should I name it "newPaint"?

[Justin Novosad, 2016/08/26 14:25:25]

Think of the person reading this code trying to understand it.  There is a
reason you are not using "paint" directly.  You only want to use part of the
settings in "paint".  This code sets up a paint and changes it further later on,
which is also bad for readability.  I would encourage you to us two paint
objects.  Call the first one  blackWithAlphaPaint, and create it inside the "if"
statement that uses it. The second one could be videoPaint, or something like
that to imply that it was construct with specifically the properties needed for
the video blit.

+  freshPaint.setAlpha(paint->getAlpha());
 
   // Paint black rectangle if there isn't a frame available or the
   // frame has an unexpected format.
   if (!video_frame.get() || video_frame->natural_size().IsEmpty() ||
       !(media::IsYuvPlanar(video_frame->format()) ||
         video_frame->HasTextures())) {
-    canvas->drawRect(dest, paint);
+    canvas->drawRect(dest, freshPaint);
     canvas->flush();
     return;
   }
@@ -361,8 +360,11 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
   if (!UpdateLastImage(video_frame, context_3d))
     return;
 
-  paint.setXfermodeMode(mode);
-  paint.setFilterQuality(kLow_SkFilterQuality);
+  SkXfermode::Mode mode;
+  if (!paint || !SkXfermode::AsMode(paint->getXfermode(), &mode))
+    mode = SkXfermode::kSrcOver_Mode;
+  freshPaint.setXfermodeMode(mode);
+  freshPaint.setFilterQuality(paint->getFilterQuality());
 
   const bool need_rotation = video_rotation != VIDEO_ROTATION_0;
   const bool need_scaling =
@@ -412,9 +414,9 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
   // threads. (skbug.com/4321).
   if (canvas->imageInfo().colorType() == kUnknown_SkColorType) {
     sk_sp<SkImage> swImage = last_image_->makeNonTextureImage();
-    canvas->drawImage(swImage, 0, 0, &paint);
+    canvas->drawImage(swImage, 0, 0, &freshPaint);
   } else {
-    canvas->drawImage(last_image_.get(), 0, 0, &paint);
+    canvas->drawImage(last_image_.get(), 0, 0, &freshPaint);
   }
 
   if (need_transform)
@@ -432,8 +434,12 @@ void SkCanvasVideoRenderer::Paint(const scoped_refptr<VideoFrame>& video_frame,
 void SkCanvasVideoRenderer::Copy(const scoped_refptr<VideoFrame>& video_frame,
                                  SkCanvas* canvas,
                                  const Context3D& context_3d) {
-  Paint(video_frame, canvas, gfx::RectF(video_frame->visible_rect()), 0xff,
-        SkXfermode::kSrc_Mode, media::VIDEO_ROTATION_0, context_3d);
+  SkPaint paint;
+  paint.setXfermodeMode(SkXfermode::kSrc_Mode);
+  paint.setAlpha(0xFF);

[Justin Novosad, 2016/08/25 19:18:18]

not necessary

[xidachen, 2016/08/26 02:21:39]

Done.

+  paint.setFilterQuality(kLow_SkFilterQuality);

[Justin Novosad, 2016/08/25 19:18:18]

not necessary

[xidachen, 2016/08/26 02:21:39]

But without this line, the default filter quality is kNone.

+  Paint(video_frame, canvas, gfx::RectF(video_frame->visible_rect()), &paint,
+        media::VIDEO_ROTATION_0, context_3d);
 }
 
 namespace {