globalCompositeOperation is ignored in stroke, strokeRect.

In Blink project, globalCompositeOperation (blending and compositing) features were already implemented.
The features works well in fill APIs but it still isn't supported in stroke APIs.
So, we make sure of supporting the features in stroke APIs.

BUG=117675, 351178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171356

[zino, 2014-04-07 08:05:18]

Please take a look.

Thank you.

[krit, 2014-04-07 08:10:35]

not LGTM

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

https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1075:
didDraw(path.boundingRect());
It think you should make the whole canvas for repaint, not just the bounding
box. These compositing operators effect more than just the path bounding box.

https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1374: didDraw(rect);
Ditto.

[zino, 2014-04-07 11:20:10]

On 2014/04/07 08:10:35, krit wrote:
> not LGTM
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1075:
> didDraw(path.boundingRect());
> It think you should make the whole canvas for repaint, not just the bounding
> box. These compositing operators effect more than just the path bounding box.
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1374: didDraw(rect);
> Ditto.

You're right! It is my mistake.

Thank you for comments.

Could you please review again?

[Justin Novosad, 2014-04-07 15:50:30]

https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):

https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1364:
boundingRect.inflate(state().m_lineWidth / 2);
I think that with this line of code removed State::m_lineWidth may now be
completely useless, and should be removed.

https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
File Source/core/html/canvas/CanvasRenderingContext2D.h (right):

https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
Source/core/html/canvas/CanvasRenderingContext2D.h:357: };
These functors add unnecessary weight to the header file. The Functor template
parameter is redundant with the T template parameter (non-orthogonal). Instead
you should use a template specialization. No need to specializ all of
"fullCanvasComposited". You can have just a tiny pecialization that contans the
same calls as these functors.

[zino, 2014-04-07 17:25:05]

On 2014/04/07 15:50:30, junov wrote:
>
https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (left):
> 
>
https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1364:
> boundingRect.inflate(state().m_lineWidth / 2);
> I think that with this line of code removed State::m_lineWidth may now be
> completely useless, and should be removed.

Done.

> 
>
https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.h (right):
> 
>
https://codereview.chromium.org/227213002/diff/80001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.h:357: };
> These functors add unnecessary weight to the header file. The Functor template
> parameter is redundant with the T template parameter (non-orthogonal). Instead
> you should use a template specialization. No need to specializ all of
> "fullCanvasComposited". You can have just a tiny pecialization that contans
the
> same calls as these functors.

Done.

Thank you for reviews!

[Justin Novosad, 2014-04-07 18:03:14]

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

https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:232: , m_lineWidth(1)
You responded "Done", but m_lineWidth is still here. That is okay. I take back
my earlier comment. m_lineWidth is still useful for retaining state when there
is no drawingContext.

https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1680: template<class T>
void CanvasRenderingContext2D::fullCanvasComposite(const T& area, void
(GraphicsContext::*drawOperation)(const T&))
drawOperation is an unnecessary indirection.  A simple template specialization
would suffice. like this:

template<typename T> void fill(GraphicsContext*, const T&)
{
    ASSERT_NOT_REACHED();
}

template<> void fill<FloatRect>(GraphicsContext* c, const FloatRect& rect)
{
    c->fillRect(rect);
}

[zino, 2014-04-07 19:17:04]

On 2014/04/07 18:03:14, junov wrote:
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:232: , m_lineWidth(1)
> You responded "Done", but m_lineWidth is still here. That is okay. I take back
> my earlier comment. m_lineWidth is still useful for retaining state when there
> is no drawingContext.
> 
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1680: template<class T>
> void CanvasRenderingContext2D::fullCanvasComposite(const T& area, void
> (GraphicsContext::*drawOperation)(const T&))
> drawOperation is an unnecessary indirection.  A simple template specialization
> would suffice. like this:
> 
> template<typename T> void fill(GraphicsContext*, const T&)
> {
>     ASSERT_NOT_REACHED();
> }
> 
> template<> void fill<FloatRect>(GraphicsContext* c, const FloatRect& rect)
> {
>     c->fillRect(rect);
> }

I'm sorry that I don't know well. So, I need more detailed explanations.

I thought a lambda function is the best because we should apply four types of
drawing operation (fillRect, fillPath, strokeRect, strokePath) to the same
logic.

But blink doesn't support C++ 11 standards. Instead I implemented as functor
template and indirection.

It looked not good to you. I also agreed with you but you left some questions to
me.

I was wondering why we should use template specialization instead of method
overloading. (like fillPrimitive)

Also, If we use template specialization or method overloading, then should we
write two methods which have the same logic?

(e.g. fullCanvasCompositedFill and fullCanvasCompositedStroke)

Could you please reply again.

Thank you :)

[zino, 2014-04-09 14:23:15]

It seems "WTF::Closure class" has been already exist.
I've just reimplemented a new patch by using the class.
I was wondering what your good opinions is.

Could you please review again?

Thank you :)

[Justin Novosad, 2014-04-09 18:56:39]

