Attachment 343545 Details for Bug 186968 – Patch v3

[patch] Patch v3

bug-186968-20180625145452.patch (text/plain), 14.32 KB, created by David Kilzer (:ddkilzer) on 2018-06-25 14:55:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: David Kilzer (:ddkilzer)

Created: 2018-06-25 14:55:47 PDT

Size: 14.32 KB

patch

obsolete

>Subversion Revision: 233170
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 2c6e6df793ce1129fcd5946b8172bdac83f105ce..afb5f84ff6698a1e373aa47922d2a096b1848796 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,61 @@
>+2018-06-25  David Kilzer  <ddkilzer@apple.com>
>+
>+        Fix clang static analyzer warnings: Branch condition evaluates to a garbage value
>+        <https://webkit.org/b/186968>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch changes two stack-allocated `bool` variables into
>+        `std::optional<bool>` since the functions that set the variable
>+        may return early without setting it.  It also changes one
>+        stack-allocated pointer to be initialized to `nullptr`.
>+
>+        * animation/AnimationTimeline.cpp:
>+        (WebCore::AnimationTimeline::updateCSSTransitionsForElement):
>+        Update for change to CSSPropertyAnimation::getPropertyAtIndex()
>+        argument type.
>+
>+        * editing/ios/EditorIOS.mm:
>+        (WebCore::Editor::writeImageToPasteboard): Initialize
>+        `cachedImage` stack pointer to nullptr since getImage() has an
>+        early return that doesn't set `cachedImage`.
>+        * editing/mac/EditorMac.mm:
>+        (WebCore::Editor::writeImageToPasteboard): Ditto.
>+
>+        * page/animation/CSSPropertyAnimation.cpp:
>+        (WebCore::CSSPropertyAnimation::getPropertyAtIndex):
>+        * page/animation/CSSPropertyAnimation.h:
>+        (WebCore::CSSPropertyAnimation::getPropertyAtIndex):
>+        - Change method to take `std::optional<bool>` instead of `bool`
>+          as second argument since the method may return early without
>+          setting `isShorthand`.
>+
>+        * page/animation/CompositeAnimation.cpp:
>+        (WebCore::CompositeAnimation::updateTransitions): Update for
>+        change to CSSPropertyAnimation::getPropertyAtIndex() argument
>+        type.
>+
>+        * rendering/InlineFlowBox.cpp:
>+        (WebCore::InlineFlowBox::placeBoxesInBlockDirection): Also
>+        rename local `emphasisMarkIsOver` to `emphasisMarkIsAbove` to
>+        match other call sites.
>+        (WebCore::InlineFlowBox::addTextBoxVisualOverflow):
>+        (WebCore::InlineFlowBox::computeOverAnnotationAdjustment const):
>+        (WebCore::InlineFlowBox::computeUnderAnnotationAdjustment const):
>+        - Update for change to InlineTextBox::emphasisMarkExistsAndIsAbove()
>+          argument type.
>+        * rendering/InlineTextBox.cpp:
>+        (WebCore::InlineTextBox::emphasisMarkExistsAndIsAbove const):
>+        - Change method to take `std::optional<bool>` instead of `bool`
>+          as second argument since the method may return early without
>+          setting `above`.
>+        (WebCore::InlineTextBox::paintMarkedTextForeground):
>+        - Update for change to InlineTextBox::emphasisMarkExistsAndIsAbove()
>+          argument type.
>+        * rendering/InlineTextBox.h:
>+        (WebCore::InlineTextBox::emphasisMarkExistsAndIsAbove const):
>+        - Change method to take `std::optional<bool>` instead of `bool`.
>+
> 2018-06-25  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         REGRESSION (r233140): Windows build failure due to incomplete FrameView and RenderBox types
>diff --git a/Source/WebCore/animation/AnimationTimeline.cpp b/Source/WebCore/animation/AnimationTimeline.cpp
>index 3f69cb169ab663e78e0cec061d137637a3681e37..ad1e43645a47fa4488bb1a32ff6baa533d047cd0 100644
>--- a/Source/WebCore/animation/AnimationTimeline.cpp
>+++ b/Source/WebCore/animation/AnimationTimeline.cpp
>@@ -320,9 +320,9 @@ void AnimationTimeline::updateCSSTransitionsForElement(Element& element, const R
> 
>     auto numberOfProperties = CSSPropertyAnimation::getNumProperties();
>     for (int propertyIndex = 0; propertyIndex < numberOfProperties; ++propertyIndex) {
>-        bool isShorthand;
>+        std::optional<bool> isShorthand;
>         auto property = CSSPropertyAnimation::getPropertyAtIndex(propertyIndex, isShorthand);
>-        if (isShorthand)
>+        if (isShorthand && *isShorthand)
>             continue;
> 
>         const Animation* matchingBackingAnimation = nullptr;
>diff --git a/Source/WebCore/editing/ios/EditorIOS.mm b/Source/WebCore/editing/ios/EditorIOS.mm
>index 6b378b21fe6b42dbe50d783ff0d66deac8edfed4..c99b1e470a16ba5a6b9fa44c4cd356a50f60a179 100644
>--- a/Source/WebCore/editing/ios/EditorIOS.mm
>+++ b/Source/WebCore/editing/ios/EditorIOS.mm
>@@ -189,7 +189,7 @@ void Editor::writeImageToPasteboard(Pasteboard& pasteboard, Element& imageElemen
>     PasteboardImage pasteboardImage;
> 
>     RefPtr<Image> image;
>-    CachedImage* cachedImage;
>+    CachedImage* cachedImage = nullptr;
>     getImage(imageElement, image, cachedImage);
>     if (!image)
>         return;
>diff --git a/Source/WebCore/editing/mac/EditorMac.mm b/Source/WebCore/editing/mac/EditorMac.mm
>index a1f4563f26d889ccfc6d96d509eec3e54ae8933a..cf289bf70e400036767808d1a1699f68237aac00 100644
>--- a/Source/WebCore/editing/mac/EditorMac.mm
>+++ b/Source/WebCore/editing/mac/EditorMac.mm
>@@ -263,7 +263,7 @@ void Editor::writeImageToPasteboard(Pasteboard& pasteboard, Element& imageElemen
> {
>     PasteboardImage pasteboardImage;
> 
>-    CachedImage* cachedImage;
>+    CachedImage* cachedImage = nullptr;
>     getImage(imageElement, pasteboardImage.image, cachedImage);
>     if (!pasteboardImage.image)
>         return;
>diff --git a/Source/WebCore/page/animation/CSSPropertyAnimation.cpp b/Source/WebCore/page/animation/CSSPropertyAnimation.cpp
>index b3f0c5a1df075702818145967d38b42aad380201..86d9dba69d7f67b3983a03ad9110ae1c66c3924d 100644
>--- a/Source/WebCore/page/animation/CSSPropertyAnimation.cpp
>+++ b/Source/WebCore/page/animation/CSSPropertyAnimation.cpp
>@@ -1790,7 +1790,7 @@ bool CSSPropertyAnimation::canPropertyBeInterpolated(CSSPropertyID prop, const R
>     return false;
> }
> 
>-CSSPropertyID CSSPropertyAnimation::getPropertyAtIndex(int i, bool& isShorthand)
>+CSSPropertyID CSSPropertyAnimation::getPropertyAtIndex(int i, std::optional<bool>& isShorthand)
> {
>     CSSPropertyAnimationWrapperMap& map = CSSPropertyAnimationWrapperMap::singleton();
> 
>diff --git a/Source/WebCore/page/animation/CSSPropertyAnimation.h b/Source/WebCore/page/animation/CSSPropertyAnimation.h
>index d3bd9c4e92346e08700a877c190ecff7c991ed26..acb24fe80029a8e325a4eb12901e6173aff7bc31 100644
>--- a/Source/WebCore/page/animation/CSSPropertyAnimation.h
>+++ b/Source/WebCore/page/animation/CSSPropertyAnimation.h
>@@ -42,7 +42,7 @@ public:
>     static bool animationOfPropertyIsAccelerated(CSSPropertyID);
>     static bool propertiesEqual(CSSPropertyID, const RenderStyle* a, const RenderStyle* b);
>     static bool canPropertyBeInterpolated(CSSPropertyID, const RenderStyle* a, const RenderStyle* b);
>-    static CSSPropertyID getPropertyAtIndex(int, bool& isShorthand);
>+    static CSSPropertyID getPropertyAtIndex(int, std::optional<bool>& isShorthand);
>     static int getNumProperties();
> 
>     static HashSet<CSSPropertyID> animatableShorthandsAffectingProperty(CSSPropertyID);
>diff --git a/Source/WebCore/page/animation/CompositeAnimation.cpp b/Source/WebCore/page/animation/CompositeAnimation.cpp
>index 1db6234a6ec747ae006030b15a83049c43e94ef2..4d5e4f0dfc2aa2bed7fe686222cc9b5fec0d1a59 100644
>--- a/Source/WebCore/page/animation/CompositeAnimation.cpp
>+++ b/Source/WebCore/page/animation/CompositeAnimation.cpp
>@@ -109,9 +109,9 @@ void CompositeAnimation::updateTransitions(Element& element, const RenderStyle*
>             for (int propertyIndex = 0; propertyIndex < CSSPropertyAnimation::getNumProperties(); ++propertyIndex) {
>                 if (all) {
>                     // Get the next property which is not a shorthand.
>-                    bool isShorthand;
>+                    std::optional<bool> isShorthand;
>                     prop = CSSPropertyAnimation::getPropertyAtIndex(propertyIndex, isShorthand);
>-                    if (isShorthand)
>+                    if (isShorthand && *isShorthand)
>                         continue;
>                 }
> 
>diff --git a/Source/WebCore/rendering/InlineFlowBox.cpp b/Source/WebCore/rendering/InlineFlowBox.cpp
>index 80f3c91e3e8a0d3342e3ac655161bc15618d556f..6cf0e7625be0d5d539a678cde0e30e25143be1ef 100644
>--- a/Source/WebCore/rendering/InlineFlowBox.cpp
>+++ b/Source/WebCore/rendering/InlineFlowBox.cpp
>@@ -698,9 +698,9 @@ void InlineFlowBox::placeBoxesInBlockDirection(LayoutUnit top, LayoutUnit maxHei
>                 }
>             }
>             if (is<InlineTextBox>(*child)) {
>-                bool emphasisMarkIsOver;
>-                if (downcast<InlineTextBox>(*child).emphasisMarkExistsAndIsAbove(childLineStyle, emphasisMarkIsOver)) {
>-                    if (emphasisMarkIsOver != childLineStyle.isFlippedLinesWritingMode())
>+                std::optional<bool> emphasisMarkIsAbove;
>+                if (downcast<InlineTextBox>(*child).emphasisMarkExistsAndIsAbove(childLineStyle, emphasisMarkIsAbove)) {
>+                    if (emphasisMarkIsAbove && (*emphasisMarkIsAbove != childLineStyle.isFlippedLinesWritingMode()))
>                         hasAnnotationsBefore = true;
>                     else
>                         hasAnnotationsAfter = true;
>@@ -894,10 +894,10 @@ inline void InlineFlowBox::addTextBoxVisualOverflow(InlineTextBox& textBox, Glyp
>     int leftGlyphOverflow = -strokeOverflow - leftGlyphEdge;
>     int rightGlyphOverflow = strokeOverflow + rightGlyphEdge;
> 
>-    bool emphasisMarkIsAbove;
>+    std::optional<bool> emphasisMarkIsAbove;
>     if (lineStyle.textEmphasisMark() != TextEmphasisMark::None && textBox.emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkIsAbove)) {
>         int emphasisMarkHeight = lineStyle.fontCascade().emphasisMarkHeight(lineStyle.textEmphasisMarkString());
>-        if (emphasisMarkIsAbove == !lineStyle.isFlippedLinesWritingMode())
>+        if (emphasisMarkIsAbove && (*emphasisMarkIsAbove == !lineStyle.isFlippedLinesWritingMode()))
>             topGlyphOverflow = std::min(topGlyphOverflow, -emphasisMarkHeight);
>         else
>             bottomGlyphOverflow = std::max(bottomGlyphOverflow, emphasisMarkHeight);
>@@ -1575,8 +1575,8 @@ LayoutUnit InlineFlowBox::computeOverAnnotationAdjustment(LayoutUnit allowedPosi
> 
>         if (is<InlineTextBox>(*child)) {
>             const RenderStyle& childLineStyle = child->lineStyle();
>-            bool emphasisMarkIsAbove;
>-            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && downcast<InlineTextBox>(*child).emphasisMarkExistsAndIsAbove(childLineStyle, emphasisMarkIsAbove) && emphasisMarkIsAbove) {
>+            std::optional<bool> emphasisMarkIsAbove;
>+            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && downcast<InlineTextBox>(*child).emphasisMarkExistsAndIsAbove(childLineStyle, emphasisMarkIsAbove) && emphasisMarkIsAbove && *emphasisMarkIsAbove) {
>                 if (!childLineStyle.isFlippedLinesWritingMode()) {
>                     int topOfEmphasisMark = child->logicalTop() - childLineStyle.fontCascade().emphasisMarkHeight(childLineStyle.textEmphasisMarkString());
>                     result = std::max(result, allowedPosition - topOfEmphasisMark);
>@@ -1623,9 +1623,9 @@ LayoutUnit InlineFlowBox::computeUnderAnnotationAdjustment(LayoutUnit allowedPos
> 
>         if (is<InlineTextBox>(*child)) {
>             const RenderStyle& childLineStyle = child->lineStyle();
>-            bool emphasisMarkIsAbove;
>+            std::optional<bool> emphasisMarkIsAbove;
>             downcast<InlineTextBox>(*child).emphasisMarkExistsAndIsAbove(childLineStyle, emphasisMarkIsAbove);
>-            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && !emphasisMarkIsAbove) {
>+            if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && emphasisMarkIsAbove && !*emphasisMarkIsAbove) {
>                 if (!childLineStyle.isFlippedLinesWritingMode()) {
>                     LayoutUnit bottomOfEmphasisMark = child->logicalBottom() + childLineStyle.fontCascade().emphasisMarkHeight(childLineStyle.textEmphasisMarkString());
>                     result = std::max(result, bottomOfEmphasisMark - allowedPosition);
>diff --git a/Source/WebCore/rendering/InlineTextBox.cpp b/Source/WebCore/rendering/InlineTextBox.cpp
>index cea1cd00edb004e37edac5cd4f3060bacf226e6a..8910b1b3cae4d5900e00367a9bfa15d28ccd305d 100644
>--- a/Source/WebCore/rendering/InlineTextBox.cpp
>+++ b/Source/WebCore/rendering/InlineTextBox.cpp
>@@ -358,7 +358,7 @@ static inline bool emphasisPositionHasNeitherLeftNorRight(OptionSet<TextEmphasis
>     return !(emphasisPosition & TextEmphasisPosition::Left) && !(emphasisPosition & TextEmphasisPosition::Right);
> }
> 
>-bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const
>+bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const
> {
>     // This function returns true if there are text emphasis marks and they are suppressed by ruby text.
>     if (style.textEmphasisMark() == TextEmphasisMark::None)
>@@ -1001,11 +1001,11 @@ void InlineTextBox::paintMarkedTextForeground(PaintInfo& paintInfo, const FloatR
>     const RenderStyle& lineStyle = this->lineStyle();
> 
>     float emphasisMarkOffset = 0;
>-    bool emphasisMarkAbove;
>+    std::optional<bool> emphasisMarkAbove;
>     bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove);
>     const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom();
>     if (!emphasisMark.isEmpty())
>-        emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
>+        emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
> 
>     TextPainter textPainter { context };
>     textPainter.setFont(font);
>diff --git a/Source/WebCore/rendering/InlineTextBox.h b/Source/WebCore/rendering/InlineTextBox.h
>index 1de69bc59feab1eda7c30bb3009a33087e74e958..065408726ee391b28a02249a5fa6311aeb46b954 100644
>--- a/Source/WebCore/rendering/InlineTextBox.h
>+++ b/Source/WebCore/rendering/InlineTextBox.h
>@@ -87,7 +87,7 @@ public:
>     int baselinePosition(FontBaseline) const final;
>     LayoutUnit lineHeight() const final;
> 
>-    bool emphasisMarkExistsAndIsAbove(const RenderStyle&, bool& isAbove) const;
>+    bool emphasisMarkExistsAndIsAbove(const RenderStyle&, std::optional<bool>& isAbove) const;
> 
>     LayoutRect logicalOverflowRect() const;
>     void setLogicalOverflowRect(const LayoutRect&);

Actions: View | Formatted Diff | Diff

Attachments on bug 186968: 343432 | 343483 | 343487 | 343545 | 343621