Removing the dependency on GraphicsContext for drawing images in 2D canvas

Removing the dependency on GraphicsContext for drawing images in 2D canvas

Is change is the final step in removing the GraphicsContext dependency
from CanvasRenderingContext2D. It changes blink::Image and all of its
child classes to make them draw directly to SkCanvas without a
GraphicsContext.  The interactions between GraphicsContext and
Image subclasses are also simplified because the SkCanvas is no
longer encapsulated, which was not possible before because
GraphicsContext used to be an abstraction layer back in the WebKit
days.

BUG=453113
TEST=all existing layout tests that use images and/or canvases.
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196424

[Justin Novosad, 2015-04-16 18:12:35]

junov@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Justin Novosad, 2015-04-16 18:12:36]

PTAL

Note: This patch is still missing "NeedsRebaseline", and theres going to be lots
of them because I moved around some floating point arithmetics which I expect to
have some epsilon issues. I will set the NeedsRebaselines, once the try bots
have returned results, which I will inspect.

[Justin Novosad, 2015-04-16 18:15:31]

https://codereview.chromium.org/1093673002/diff/1/Source/core/html/canvas/Can...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):

https://codereview.chromium.org/1093673002/diff/1/Source/core/html/canvas/Can...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:64: #include
"platform/graphics/GraphicsContext.h"
I enjoyed deleting this line. Booyah! \o/

[Stephen White, 2015-04-17 15:12:03]

senorblanco@chromium.org changed reviewers:
+ fmalita@chromium.org, schenney@chromium.org

[Stephen White, 2015-04-17 15:12:17]

On 2015/04/16 18:15:31, junov wrote:
>
https://codereview.chromium.org/1093673002/diff/1/Source/core/html/canvas/Can...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):
> 
>
https://codereview.chromium.org/1093673002/diff/1/Source/core/html/canvas/Can...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:64: #include
> "platform/graphics/GraphicsContext.h"
> I enjoyed deleting this line. Booyah! \o/

w00t!

[Stephen White, 2015-04-17 15:14:56]

Stephen C. and/or Florin should take a look at this too.
Looks good for the parts I know about (once layout tests are happy with it).

My comments are mostly minor. The image resampling modes are kind of tricky and
don't have great perf tests; if you could check performance on
http://ie.microsoft.com/testdrive/Performance/01FlyingImages/Default.html before
and after your change, that would be great.

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/HTMLVi...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/HTMLVi...
Source/core/html/HTMLVideoElement.cpp:212: // TODO(junov, philipj): pass the
whole SkPaint instead of only alpha and xfermode
Please add a reference in the comment to the video/shadow bug this is going to
help fix.

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/canvas...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1248:
paint->getImageFilter()->computeFastBounds(bounds, &filteredBounds);
Nit: I realize this is just moving code from elsewhere, but you should be able
to call computeFastBounds() directly on the paint, and it will defer to the
filter.

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1252:
c->saveLayer(&filteredBounds, &layerPaint);
It seems unfortunate to have to call saveLayer() yourself for this case, since
Skia can do it internally. However, as we discussed, you have to do it because
the compositing mode is applied by skia to the offscreen primitive draw, not to
the layer paint.

I've logged skbug.com/3741 to see if we can change the Skia implementation to
match the above semantics.

https://codereview.chromium.org/1093673002/diff/40001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2039: #if OS(MACOSX)
Nit: I wonder if we should keep this stuff in platform (some kind of
SkiaUtils::drawFocusRing maybe?) so we don't have to do platform-specific
#ifdefs here.

[f(malita), 2015-04-17 16:18:32]

