[Webkit-unassigned] [Bug 11272] Implement currentScale(), setCurrentScale() and currentTranslate() in SVGSVGElement

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=11272





------- Comment #11 from rwlbuis at gmail.com  2007-05-30 08:34 PDT -------
Hi Darin,

(In reply to comment #10)
> (From update of attachment 14782 [edit])
> 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.

Right, Text zoom seems very specific and doesn't translate well to the svg
world. My original patch had them separated.

> +    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".

Fixed.

> +        document()->frame()->setZoomFactor(scale / 100.);
> 
> This will do the math as double. You want "scale / 100.0f".

Fixed.

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

Fixed.

> -    //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.

Its being removed here :)

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

No particular reason. Fixed.

> +       static_cast<SVGDocument*>(m_frame->document())->zoomEnabled()) {
> 
> Why does zoomEnabled() control panning? Should it be named zoomAndPanEnabled()
> instead?

Agreed. Fixed.

> +        AffineTransform ctm;
> 
> This local variable is unused?

Maybe copy and paste error. Fixed.

> +        m_width = float(m_width) * svg->currentScale();
> +        m_height = float(m_height) * svg->currentScale();
> 
> No need for the cast to float here.

Fixed.
Cheers,

Rob.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.