Summary: | Don't set m_fontDirty when setting zoom unless zoom has actually changed | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Luke Macpherson <macpherson> | ||||||||||
Component: | New Bugs | Assignee: | Luke Macpherson <macpherson> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, eric, macpherson, simon.fraser, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Luke Macpherson
2011-07-24 20:46:28 PDT
Created attachment 101847 [details]
Patch
Ping! 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? Created attachment 102470 [details]
Patch
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. Created attachment 102479 [details]
Patch
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. 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. (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. (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. Ah, ok, I misunderstood what you were asking for. Will do. Created attachment 102607 [details]
Patch for landing
Comment on attachment 102607 [details] Patch for landing Clearing flags on attachment: 102607 Committed r92164: <http://trac.webkit.org/changeset/92164> All reviewed patches have been landed. Closing bug. |