Nice! Non-CRC2D LGTM w/ nits.

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/CrossfadeGeneratedImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/CrossfadeGeneratedImage.cpp:55: compositeOp =
SkXfermode::kSrcOver_Mode;
Nit: how about just

  layerPaint.setXfermode(paint.getXfermode())

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/CrossfadeGeneratedImage.cpp:70: canvas->restore();
Nit: use SkAutoCanvasRestore instead?

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/CrossfadeGeneratedImage.cpp:79: canvas->save();
Nit: same here

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GradientGeneratedImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/GradientGeneratedImage.cpp:36: canvas->save();
Nit: ditto

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.cpp:1022: // Take into account scale
applied to the canvas when computing sampling mode (e.g. CSS scale or page
scale).
Not new to this CL, but let's add a comment here: in SP we create GCs on the fly
when entering a new DisplayItemList scope, so relying on getTotalMatrix here is
not sound.

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (left):

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:158: void setAlphaAsFloat(float
alpha) { mutableState()->setAlphaAsFloat(alpha);}
Yes!!! global alpha begone!

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1093673002/diff/40001/Source/platform/graphic...
Source/platform/graphics/GraphicsContext.h:160: SkXfermode::Mode
compositeOperation() const;
Are these still used now?

[Justin Novosad, 2015-05-27 00:27:03]

junov@chromium.org changed reviewers:
+ pdr@chromium.org

[Justin Novosad, 2015-05-27 00:27:04]

New Patch!
Since previous review (patch set 3) I have done:
1. A whole lot of rebasing, so don't look at the inter-diffs because they are
too noisy.
2. I applied all of Florin's review comments
3. I fixed a few subtle rendering bugs, the most significant of which is to use
the correct anti-aliasing setting. See shouldUseAntiAliasing methods in GC.cpp
and CRC2D.cpp, and the old version of that stuff which I removed from GC.cpp and
SkiaUtils.cpp

[Justin Novosad, 2015-05-27 00:27:48]

+pdr for core/svg

[pdr., 2015-05-27 04:23:30]

core/svg LGTM with a nit.

https://codereview.chromium.org/1093673002/diff/160001/Source/core/svg/graphi...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/160001/Source/core/svg/graphi...
Source/core/svg/graphics/SVGImage.cpp:308: bool needsLayer = false;
Can you move this needsLayer code into a helper function?

[Justin Novosad, 2015-05-27 14:23:51]

New Patch: applied pdr's comment + added NeedsRebaseline on two svg layout
tests. Diffs look like probable epsilon propagation issues resulting from
shuffling floating-point code. Nothing worrysome.

senorblanco: core/html

Thx.

