Refine toRenderSVGResourceContainer()

Refine toRenderSVGResourceContainer()

As other toFooElements do, this patch checks security vulnerability in every use of toRenderSVGResourceContainer.
Plus, small binary size gain on content_shell(linux 64bit release build) by 0.17K
as it removes same function from RenderObject.

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

[kangil_, 2013-10-17 03:06:45]

review please? :)

[pdr., 2013-10-17 21:12:51]

Nice patch. LGTM with one minor change.

https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):

https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
Source/core/rendering/svg/SVGRenderTreeAsText.cpp:486:
RenderSVGResourceContainer* resource =
toRenderSVGResourceContainer(element->renderer());
I think you should use object here instead of element->renderer()

[kangil_, 2013-10-18 02:37:07]

On 2013/10/17 21:12:51, pdr wrote:
> Nice patch. LGTM with one minor change.
> 
>
https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
> File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):
> 
>
https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
> Source/core/rendering/svg/SVGRenderTreeAsText.cpp:486:
> RenderSVGResourceContainer* resource =
> toRenderSVGResourceContainer(element->renderer());
> I think you should use object here instead of element->renderer()

Thx for review. :)
Seems there is no address-of operator provided by RenderObject class as well as
std::addressof doesn't work on this.
Do you have some suggestions of RenderObject& type casting to take your comment
into consideration? :)

[pdr., 2013-10-18 03:00:12]

On 2013/10/18 02:37:07, kangil_ wrote:
> On 2013/10/17 21:12:51, pdr wrote:
> > Nice patch. LGTM with one minor change.
> > 
> >
>
https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
> > File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):
> > 
> >
>
https://codereview.chromium.org/27648002/diff/1/Source/core/rendering/svg/SVG...
> > Source/core/rendering/svg/SVGRenderTreeAsText.cpp:486:
> > RenderSVGResourceContainer* resource =
> > toRenderSVGResourceContainer(element->renderer());
> > I think you should use object here instead of element->renderer()
> 
> Thx for review. :)
> Seems there is no address-of operator provided by RenderObject class as well
as
> std::addressof doesn't work on this.
> Do you have some suggestions of RenderObject& type casting to take your
comment
> into consideration? :)

I think you may just be hitting const correctness issues. Try this out:
toRenderSVGResourceContainer(const_cast<RenderObject*>(&object));

SVGRenderTreeAsText is just a class for debugging so lets not spend too much
time on it. Feel free to commit your patch as-is (with element->renderer()) if
you run into difficulty.

[kangil_, 2013-10-18 03:21:16]

> I think you may just be hitting const correctness issues. Try this out:
> toRenderSVGResourceContainer(const_cast<RenderObject*>(&object));

Ah, thx! Done. :)

> SVGRenderTreeAsText is just a class for debugging so lets not spend too much
> time on it. Feel free to commit your patch as-is (with element->renderer()) if
> you run into difficulty.

Agreed. :)

[commit-bot: I haz the power, 2013-10-18 03:21:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangil.han@samsung.com/27648002/24001

[commit-bot: I haz the power, 2013-10-18 18:08:57]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-20 23:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangil.han@samsung.com/27648002/24001

[commit-bot: I haz the power, 2013-10-20 23:34:05]

Failed to apply patch for
Source/core/rendering/svg/RenderSVGResourceContainer.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/svg/RenderSVGResourceContainer.h
  Hunk #1 FAILED at 36.
  Hunk #2 succeeded at 97 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/svg/RenderSVGResourceContainer.h.rej

Patch:       Source/core/rendering/svg/RenderSVGResourceContainer.h
Index: Source/core/rendering/svg/RenderSVGResourceContainer.h
diff --git a/Source/core/rendering/svg/RenderSVGResourceContainer.h
b/Source/core/rendering/svg/RenderSVGResourceContainer.h
index
c63c09d3bb6de85cd291ffca554e95bb264725c1..7ef3a4078e486bb6efd6787e4cfe261620ea1ba0
100644
--- a/Source/core/rendering/svg/RenderSVGResourceContainer.h
+++ b/Source/core/rendering/svg/RenderSVGResourceContainer.h
@@ -36,8 +36,6 @@ public:
     virtual void layout();
     virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle)
OVERRIDE FINAL;
 
-    virtual RenderSVGResourceContainer* toRenderSVGResourceContainer() OVERRIDE
FINAL { return this; }
-
     static bool shouldTransformOnTextPainting(RenderObject*, AffineTransform&);
     static AffineTransform transformOnNonScalingStroke(RenderObject*, const
AffineTransform& resourceTransform);
 
@@ -96,6 +94,21 @@ Renderer* getRenderSVGResourceById(Document& document, const
AtomicString& id)
     return 0;
 }
 
+inline RenderSVGResourceContainer* toRenderSVGResourceContainer(RenderObject*
object)
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(!object ||
object->isSVGResourceContainer());
+    return static_cast<RenderSVGResourceContainer*>(object);
+}
+
+inline const RenderSVGResourceContainer* toRenderSVGResourceContainer(const
RenderObject* object)
+{
+    ASSERT_WITH_SECURITY_IMPLICATION(!object ||
object->isSVGResourceContainer());
+    return static_cast<const RenderSVGResourceContainer*>(object);
+}
+
+// This will catch anyone doing an unnecessary cast.
+void toRenderSVGResourceContainer(const RenderSVGResourceContainer*);
+
 }
 
 #endif

[commit-bot: I haz the power, 2013-10-21 00:35:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangil.han@samsung.com/27648002/204001

[commit-bot: I haz the power, 2013-10-21 02:12:02]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-10-21 02:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kangil.han@samsung.com/27648002/204001

[commit-bot: I haz the power, 2013-10-21 02:54:26]

Change committed as 160042