On 2014/04/07 19:17:04, zino wrote:
> On 2014/04/07 18:03:14, junov wrote:
> >
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:232: , m_lineWidth(1)
> > You responded "Done", but m_lineWidth is still here. That is okay. I take
back
> > my earlier comment. m_lineWidth is still useful for retaining state when
there
> > is no drawingContext.
> > 
> >
>
https://codereview.chromium.org/227213002/diff/110001/Source/core/html/canvas...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:1680: template<class T>
> > void CanvasRenderingContext2D::fullCanvasComposite(const T& area, void
> > (GraphicsContext::*drawOperation)(const T&))
> > drawOperation is an unnecessary indirection.  A simple template
specialization
> > would suffice. like this:
> > 
> > template<typename T> void fill(GraphicsContext*, const T&)
> > {
> >     ASSERT_NOT_REACHED();
> > }
> > 
> > template<> void fill<FloatRect>(GraphicsContext* c, const FloatRect& rect)
> > {
> >     c->fillRect(rect);
> > }
> 
> I'm sorry that I don't know well. So, I need more detailed explanations.
> 
> I thought a lambda function is the best because we should apply four types of
> drawing operation (fillRect, fillPath, strokeRect, strokePath) to the same
> logic.
> 
> But blink doesn't support C++ 11 standards. Instead I implemented as functor
> template and indirection.
> 
> It looked not good to you. I also agreed with you but you left some questions
to
> me.
> 
> I was wondering why we should use template specialization instead of method
> overloading. (like fillPrimitive)
> 
> Also, If we use template specialization or method overloading, then should we
> write two methods which have the same logic?
> 
> (e.g. fullCanvasCompositedFill and fullCanvasCompositedStroke)
> 
> Could you please reply again.
> 
> Thank you :)

You are right, a template is not necessary. Function overloading would be fine
here, just keep them outside of the header file.

[Justin Novosad, 2014-04-09 18:58:01]

On 2014/04/09 14:23:15, zino wrote:
> It seems "WTF::Closure class" has been already exist.
> I've just reimplemented a new patch by using the class.
> I was wondering what your good opinions is.
> 
> Could you please review again?
> 
> Thank you :)

I think Closure is overkill for this situation.

[zino, 2014-04-11 05:36:19]

On 2014/04/09 18:58:01, junov wrote:
> On 2014/04/09 14:23:15, zino wrote:
> > It seems "WTF::Closure class" has been already exist.
> > I've just reimplemented a new patch by using the class.
> > I was wondering what your good opinions is.
> > 
> > Could you please review again?
> > 
> > Thank you :)
> 
> I think Closure is overkill for this situation.

I got it. I uploaded a new patch w/o Closure.
Please take a look.

Thank you.

[Justin Novosad, 2014-04-11 12:54:15]

On 2014/04/11 05:36:19, zino wrote:
> On 2014/04/09 18:58:01, junov wrote:
> > On 2014/04/09 14:23:15, zino wrote:
> > > It seems "WTF::Closure class" has been already exist.
> > > I've just reimplemented a new patch by using the class.
> > > I was wondering what your good opinions is.
> > > 
> > > Could you please review again?
> > > 
> > > Thank you :)
> > 
> > I think Closure is overkill for this situation.
> 
> I got it. I uploaded a new patch w/o Closure.
> Please take a look.
> 
> Thank you.

lgtm

[zino, 2014-04-11 13:11:17]

On 2014/04/07 08:10:35, krit wrote:
> not LGTM
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1075:
> didDraw(path.boundingRect());
> It think you should make the whole canvas for repaint, not just the bounding
> box. These compositing operators effect more than just the path bounding box.
> 
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1374: didDraw(rect);
> Ditto.

@Dirk,

I already addressed your comments.
Would you mind if I commit this CL?

[krit, 2014-04-11 13:16:31]

On 2014/04/11 13:11:17, zino wrote:
> On 2014/04/07 08:10:35, krit wrote:
> > not LGTM
> > 
> >
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> > File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):
> > 
> >
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:1075:
> > didDraw(path.boundingRect());
> > It think you should make the whole canvas for repaint, not just the bounding
> > box. These compositing operators effect more than just the path bounding
box.
> > 
> >
>
https://codereview.chromium.org/227213002/diff/50001/Source/core/html/canvas/...
> > Source/core/html/canvas/CanvasRenderingContext2D.cpp:1374: didDraw(rect);
> > Ditto.
> 
> @Dirk,
> 
> I already addressed your comments.
> Would you mind if I commit this CL?

LGTM as well

[zino, 2014-04-11 13:54:45]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-11 13:54:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/227213002/170001

[commit-bot: I haz the power, 2014-04-11 14:32:14]

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

[commit-bot: I haz the power, 2014-04-11 14:32:15]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[zino, 2014-04-11 16:21:39]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-11 16:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/227213002/190001

[zino, 2014-04-11 16:29:38]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2014-04-11 16:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jinho.bang@samsung.com/227213002/190001

[commit-bot: I haz the power, 2014-04-11 17:24:25]

Change committed as 171356