[webkit-reviews] review requested: [Bug 78631] getCTM() on SVG root element with borders, paddings, and viewbox returns incorrect values : [Attachment 127263] Patch

[bugzilla-daemon at webkit.org]

Max Vujovic <mvujovic at adobe.com> has asked  for review:
Bug 78631: getCTM() on SVG root element with borders, paddings, and viewbox
returns incorrect values
https://bugs.webkit.org/show_bug.cgi?id=78631

Attachment 127263: Patch
https://bugs.webkit.org/attachment.cgi?id=127263&action=review

------- Additional Comments from Max Vujovic <mvujovic at adobe.com>
(In reply to comment #4)
> (From update of attachment 127210 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=127210&action=review
> 
> Nice patch Max, almost there!

Thanks Nikolas!

> > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:12
> > +	     <script src="../../fast/js/resources/js-test-pre.js"></script>
> 
> I think it would be helpful, if this testcase would use
"window.enablePixelTesting = true" before including js-test-pre.js, to dump
pixels as well.

I've added this. Please forgive my newness, but what exactly does
window.enablePixelTesting do? 

> > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:32
> > +	     <svg id="svgRoot1" xmlns="http://www.w3.org/2000/svg"
> 
> Inside a HTML5 document, no xmlns is needed here.
> 
> > LayoutTests/svg/dom/SVGLocatable-getCTM-svg-root.html:34
> > +		  viewbox="0 0 100 100"">
> 
> Oh, when the HTML5 parses parses this file, lower case attributes are
allowed?? The correct name is 'viewBox'.
> I'm not sure if HTML5 explicitly allows this, if not please use "viewBox".

I kept this as "viewbox" since the convention for HTML5 seems to be keeping all
attribute names lowercase. 

The closest mention I found in the HTML5 spec was section 4.14.1
Case-sensitivity, which implies that "viewbox" should work:
"Attribute and element names of HTML elements in HTML documents must be treated
as ASCII case-insensitive for the purposes of selector matching."
(http://www.whatwg.org/specs/web-apps/current-work/multipage/selectors.html#cas
e-sensitivity)

Some search results I found suggest that lowercase is the convention:
"HTML5 attributes are case insensitive and may be written in all uppercase or
mixed case, although the most common convention is to stick with lowercase."
(http://www.tutorialspoint.com/html5/html5_syntax.htm)

> > Source/WebCore/ChangeLog:22
> > +		 SVGSVGElement::currentViewportSize was using the SVG root
element's frameRect to compute
> > +		 the viewport size. However, the frameRect includes borders and
paddings, which should
> > +		 not be considered part of the SVG viewport.
> > +
> > +		 SVGSVGElement::currentViewportSize now uses the contentBoxRect
instead of the frameRect.
> > +		 The contentBoxRect corresponds to the SVG viewport and does
not include borders and
> > +		 paddings.
> 
> Double explanation, either is okay.

Removed the first paragraph. 

> > Source/WebCore/svg/SVGSVGElement.cpp:557
> >	 FloatRect frameRect =
toRenderSVGViewportContainer(renderer())->viewport();
> 
> I'd rename this as well for consistency to avoid confusion.

Good idea. I renamed "frameRect" to "viewportRect" in this section.