Implement interfaces in PaintInfo and make it a class.

Implement interfaces in PaintInfo and make it a class.

PaintInfo is a struct used all around in rendering, there is a FIXME that dates way 
back to August 2011 to implement interfaces on it. This patch implement setters/getters as needed by 
current rendering/svg classes.

It is interesting to observe that in several places, PaintInfo users were using a const pointer
to access its members and call non const methods on them (thus making the use of const qualifier in the
PaintInfo pointer at best useless and at worst misleading).

Attention to RenderSVGResourceFilter class since its apply/postApplyResource methods may write into 
the received context, thus the need to restore the GraphicsContext in PaintInfo.

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

[Savago-old, 2013-07-31 23:58:49]

It is simple, but a long patch.

It implements setters/getters in PaintInfo (as marked in a FIXME that dates way
back to 2011).

[do-not-use, 2013-08-01 18:50:14]

I haven't looked at everything yet because the CL is big. But it is important to
update the API to follow the coding style.

https://codereview.chromium.org/21430003/diff/6001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContextAnnotation.h (right):

https://codereview.chromium.org/21430003/diff/6001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContextAnnotation.h:93: void
annotate(PaintInfo&, const RenderObject*);
I think it would be nice to keep the const here since we don't modifier the
object.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
File Source/core/rendering/PaintInfo.h (right):

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:106: GraphicsContext* getContext() { return
context; }
We usually don't use 'get' prefix for getters, so this would be called
"context()".

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:107: void setContext(GraphicsContext
*acontext) { context = acontext; }
* on wrong side.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:107: void setContext(GraphicsContext
*acontext) { context = acontext; }
acontext -> context once you rename the member to m_context.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:110: void setRect(const IntRect &arect) { rect
= arect; }
Space should be after the &, not before.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:122: const RenderLayerModelObject*
getPaintContainer() { return paintContainer; }
Getter can be const since you return a const pointer.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:125: GraphicsContext* context;
now that those are private, we should really use m_ prefix.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/svg/...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/svg/...
Source/core/rendering/svg/SVGRenderingContext.cpp:61: GraphicsContext* context =
m_paintInfo->getContext();
Why this new variable?

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/svg/...
Source/core/rendering/svg/SVGRenderingContext.cpp:151:
m_paintInfo->setContext(context);
Why these new calls to setContext()? Since applyResource() takes an
GraphicsContext*& in argument, this should not be needed.

[Savago-old, 2013-08-02 14:54:11]

Christophe

Thanks a lot for the review. I will comment ASAP.

[Savago-old, 2013-08-02 14:56:55]

https://codereview.chromium.org/21430003/diff/6001/Source/core/platform/graph...
File Source/core/platform/graphics/GraphicsContextAnnotation.h (right):

https://codereview.chromium.org/21430003/diff/6001/Source/core/platform/graph...
Source/core/platform/graphics/GraphicsContextAnnotation.h:93: void
annotate(PaintInfo&, const RenderObject*);
On 2013/08/01 18:50:14, Christophe Dumez wrote:
> I think it would be nice to keep the const here since we don't modifier the
> object.

In the constructor it will access the PaintInfo's context, that is not a const
function.

An alternative would be to implement 
GraphicsContext* PaintInfo::context() and 
const GraphicsContext* PaintInfo::getContext() const.

What you think?

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
File Source/core/rendering/PaintInfo.h (right):

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:107: void setContext(GraphicsContext
*acontext) { context = acontext; }
On 2013/08/01 18:50:14, Christophe Dumez wrote:
> * on wrong side.
Ouch! You are right, I guess the check-style script forgot this one. I will fix
it.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:110: void setRect(const IntRect &arect) { rect
= arect; }
Same case.

https://codereview.chromium.org/21430003/diff/6001/Source/core/rendering/Pain...
Source/core/rendering/PaintInfo.h:125: GraphicsContext* context;
Nice suggestion, I will update the patch.

[do-not-use, 2013-08-05 11:16:29]

https://codereview.chromium.org/21430003/diff/17001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsContextAnnotation.h (right):

https://codereview.chromium.org/21430003/diff/17001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsContextAnnotation.h:63:
GraphicsContextAnnotation(PaintInfo&, const RenderObject*);
I guess we could have 2 context getters so that we can keep the const here:
GraphicsContext* context() { return m_context; }
const GraphicsContext* context() const { return m_context; }

https://codereview.chromium.org/21430003/diff/17001/Source/core/rendering/Pai...
File Source/core/rendering/PaintInfo.h (right):

https://codereview.chromium.org/21430003/diff/17001/Source/core/rendering/Pai...
Source/core/rendering/PaintInfo.h:106: GraphicsContext* getContext() { return
m_context; }
As explained in my previous review, we don't use "get" prefix for getters. This
should be named "context()". Same comment for getters below.

https://codereview.chromium.org/21430003/diff/17001/Source/core/rendering/svg...
File Source/core/rendering/svg/SVGRenderingContext.cpp (right):

https://codereview.chromium.org/21430003/diff/17001/Source/core/rendering/svg...
Source/core/rendering/svg/SVGRenderingContext.cpp:61: GraphicsContext* context =
m_paintInfo->getContext();
Useless new local variable?

https://codereview.chromium.org/21430003/diff/17001/Source/core/rendering/svg...
Source/core/rendering/svg/SVGRenderingContext.cpp:140:
m_paintInfo->setContext(context);
IMHO, this results in less readable code. Maybe the context() getter could
getter a GraphicsContext*& instead of a GraphicsContext* so that we can keep the
code as it was?

I'd like another owner to confirm that this would be acceptable though.

[Savago-old, 2013-08-06 19:19:48]

Thanks again for the review, the last version of patch addresses the issues you
listed: 

a) no longer using 'get' prefix on class member functions;
b) a const version for accessing the context;
c) a class member for handling that case in SVG where there is the need to
access a ref-to-pointer to update the graphic context in PaintInfo.

[eseidel, 2013-08-09 20:38:54]

OK, this is great.  BUT, we need to do this in tiny pieces for it to be
reviewable.

Could you do one of these functions at at time?  Just make one member private
and leave the others public?

Yes, I know that will be lots of patches (which I'm happy to review promptly),
but a patch this large is impossible to verify.

[eseidel, 2013-08-12 19:55:04]

lgtm

[commit-bot: I haz the power, 2013-08-12 19:55:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.cavalcanti@partner.samsung.com/21430...

[commit-bot: I haz the power, 2013-08-12 22:16:36]

Change committed as 155955