Bug 65092 – Don't set m_fontDirty when setting zoom unless zoom has actually changed

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Don't set m_fontDirty when setting zoom unless zoom has actually changed

[Luke Macpherson]

Created attachment 101847 [details]
Patch

[Luke Macpherson]

Ping!

[Simon Fraser (smfr)]

Comment on attachment 101847 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101847&action=review

> Source/WebCore/ChangeLog:8
> +        Don't set m_fontDirty when setting zoom unless zoom has actually changed
> +        https://bugs.webkit.org/show_bug.cgi?id=65092
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests / cleanup.

Why?

[Luke Macpherson]

Created attachment 102470 [details]
Patch

[Darin Adler]

Comment on attachment 102470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102470&action=review

> Source/WebCore/rendering/style/RenderStyle.h:948
> +    template<typename T> inline T setZoom(float f)

Even though we do need a version of the function without the bool return value in one place, the template is awkward overkill. We can simply have setZoomWithoutReturnValue, which can turn around and call setZoom but ignore its return value. And use it in AnimationBase.cpp.

[Luke Macpherson]

Created attachment 102479 [details]
Patch

[Luke Macpherson]

Comment on attachment 102470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102470&action=review

>> Source/WebCore/rendering/style/RenderStyle.h:948
>> +    template<typename T> inline T setZoom(float f)
> 
> Even though we do need a version of the function without the bool return value in one place, the template is awkward overkill. We can simply have setZoomWithoutReturnValue, which can turn around and call setZoom but ignore its return value. And use it in AnimationBase.cpp.

done.

[Darin Adler]

Comment on attachment 102479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102479&action=review

> Source/WebCore/rendering/style/RenderStyle.h:963
> +    bool setZoom(float f)
> +    {
> +        if (compareEqual(visual->m_zoom, f))
> +            return false;
> +        visual.access()->m_zoom = f;
> +        setEffectiveZoom(effectiveZoom() * zoom());
> +        return true;
> +    }
> +    void setZoomWithoutReturnValue(float f) { setZoom(f); }
> +    bool setEffectiveZoom(float f)
> +    {
> +        if (compareEqual(rareInheritedData->m_effectiveZoom, f))
> +            return false;
> +        rareInheritedData.access()->m_effectiveZoom = f;
> +        return true;
> +    }

Now that these function bodies are long, it might be worthwhile to put them outside the class definition, so you can read class definition more easily.

[Luke Macpherson]

(In reply to comment #8)
> (From update of attachment 102479 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102479&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.h:963
> > +    bool setZoom(float f)
> > +    {
> > +        if (compareEqual(visual->m_zoom, f))
> > +            return false;
> > +        visual.access()->m_zoom = f;
> > +        setEffectiveZoom(effectiveZoom() * zoom());
> > +        return true;
> > +    }
> > +    void setZoomWithoutReturnValue(float f) { setZoom(f); }
> > +    bool setEffectiveZoom(float f)
> > +    {
> > +        if (compareEqual(rareInheritedData->m_effectiveZoom, f))
> > +            return false;
> > +        rareInheritedData.access()->m_effectiveZoom = f;
> > +        return true;
> > +    }
> 
> Now that these function bodies are long, it might be worthwhile to put them outside the class definition, so you can read class definition more easily.

I see your point. The counter argument is to keep it here to preserve the existing inlining behavior, since I perceive that the intent in RenderStyle is that all the setters and getters are inlined.

I might cq+ this as is, but if you disagree with that reasoning then let me know and I'll follow up with a patch to move it into the .cpp.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > Now that these function bodies are long, it might be worthwhile to put them outside the class definition, so you can read class definition more easily.
> 
> I see your point. The counter argument is to keep it here to preserve the existing inlining behavior

You should not have to sacrifice inlining to achieve this. You can put the function definitions in the header outside the class definition and use the inline keyword.

[Luke Macpherson]

Ah, ok, I misunderstood what you were asking for. Will do.

[Luke Macpherson]

Created attachment 102607 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 102607 [details]
Patch for landing

Clearing flags on attachment: 102607

Committed r92164: <http://trac.webkit.org/changeset/92164>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.