[webkit-reviews] review denied: [Bug 11272] Implement currentScale(), setCurrentScale() and currentTranslate() in SVGSVGElement : [Attachment 14782] Without the webkit changes

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 30 07:50:03 PDT 2007


Darin Adler <darin at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 11272: Implement currentScale(), setCurrentScale() and currentTranslate()
in SVGSVGElement
http://bugs.webkit.org/show_bug.cgi?id=11272

Attachment 14782: Without the webkit changes
http://bugs.webkit.org/attachment.cgi?id=14782&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I don't think we should necessarily mix the zoom factor together with the text
size multiplier. The text size multiplier is still called "zoomFactor" in
Frame.cpp, but I think we may want a separate way to manipulate the zoom factor
and text size multiplier independently. And I think a zoom factor should zoom
everything, not just SVG content.

+    if (document() && document()->frame())
+	 return float(document()->frame()->zoomFactor()) / 100.;
+    return 1.0;

The right way to write the above so it will be floating point is:

    if (document() && document()->frame())
	return document()->frame()->zoomFactor() / 100.0f;
    return 1.0f;

There's no need for the cast to "float".

+	 document()->frame()->setZoomFactor(scale / 100.);

This will do the math as double. You want "scale / 100.0f".

The word "translate" is a verb not a noun. It should be "translation" instead
in the various function names.

-    //canvasView()->enableZoomAndPan(zoomAndPan == SVG_ZOOMANDPAN_MAGNIFY);

We normally don't check in commented-out code. if we must check this in, it
needs a comment to indicate why it's commented out.

	 return m_lastScrollbarUnderMouse->handleMouseMoveEvent(mouseEvent);
-
     // Treat mouse move events while the mouse is pressed as "read-only" in
prepareMouseEvent

Why remove this blank line?

+	static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) {

Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled()
instead?

+	 AffineTransform ctm;

This local variable is unused?

+	 m_width = float(m_width) * svg->currentScale();
+	 m_height = float(m_height) * svg->currentScale();

No need for the cast to float here.

review- at least for the "translate" name.



More information about the webkit-reviews mailing list