[Stephen White, 2015-05-27 15:46:02]

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (left):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:687:
m_imageBuffer->context()->setImageInterpolationQuality(CanvasDefaultInterpolationQuality);
Is this still used / necessary? If we're no longer using the GC, we should be
setting this on the new SkCanvas instead, no?

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:292: notifyObserversCanvasChanged(rect);
Out of curiosity, is this a bugfix / optimization?

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLV...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLV...
Source/core/html/HTMLVideoElement.cpp:217: // TODO(junov, philipj): pass the
whole SkPaint instead of only alpha and xfermode
Please add a reference in the comment to the video/shadow bug this is going to
help fix.

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/canva...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/canva...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2086: #if OS(MACOSX)
Nit: IWBN to relocate these platform-specific differences to SkiaUtils, or
somewhere other in the platform/graphics area.

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:144: virtual void drawPattern(GraphicsContext*,
const FloatRect&,
Perhaps at some point we could circle back and change the other GraphicsContext
references here to take an SkCanvas. Maybe it's just my OCD, but it feels weird
having a mix of the two in Image's public API.

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:152: virtual void draw(SkCanvas*, const
SkPaint&, const FloatRect& dstRect, const FloatRect& srcRect,
RespectImageOrientationEnum, const ImageFilterQualityHelper*) = 0;
To be honest, I'm not a fan of this callback object. I'd really prefer if we
could pass in some dumb state (SkPaint plus additional struct or params?). Maybe
I'm being optimistic, but it seems strange that we can't infer the desired
behaviour from passed-in state combined with specialization in the draw()
virtual on each Image subclass.

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
File Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
Source/platform/graphics/StaticBitmapImage.cpp:41: FloatRect normDstRect =
adjustForNegativeSize(dstRect);
This may be redundant with computations done up in CRC2D, and could be replaced
with an assert, in order to allow us to get rid of the helper object. (Your
idea, I'm just reminding you. :) )

[Justin Novosad, 2015-05-27 20:08:16]

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (left):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:687:
m_imageBuffer->context()->setImageInterpolationQuality(CanvasDefaultInterpolationQuality);
On 2015/05/27 15:46:02, Stephen White wrote:
> Is this still used / necessary? If we're no longer using the GC, we should be
> setting this on the new SkCanvas instead, no?

Acknowledged.

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:292: notifyObserversCanvasChanged(rect);
On 2015/05/27 15:46:02, Stephen White wrote:
> Out of curiosity, is this a bugfix / optimization?
Drive-by optimization. We use to do these notification per frame instead of per
draw, but that led to sync issues. Before, when it was per frame, m_dirtyRect
was the right thing. This change probably won't make any significant difference
except in degenerate  cases of -webkit-canvas

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLV...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/HTMLV...
Source/core/html/HTMLVideoElement.cpp:217: // TODO(junov, philipj): pass the
whole SkPaint instead of only alpha and xfermode
On 2015/05/27 15:46:02, Stephen White wrote:
> Please add a reference in the comment to the video/shadow bug this is going to
> help fix.

Acknowledged.

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/canva...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/core/html/canva...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:2086: #if OS(MACOSX)
On 2015/05/27 15:46:02, Stephen White wrote:
> Nit: IWBN to relocate these platform-specific differences to SkiaUtils, or
> somewhere other in the platform/graphics area.

Looking at the GC counterpart of this code... I smell a lambda. Mouahahaha

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
Source/platform/graphics/Image.h:144: virtual void drawPattern(GraphicsContext*,
const FloatRect&,
On 2015/05/27 15:46:02, Stephen White wrote:
> Perhaps at some point we could circle back and change the other
GraphicsContext
> references here to take an SkCanvas. Maybe it's just my OCD, but it feels
weird
> having a mix of the two in Image's public API.

Yeah, it is not required to enact the separation, but it would feel cleaner, I
agree. created crbug.com/492794

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
File Source/platform/graphics/StaticBitmapImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/180001/Source/platform/graphi...
Source/platform/graphics/StaticBitmapImage.cpp:41: FloatRect normDstRect =
adjustForNegativeSize(dstRect);
On 2015/05/27 15:46:02, Stephen White wrote:
> This may be redundant with computations done up in CRC2D, and could be
replaced
> with an assert, in order to allow us to get rid of the helper object. (Your
> idea, I'm just reminding you. :) )

Yep. I tried it in a debug build and there are no asserts. This code is just a
remnant of an old way of doing things. Yay

[chrishtr, 2015-05-28 21:08:24]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2015-05-28 21:08:25]

What about the use case where HTMLCanvasElement creates an ImageBuffer, which in
turn relies on GraphicsContext?

[Justin Novosad, 2015-05-29 15:10:38]

On 2015/05/28 21:08:25, chrishtr wrote:
> What about the use case where HTMLCanvasElement creates an ImageBuffer, which
in
> turn relies on GraphicsContext?

That GraphicsContext is no longer used after the CL.  There are a few other
non-canvas call sites that use the GC provided by ImageBuffer.  These are simple
uses that will be easy to refactor. In a follow-up CL I intend to clean those
up.

[Justin Novosad, 2015-05-29 16:20:40]

New patch.

[Justin Novosad, 2015-05-29 16:24:40]

Most significant diff wrt previous patch is that I factored out the filter
quality helper objects which were overkill and added unnecessary complexity. 
However, without them, it is not possible to make the filter quality heuristics
behave 100% the same as before, but I still thing it's a good call. So I had to
add a few more needsrebaselines

[Stephen White, 2015-05-29 17:26:37]

https://codereview.chromium.org/1093673002/diff/200001/Source/core/html/HTMLC...
File Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/1093673002/diff/200001/Source/core/html/HTMLC...
Source/core/html/HTMLCanvasElement.cpp:681:
toCanvasRenderingContext2D(m_context.get())->setShouldAntialias(!disableAntiAliasing);
Perhaps we should make the default for CRC2D "should antialias" to be true. Then
we can just disable it here if the flags request it, as before.

https://codereview.chromium.org/1093673002/diff/200001/Source/core/svg/graphi...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/200001/Source/core/svg/graphi...
Source/core/svg/graphics/SVGImage.cpp:217: bool shouldClampToSourceRect = true;
Nit: I think you're obeying the letter of the rule, but not the spirit, here. :)
Consider an enum instead of a bool.

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/DragIm...
File Source/platform/DragImageTest.cpp (right):

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/DragIm...
Source/platform/DragImageTest.cpp:95: void draw(SkCanvas*, const SkPaint&, const
FloatRect&, const FloatRect&, RespectImageOrientationEnum, bool) override
Nit: please name the param here.

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/graphi...
File Source/platform/graphics/GraphicsContext.h (right):

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/graphi...
Source/platform/graphics/GraphicsContext.h:287: bool shouldAntialiasImages() {
return shouldAntialias() && !getTotalMatrix().rectStaysRect();}
Aside: not new to this code, but this feels like an optimization that should be
in Skia (or CC), rather than Blink. But perhaps I'm misunderstanding its
purpose.

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/graphi...
File Source/platform/graphics/ImageLayerChromiumTest.cpp (right):

https://codereview.chromium.org/1093673002/diff/200001/Source/platform/graphi...
Source/platform/graphics/ImageLayerChromiumTest.cpp:88: void draw(SkCanvas*,
const SkPaint&, const FloatRect&, const FloatRect&, RespectImageOrientationEnum,
bool) override
Nit: Please name the param here.

https://codereview.chromium.org/1093673002/diff/220001/Source/platform/graphi...
File Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1093673002/diff/220001/Source/platform/graphi...
Source/platform/graphics/skia/SkiaUtils.h:138: void drawPlatformFocusRing(const
DrawFunc& drawFunc, SkColor color, int width)
It looks like the two call sites which draw focus rings as paths provide almost
identical lambdas.

Rather than exposing the functor as an API here, could we provide two functions
here in SkiaUtils (drawPlatformFocusRingPath and drawPlatformFocusRingRect)
which use a lambda internally if necessary?

[Justin Novosad, 2015-05-29 21:25:30]

Done.

[Stephen White, 2015-05-29 21:38:52]

LGTM w/enum nit fixed

https://codereview.chromium.org/1093673002/diff/240001/Source/platform/graphi...
File Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/1093673002/diff/240001/Source/platform/graphi...
Source/platform/graphics/Image.h:146: DoNotClampImage
Nit: DoNotClampImageToSourceRect?

https://codereview.chromium.org/1093673002/diff/240001/Source/platform/graphi...
File Source/platform/graphics/skia/SkiaUtils.h (right):

https://codereview.chromium.org/1093673002/diff/240001/Source/platform/graphi...
Source/platform/graphics/skia/SkiaUtils.h:135: template<typename PrimitiveType>
Nit: I would prefer to have two non-templated functions in the header, which in
the .cpp wrap calls to the template instantiations, but now we're deep into
personal style and I've subjected you to too much of that already. :)

[Justin Novosad, 2015-05-29 21:42:07]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2015-05-29 21:42:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, pdr@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1093673002/#ps260001
(title: "bikeshed")

[commit-bot: I haz the power, 2015-05-29 22:01:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093673002/260001

[commit-bot: I haz the power, 2015-05-29 23:04:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-29 23:04:11]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Justin Novosad, 2015-06-01 19:30:06]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2015-06-01 19:30:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, pdr@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1093673002/#ps360001
(title: "fix merge error")

[commit-bot: I haz the power, 2015-06-01 19:30:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093673002/360001

[commit-bot: I haz the power, 2015-06-01 19:40:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-01 19:40:09]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64641)

[Justin Novosad, 2015-06-03 15:09:36]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2015-06-03 15:09:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, pdr@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/1093673002/#ps380001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-03 15:10:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093673002/380001

[commit-bot: I haz the power, 2015-06-03 17:40:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-03 17:40:24]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65047)

[Justin Novosad, 2015-06-03 19:09:19]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2015-06-03 19:10:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093673002/380001

[commit-bot: I haz the power, 2015-06-03 19:15:19]

Committed patchset #20 (id:380001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196424

[chrishtr, 2015-06-03 19:56:42]

\o/

[Noel Gordon, 2015-06-10 12:21:13]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2015-06-10 12:21:15]

Some comments re the other bug we are discussing about filters.

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:299: void BitmapImage::draw(SkCanvas*
canvas, const SkPaint& paint, const FloatRect& dstRect, const FloatRect&
srcRect, RespectImageOrientationEnum shouldRespectImageOrientation,
ImageClampingMode clampMode)
Previously, this routine created its SkPaint internally (line 332 left). 
Curious about why it needs to be passed in as a parameter now, and why const?

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:337: SkRect skSrcRect =
adjustedSrcRect;
Anyho, as we are discussion on another bug, I can interpolate an appropriate
SkColorFilter about here, and add it to the SkPaint ...

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:340:
canvas->drawBitmapRectToRect(bitmap, &skSrcRect, adjustedDstRect, &paint,
flags);
... and the drawBitmapRectToRect records the SkPaint.  Hence, my SkColorFilter
gets sent to the impl-side where it is applied to this bitmap image, right?

[Justin Novosad, 2015-06-10 13:46:02]

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
File Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:299: void BitmapImage::draw(SkCanvas*
canvas, const SkPaint& paint, const FloatRect& dstRect, const FloatRect&
srcRect, RespectImageOrientationEnum shouldRespectImageOrientation,
ImageClampingMode clampMode)
On 2015/06/10 12:21:15, noel gordon wrote:
> Previously, this routine created its SkPaint internally (line 332 left). 
> Curious about why it needs to be passed in as a parameter now, and why const?

The old code required a GraphicsContext object for setting-up the SkPaint (line
324). With the removal of the dependency on GraphicsContext, that is no longer
possible.  The SkPaint that is passed in is now how drawing parameters are
communicated to this function, instead of the call to
GraphicsContext::preparePaintForDrawRectToRect that existed before

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:337: SkRect skSrcRect =
adjustedSrcRect;
On 2015/06/10 12:21:15, noel gordon wrote:
> Anyho, as we are discussion on another bug, I can interpolate an appropriate
> SkColorFilter about here, and add it to the SkPaint ...

Yes, just create a local non-const copy of paint, and add the filter to it.

https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
Source/platform/graphics/BitmapImage.cpp:340:
canvas->drawBitmapRectToRect(bitmap, &skSrcRect, adjustedDstRect, &paint,
flags);
On 2015/06/10 12:21:15, noel gordon wrote:
> ... and the drawBitmapRectToRect records the SkPaint.  Hence, my SkColorFilter
> gets sent to the impl-side where it is applied to this bitmap image, right?

Yes

[Noel Gordon, 2015-06-10 15:43:46]

> On 2015/06/10 12:21:15, noel gordon wrote:
> > Previously, this routine created its SkPaint internally (line 332 left). 
> > Curious about why it needs to be passed in as a parameter now, and why
const?

> Justin noted.
> The old code required a GraphicsContext object for setting-up the SkPaint
(line
> 324). With the removal of the dependency on GraphicsContext, that is no longer
> possible.  

Right.

> The SkPaint that is passed in is now how drawing parameters are
> communicated to this function, instead of the call to
> GraphicsContext::preparePaintForDrawRectToRect that existed before

Yeap, I read back through the canvas code above, drop shadows etc in the paint,
makes sense.

[Noel Gordon, 2015-06-10 15:52:48]

On 2015/06/10 13:46:02, Justin Novosad wrote:

> On 2015/06/10 12:21:15, noel gordon wrote:
> > Anyho, as we are discussion on another bug, I can interpolate an appropriate
> > SkColorFilter about here, and add it to the SkPaint ...
> 
> Yes, just create a local non-const copy of paint, and add the filter to it.

Thanks Justin, works for me.

>
https://codereview.chromium.org/1093673002/diff/380001/Source/platform/graphi...
> Source/platform/graphics/BitmapImage.cpp:340:
> canvas->drawBitmapRectToRect(bitmap, &skSrcRect, adjustedDstRect, &paint,
> flags);
> On 2015/06/10 12:21:15, noel gordon wrote:
> > ... and the drawBitmapRectToRect records the SkPaint.  Hence, my
SkColorFilter
> > gets sent to the impl-side where it is applied to this bitmap image, right?
> 
> Yes

k, we'll call this the meet up at the pass point :) Thanks for your answers, and
nice patch btw.